-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Sphere Fix: Add branch for empty condition in expandByPoint function #24694
Conversation
Nice find! I expect /ping @Mugen87 |
What do you mean by this? Do you mean the comment source references? It does look like union would have the same issue. It should be simple to handle the empty cases for both spheres in that function. |
In Also, the "nudge" comment makes it sound like it's a hack. It's not. |
It looks like the nudge comment is from the original source from which this was ported 🤷: I can plan to at least fix the union function in another PR when I get a chance unless someone beats me to it. I'll let someone else rewrite the functions if necessary. |
Why is the default radius -1 and not 0? It seems |
-1 is used to indicate "empty" meaning both the sphere center and radius are invalid. If "empty" was denoted by a radius of 0 then it would be impossible to tell the difference between a sphere with a center that needed to be initialized by the first point and a valid bounding sphere that encapsulated a single point at the origin. |
When do you need this kind of distinction? Does it actually matter? |
The code in this PR shows the very case where this distinction is important... here is what will happen when expanding by a point Radius == 0, Center == 0, 0, 0
Radius == - 1, Center == 0, 0, 0
|
Thanks! |
Related issue: --
Description
Currently when calling "Sphere.expandByPoint" on an "empty" sphere with a point that is < 1.0 distance from the sphere center nothing will happen because the radius is squared before checking if the point is already encapsulated:
cc @WestLangley