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

Fix mixed smoothed and non-smoothed face normals computation for CSG shapes #59039

Merged

Conversation

MythTitans
Copy link
Contributor

@MythTitans MythTitans commented Mar 11, 2022

When computing smoothed normals on CSG shapes, the normals from all the faces sharing a vertex are averaged together to compute the smoothed "shared" normal between the faces.

I don't think the behavior is correct, because it means that even faces that are not smoothed participate to the computation of their smooth neighbor's faces.

The problem is particularly visible when substracting a sphere or a torus from a box, at the edge between them :

Before
smooth_normals_before

After
smooth_normals_after

and also on the cylinder which has a strange shading (and a small glitch is visible at the bottom left) :

Before
cylinder_bottom_before

After
cylinder_bottom_after

This is related to some comments in #49800.

Bugsquad edit:

@MythTitans MythTitans requested a review from a team as a code owner March 11, 2022 18:10
@akien-mga akien-mga added this to the 4.0 milestone Mar 11, 2022
@akien-mga akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.4 labels Mar 11, 2022
@akien-mga
Copy link
Member

CC @hoontee

modules/csg/csg_shape.cpp Outdated Show resolved Hide resolved
@hoontee
Copy link
Contributor

hoontee commented Mar 11, 2022

Fixes #49800.

@akien-mga
Copy link
Member

After fixing the style issue, be sure to rebase to squash the commits into one (see https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase).

@Calinou
Copy link
Member

Calinou commented Mar 11, 2022

By the way, this likely supersedes godotengine/godot-proposals#1862. Great work 🙂

@MythTitans
Copy link
Contributor Author

@akien-mga Thanks, I'll read and try that

@MythTitans MythTitans force-pushed the fix-smoothed-normals-computation branch from d323010 to a7f4a3e Compare March 11, 2022 19:06
@MythTitans MythTitans force-pushed the fix-smoothed-normals-computation branch from a7f4a3e to ec2984f Compare March 11, 2022 19:24
@hoontee
Copy link
Contributor

hoontee commented Mar 11, 2022

By the way, this likely supersedes godotengine/godot-proposals#1862. Great work 🙂

No, there are still instances where angle based smoothing would be appreciable. Note the odd lighting on the cylinder due to a lack of hard edges around the sphere negation:
image

@MythTitans MythTitans removed the request for review from a team March 11, 2022 19:58
@akien-mga akien-mga merged commit d507643 into godotengine:master Mar 11, 2022
@akien-mga
Copy link
Member

Thanks!

@MythTitans MythTitans deleted the fix-smoothed-normals-computation branch March 11, 2022 22:51
@akien-mga
Copy link
Member

Cherry-picked for 3.5.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Mar 12, 2022
@akien-mga
Copy link
Member

Cherry-picked for 3.4.4.

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.

CSGCylinder3D has incorrect normals compared to the CylinderMesh primitive
4 participants