MeshSurfaceSampler: interpolate normals

[makc]

This is 2nd change I mentioned in #26207 - it is going to sound silly after @gkjohnson said

At some point this function should really just return a barycoord value and face index so users can interpolate between whatever buffer attributes they want to. Adding a new [code] for every possible buffer attribute doesn't seem sustainable 😅

and yet this is exactly what I am proposing here 😅

The thing is, for the grass you need both the texture and the normal of the terrain to match the color at the base of the blade - then it can seamlessly blend in. Meanwhile MeshSurfaceSampler ignores normal attribute and just calculates flat normal from vertex positions, which causes very visible difference and the blending does not work. I agree with @donmccurdy when he says

Adding that method (sampleBarycentric?) would be ideal

but so far as the method is not there, this PR reflects what I had to do in order to make it work (I did not bother to keep _face.getNormal call in case there is no normal attribute originally since there is hardly ever such a case, but I left it in this PR in case you will have any backward compatibility concerns).

[makc]

(I see that PR fails teh screenshot for webgl_instancing_scatter - it still works but now looks kind of different, I guess)

[donmccurdy]

Thanks @makc! I think this is a strict improvement on the current behavior. Even if we added sampleBarycentric, it still makes sense for the sample method to handle position and normal vectors.

EDIT: We'll want to regenerate the screenshot too.

[makc]

also it just came to me, but do we need to normalize() targetNormal 🤔 it will most likely be normalized in whatever fragment shader is using it, but in javascript land people might count on unit length.

[gkjohnson]

I think this is a strict improvement on the current behavior

Geometric normals and interpolated surface normals can serve different use cases so I'm not sure if I see this as an improvement in some cases because it blocks the ability to reliably retrieve the geometric normals which I've used in the past.

I'm fine with this change but please add the ability to sample the face, barycoord, and index so advanced use cases that require more control and granularity can still function. Something like the following, though I'm flexible on API. This is the data I think should be returned:

sampleBarycoord( targetBarycoord, targetTriangle ) : faceIndex

[donmccurdy]

@gkjohnson what do you mean by "targetTriangle" above? Face index + barycentric coordinates are unambiguous without further information, correct?

EDIT: Hm, also note that given an indexed mesh, MeshSurfaceSampler logs a warning and de-indexes it. That would affect our use of a face index...

[gkjohnson]

what do you mean by "targetTriangle" above? Face index + barycentric coordinates are unambiguous without further information, correct?

You're right - this was more for convenience but I was thinking a Triangle instance with the vertices filled in. Barycoord and face index should be enough, though.

EDIT: Hm, also note that given an indexed mesh, MeshSurfaceSampler logs a warning and de-indexes it. That would affect our use of a face index...

Yes this is something the user has to be aware of. This is a bit of a separate topic but it should be possible to implement this without deindexing the geometry, right?

[donmccurdy]

This is a bit of a separate topic but it should be possible to implement this without deindexing the geometry, right?

Yes, I think that could be fixed. 👍

[makc]

@gkjohnson you could have what you want with only a minor change, that is by making sampleFace() return "u" and "v" instead of "this" - then you could inline sample() in your app code to gain access to face index value and, together with barycentric coords it returns, work the magic on any other attributes beyond position-normal-color-uv

[gkjohnson]

Face index is also needed. And I think a barycoord is also a more common and understandable way of reporting a point on a triangles surface. It's not necessarily obvious how u and v are applied.

[makc]

that's what I am saying, you could have face index today if instead of

meshSampler.sample( ... );

you write

const faceIndex = meshSampler.sampleFaceIndex();
meshSampler.sampleFace( faceIndex, .... );

if you think u and v are not enough you could return Vector3 with u, v and 1 - u - v, too.

[donmccurdy]

if you think u and v are not enough you could return Vector3 with u, v and 1 - u - v, too.

I think that's what is meant by targetBarycoord above, which would be my preference!

[makc]

not quite sure if this conversation is off-topic, or is this something you want me to add to this PR 🤔 I mean, if @gkjohnson has some better implementation in mind but not the time to make his own PR - just paste it here and I will commit it.

[donmccurdy]

Could you update the screenshot?

npm run make-screenshot webgl_instancing_scatter

I think this could be merged after that is fixed.

[makc]

ok done that. btw @gkjohnson I think after this PR you could do meshSampler.normalAttribute = undefined and get previous behavior again.

[donmccurdy]

@makc thanks!