-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
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 getInterpolation()
if triangle is degenerate.
#27331
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
if ( this.getBarycoord( point, p1, p2, p3, _v3 ) === null ) { | ||
|
||
target.set( 0, 0, 0 ); | ||
return null; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 );
// []
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
null
in getInterpolation()
if triangle is degenerate.
…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 <michael.herzog@human-interactive.org>
Related issue: #27311
Description
I had intended to make this one last change in the other PR - but this changes
getInterpolation
to behave similarly togetBarycoord
in that it now returns null if the triangle is degenerate.