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

Triangle: Return null in getBarycoord() if the triangle is degenerated. #27311

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

gkjohnson
Copy link
Collaborator

Related issue: gkjohnson/three-bvh-csg#183

Description

Previously the getBarycoord function was providing a reasonable looking result where no reasonable value could be provided when a degenerate triangle was used. This makes tracking errors down extremely difficult. Now an error will be thrown if a degenerate triangle is used.

@gkjohnson gkjohnson added this to the r160 milestone Dec 5, 2023
@gkjohnson gkjohnson changed the title Triangle: Throw an error when get the barycoord of a degenerate triangle Triangle: Throw an error when getting the barycoord of a degenerate triangle Dec 5, 2023
Copy link

github-actions bot commented Dec 5, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
669.1 kB (166.1 kB) 669.1 kB (166.1 kB) +12 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
450.1 kB (109 kB) 450.1 kB (109 kB) +12 B

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 5, 2023

It seems the unit tests require an update now.

Triangle.getBarycoord() is also used in context of ray casting and I fear a less graceful approach could produce issues. If you think throwing an error is better to return and undefined result, callers of Triangle.getBarycoord() in the repository should implement an error handler. That is Triangle.getInterpolation() and Triangle.containsPoint(). I think it would be not good to see unhandled exceptions when user perform ray casting. Even if triangles are degenerated.

@gkjohnson
Copy link
Collaborator Author

If you think throwing an error is better to return and undefined result, callers of Triangle.getBarycoord() in the repository should implement an error handler.

I just changed the function to return null. But the reality of it is that all of these functions are already returning unreasonable results if they're not checking for degenerate triangles (none of them are). If we think this is a possibility then this these functions should already be handling these cases.

Documenting the possibility and returning null will hopefully make this easier to detect / understand. The biggest issue is that the result looked reasonable.

@Mugen87 Mugen87 changed the title Triangle: Throw an error when getting the barycoord of a degenerate triangle Triangle: Return null if the triangle is degenerated. Dec 6, 2023
@Mugen87 Mugen87 merged commit 62a2a0c into mrdoob:dev Dec 6, 2023
12 checks passed
@gkjohnson gkjohnson deleted the barycoord-error branch December 7, 2023 02:07
@Mugen87 Mugen87 changed the title Triangle: Return null if the triangle is degenerated. Triangle: Return null in getBarycoord() if the triangle is degenerated. Dec 8, 2023
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
* BatchedMesh: Add sorting and frustum culling for shadows

* Triangle.getBarycoord: throw error if a degenerate triangle is used

* Return null, instead

* Fix test

* Fix containsPoint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants