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 gap in primitive circular meshes due to floating point error #94199

Closed

Conversation

ayanchavand
Copy link
Contributor

Fixes #93865
This is my first engine contribution besides docs therefore I have only fixed the sphere mesh for now as I am sure there will be changes needed in my implementation.

  • Added 4 functions including custom_sin , custom_cos , remove_negative_zero_vector3 and remove_negative_zero
  • Replaced sin and cos with custom functions for fixing the floating point error and used remove_negative_zero for standard 0 comparison
    image

@AThousandShips AThousandShips added bug topic:3d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 11, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Jul 11, 2024
scene/resources/3d/primitive_meshes.cpp Outdated Show resolved Hide resolved
scene/resources/3d/primitive_meshes.cpp Outdated Show resolved Hide resolved
scene/resources/3d/primitive_meshes.cpp Outdated Show resolved Hide resolved
scene/resources/3d/primitive_meshes.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips requested a review from a team July 11, 2024 08:43
ayanchavand and others added 4 commits July 11, 2024 14:34
Co-authored-by: A Thousand Ships <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
@AThousandShips
Copy link
Member

It would be good to add some unit tests for this in tests/scene/test_primitives.h to make sure it works correctly

@ayanchavand
Copy link
Contributor Author

I have never tried unit testing before, can you share some resources as to how I can implement it for this case?

@AThousandShips
Copy link
Member

See this and you can look at the existing code to learn how it's used

Co-authored-by: A Thousand Ships <[email protected]>
@Lielay9
Copy link
Contributor

Lielay9 commented Jul 11, 2024

The Torus and Capsule still have gaps (since nothing was done to them) and the cylinder still has gaps in the rings of the caps.

Additionally the approach taken is bit worrisome, as there is at least a theoretical possibility that vertices that should be different will be snapped to the same position if they're close enough (difference is less than epsilon). The sin and cos functions were fine, the problem was with the last vertex in the ring not matching the first; simpler approach would be to make the last iterations position zero with an if statement (whilst ensuring the UV:s remain correct).

@fire
Copy link
Member

fire commented Dec 5, 2024

Hi, I also work on CSG, would it be ok to also test CSGMesh3D too?

@fire
Copy link
Member

fire commented Dec 5, 2024

Superseded by: #100020

@fire fire closed this Dec 5, 2024
@akien-mga akien-mga added archived and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Dec 5, 2024
@akien-mga akien-mga removed this from the 4.4 milestone Dec 5, 2024
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.

Round primitive meshes contain gaps.
5 participants