Skip to content

Triangle: Return null in getInterpolation() if triangle is degenerate. #27331

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 8, 2023
Merged

Triangle: Return null in getInterpolation() if triangle is degenerate. #27331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
903d45f
Return null if triangle is degenerate
gkjohnson Dec 7, 2023
4568ac5
Update docs
gkjohnson Dec 7, 2023
5c75280
Ensure we support for vector4 and vector2
gkjohnson Dec 8, 2023
40b0c30
Update Triangle.html
Mugen87 Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/api/en/math/Triangle.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ <h3>
[page:Vector3 target] — the result will be copied into this Vector3.<br /><br />

Return a [link:https://en.wikipedia.org/wiki/Barycentric_coordinate_system barycentric coordinate]
from the given vector. Returns null if the triangle is degenerate.<br /><br />
from the given vector. Returns `null` if the triangle is degenerate.<br /><br />

[link:http://commons.wikimedia.org/wiki/File:Barycentric_coordinates_1.png Picture of barycentric coordinates]
</p>
Expand Down Expand Up @@ -137,7 +137,7 @@ <h3>
[page:Vector target] — Result will be copied into this Vector.<br /><br />

Returns the value barycentrically interpolated for the given point on the
triangle.
triangle. Returns `null` if the triangle is degenerate.
</p>

<h3>[method:Boolean intersectsBox]( [param:Box3 box] )</h3>
Expand Down
10 changes: 9 additions & 1 deletion src/math/Triangle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,15 @@ class Triangle {

static getInterpolation( point, p1, p2, p3, v1, v2, v3, target ) {

this.getBarycoord( point, p1, p2, p3, _v3 );
if ( this.getBarycoord( point, p1, p2, p3, _v3 ) === null ) {

target.x = 0;
target.y = 0;
if ( 'z' in target ) target.z = 0;
if ( 'w' in target ) target.w = 0;
return null;
Copy link
Collaborator

@Mugen87 Mugen87 Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

getInterpolation() is used in context of ray casting to determine uv coordinates and normals (in Mesh and Sprite). I think we have to handle the case now when this method returns null and apply more appropriate default/fallback values instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would a better default be here? Previously it would have just been returning incorrect values.

And either way is it the case that this can cause a problem with raycasting? It seems that the raycaster does not intersect with degenerate triangles:

const buff = new Float32Array( [ 1, 1, 1, 1, 1, 1, 1, 1, 1 ] );
const geom = new THREE.BufferGeometry();
geom.setAttribute( 'position', new THREE.BufferAttribute( buff ) );

const mesh = new THREE.Mesh( geom );
const raycaster = new THREE.Raycaster();
raycaster.ray.origin.set( 0, 0, - 1 );
raycaster.ray.origin.set( 0, 0, 1 );

raycaster.intersectObject( mesh );
// []

Copy link
Collaborator

@Mugen87 Mugen87 Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What would a better default be here? Previously it would have just been returning incorrect values.

Um, I guess I would return the data type that is expected in the intersection object (e.g. Vector2 or Vector3). Probably with initial numbers or maybe NaN. But if there can't be an intersection anyway, it probably doesn't matter.

The issue is that we silently ignored such issues so far. When we now start making methods more strict, we have to think about how we want to handle edge cases. I'm not 100% sure on my side what to recommend, tbh.

Copy link
Collaborator

@Mugen87 Mugen87 Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

BTW: Is it possible getInterpolation() could be used with an instance of Vector4? If so, target.set( 0, 0, 0 ); isn't safe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I say we leave it as potentially setting null for the raycast results for now - so it will be obvious if something isn't working. And we can reevaluate if it turns out people are getting null results.

BTW: Is it possible getInterpolation() could be used with an instance of Vector4? If so, target.set( 0, 0, 0 ); isn't safe.

You're right! Just made the change.


}

target.setScalar( 0 );
target.addScaledVector( v1, _v3.x );
Expand Down