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

Automatically ensure correct normals in Compatibility renderer #82804

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Oct 4, 2023

Fixes #82763

The ensure_correct_normals define is redundant in the Forward+ and Mobile renderers, as they check whether individual instances are uniform or not. Due to the way instancing is done in the Compatibility renderer, however, the ENSURE_CORRECT_NORMALS shader define has to be used instead.

This PR changes the shader's logic to automatically figure out which objects have a non-uniform scale.

Results (Compatibility):

aaa

Progress:

  • Implementation for non-instanced rendering

@BlueCube3310 BlueCube3310 requested a review from a team as a code owner October 4, 2023 17:52
@BlueCube3310 BlueCube3310 force-pushed the compat_correct_normals branch from 1e8a23a to 8ab0c94 Compare October 4, 2023 17:57
@Calinou Calinou added this to the 4.x milestone Oct 4, 2023
@clayjohn
Copy link
Member

clayjohn commented Oct 4, 2023

As Calinou mentioned earlier #82763 (comment), I think we should make this consistent with the Forward+ and Mobile renderers instead of making it use the render mode. Matching behaviour between the renderers is more important than saving a single integer uniform

@BlueCube3310 BlueCube3310 marked this pull request as draft October 4, 2023 21:19
@BlueCube3310 BlueCube3310 changed the title Support ensure_correct_normals in Compatibility renderer Ensure correct normals automatically in Compatibility renderer Oct 4, 2023
@BlueCube3310 BlueCube3310 changed the title Ensure correct normals automatically in Compatibility renderer Automatically ensure correct normals in Compatibility renderer Oct 4, 2023
@BlueCube3310 BlueCube3310 force-pushed the compat_correct_normals branch 2 times, most recently from 2c13eba to 4de577c Compare October 22, 2023 09:50
@BlueCube3310 BlueCube3310 force-pushed the compat_correct_normals branch from 4de577c to f31371c Compare October 22, 2023 13:01
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great!

I can confirm that this fixes the reported issue.

@clayjohn clayjohn marked this pull request as ready for review November 13, 2023 16:43
@clayjohn clayjohn modified the milestones: 4.x, 4.3 Nov 13, 2023
@clayjohn clayjohn added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 13, 2023
@Calinou
Copy link
Member

Calinou commented Nov 13, 2023

  • Implementation for instanced rendering

Is this implemented now in the latest revision of this PR? If not, I suggest opening another issue to track this once this is merged.

@BlueCube3310
Copy link
Contributor Author

BlueCube3310 commented Nov 14, 2023

I haven't made much progress on the instanced rendering part, so I will do as suggested.

Edit: I've opened the new issue here: #84903

@akien-mga akien-mga merged commit 1749ea8 into godotengine:master Dec 4, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

@BlueCube3310 BlueCube3310 deleted the compat_correct_normals branch April 25, 2024 10:23
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.

Render mode ensure_correct_normals seemingly not working
5 participants