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 normals of very large SphereMesh and CapsuleMesh #98610

Merged
merged 1 commit into from
Dec 14, 2024

Conversation

Flarkk
Copy link
Contributor

@Flarkk Flarkk commented Oct 28, 2024

SphereMesh and CapsuleMesh compute their sphere normals by normalizing the vertex position.
Normalizing involves squaring the vertex coordinates, which generate INFINITE values if at least one of the coordinates is larger than sqrt(MAX_FLOAT_VALUE).
With 32 bit floats, this threshold value is ~1.84E19. With 64 bit floats it's ~1.34E154.

This PR fixes this behavior by dividing the vertex position by the radius before normalizing.
This doesn't change the end result of the normalization, but avoid running into the floating point precision issue.

[EDIT] This PR fixes this behavior by working with unit length vectors all the way down, and scaling with radius only to calculate vertex positions.
Radius is not involved anymore in normal computation logic, which avoids running into the floating point precision issue.

I tested the other primitives and all of them are fine with normals.

Normals with this PR Normals without
SphereMesh of radius 1E28 Capture d’écran du 2024-10-28 11-28-51 Capture d’écran du 2024-10-28 11-34-38
CapsuleMesh of radius 1E28 Capture d’écran du 2024-10-28 11-31-19 Capture d’écran du 2024-10-28 11-34-23

Note : I stumbled upon this issue by working on #95944.
Being able to render very large primitives can be useful in very large worlds (space sims, terrain generation ...).

@Flarkk Flarkk requested review from a team as code owners October 28, 2024 11:07
@AThousandShips AThousandShips added this to the 4.4 milestone Oct 28, 2024
@fire
Copy link
Member

fire commented Oct 28, 2024

Ok with the theory of the enhancement. Did not review yet.

@Flarkk
Copy link
Contributor Author

Flarkk commented Dec 4, 2024

@fire would you have time now to review this one ?

@fire fire requested a review from a team December 4, 2024 18:27
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

That's neat; the rearranging of the equations reduces the precision loss.

I would like someone from the rendering to review the equations, too.

@Flarkk
Copy link
Contributor Author

Flarkk commented Dec 4, 2024

Good. Best if you can ping this person right here then. Thanks !

Copy link
Contributor

@BlueCube3310 BlueCube3310 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I compared the meshes and their UV channels and they look the same, so the equations are correct

@Repiteo
Copy link
Contributor

Repiteo commented Dec 12, 2024

Needs rebase before this can be merged

@Flarkk
Copy link
Contributor Author

Flarkk commented Dec 12, 2024

Will do tomorrow

@Flarkk Flarkk force-pushed the fix_sphere_capsule branch from 3365aee to 4c34813 Compare December 13, 2024 10:22
@Flarkk Flarkk requested a review from a team as a code owner December 13, 2024 10:22
@Flarkk
Copy link
Contributor Author

Flarkk commented Dec 13, 2024

Here we go

@akien-mga akien-mga merged commit 66a2ea4 into godotengine:master Dec 14, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Flarkk Flarkk deleted the fix_sphere_capsule branch December 14, 2024 20:36
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

6 participants