Skip to content

Conversation

@gkjohnson
Copy link
Collaborator

Related issue: #27311

Description

I had intended to make this one last change in the other PR - but this changes getInterpolation to behave similarly to getBarycoord in that it now returns null if the triangle is degenerate.

@gkjohnson gkjohnson added this to the r160 milestone Dec 7, 2023
@github-actions
Copy link

github-actions bot commented Dec 7, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
669 kB (166.1 kB) 669 kB (166.1 kB) +0 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
449.9 kB (109 kB) 449.9 kB (109 kB) +0 B

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

target.set( 0, 0, 0 );
return null;
Copy link
Collaborator

@Mugen87 Mugen87 Dec 7, 2023

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

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

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.

@Mugen87 Mugen87 merged commit 23d8557 into mrdoob:dev Dec 8, 2023
@Mugen87 Mugen87 changed the title Triangle.getInterpolation: Return null if triangle is degenerate Triangle: Return null in getInterpolation() if triangle is degenerate. Dec 8, 2023
@gkjohnson gkjohnson deleted the get-interpolation-corner-case branch December 8, 2023 18:33
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
…oob#27331)

* Return null if triangle is degenerate

* Update docs

* Ensure we support for vector4 and vector2

* Update Triangle.html

---------

Co-authored-by: Michael Herzog <[email protected]>
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.

2 participants