Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-Added screen space reflection. #37512

Merged
merged 1 commit into from Apr 2, 2020
Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Apr 2, 2020

Some changes for Godot 4.0

  • Roughness is now a post-process, the old code sampling mipmaps is gone.
  • Different roughness quality available on project settings
  • Entirely done using Compute
  • Many precission fixes, improves quality

Comment on lines 29 to 30


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is complaining about these two lines.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the pre commit hook that executes black is broken, I had to disable it. I was getting this error all the time and was unable to commit:

Do you want to apply that patch (Y - Apply, N - Do not apply, S - Apply and stage files)? [Y/N/S] Y
error: patch failed: servers/rendering/rasterizer_rd/shaders/SCsub:24
error: servers/rendering/rasterizer_rd/shaders/SCsub: patch does not apply

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that black --diff generates a diff which is not always a valid patch: psf/black#526
I'll check how to work it around :|

Comment on lines 1959 to 1960
RENDER_TIMESTAMP("Sub Surface Scattering");
//call SSS process
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the above, I assume only one is necessary?

u.ids.push_back(p_texture2);
uniforms.push_back(u);
}
//any thing with the same configuration (one texture in binding 0 for set 0), is good
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is out of date

u.ids.push_back(p_texture2);
uniforms.push_back(u);
}
//any thing with the same configuration (one texture in binding 0 for set 0), is good
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, comment is no longer applicable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you know why the codebase doesn't have many comments. Juan's mind parser seems to just skip them and then copy/paste outdated comments at will :P

Comment on lines 8 to 10
/* clang-format on */

layout(local_size_x = 8, local_size_y = 8, local_size_z = 1) in;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First call to "layout" needs to be above /* clang-format on */ to avoid this mess I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, clang-format works ok on C-like GLSL, but it doesn't like our [vertex] and [fragment] custom attributes, so it needs to be off until after the first layout statement to work ok. It's weird and ideally that's a bug that should be fixed upstream, but until then we need to work it around this way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are #defines between the [vertex]/[fragment] and the first layout, we still need to include them in the off block. Everything until the first layout should have clang-format disabled.

Comment on lines 8 to 10
/* clang-format on */

layout(local_size_x = 8, local_size_y = 8, local_size_z = 1) in;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, move this above "clang-format on"

Comment on lines 8 to 10
/* clang-format on */

layout(local_size_x = 8, local_size_y = 8, local_size_z = 1) in;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@akien-mga akien-mga merged commit 6a38ce1 into godotengine:master Apr 2, 2020
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants