-
-
Notifications
You must be signed in to change notification settings - Fork 36k
Normal.js: Improve JSDoc #31341
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
Normal.js: Improve JSDoc #31341
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
What is the proper distinction between // This may need clarification, too -- in a separate PR, that is. https://github.com/mrdoob/three.js/wiki/Three.js-Shading-Language#normal |
The transformed term reflects the modifications applied by processes such as skinning, morphing, and similar techniques. The term transformed also includes following the correct orientation of the face, so that the normals are inverted inside the geometry. |
Trying to clarify...
//
|
1, 2. No for both I imagine.
The developer could still use the function const objectNormalView = directionToFaceDirection( normalViewGeometry ); |
|
||
/** | ||
* TSL object that represents the transformed vertex normal in view space of the current rendered object. | ||
* TSL object that represents the vertex normal of the current rendered object in view space. |
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.
The word "transformed" was removed. Is that OK, or do you want the word restored?
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.
Its ok for me. If we are going to remove the term transformed, I think it would be better to modify the description of nodes with the *Geometry suffix so that it is explicit that they are not modified with skinned or morph.
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.
Yes, that is why I removed the term.
And I mentioned this because with this PR normalView
and normalViewGeometry
now have identical descriptions -- as do normalWorld
and normalWorldGeometry
.
I do not know how to describe the accessors having the *Geometry suffix. With the exception of the three.js roughness calculation, I am not aware of any other use cases in the literature where one would need them.
This is why I asked if you would consider removing them, and using them only internally. That would be my preference.
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.
It is definitely something very specific and I think each engine assigns a name to it, Unreal uses Pre-Skinned Normal for similar purpose and gives some examples using tri-planar texture here. I think it is useful that we have it, as we saw this was used to optimize the code in some examples like webgpu_tsl_earth
, webgpu_reflection
and the background shader, so it is positive.
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.
@sunag OK. Do you like the Pre-Skinned terminology? I can refer to it as the "pre-skinned vertex normal of the currently-rendered object in view space."
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’d like it to be automatic for the user to understand that the Geometry
suffix does not process vertices for skinned, morphed, or custom transformations. Pre-Skinned
seems to only refer to skinned meshes. Maybe we could add something like this?
/**
* TSL object that represents the vertex normal of the currently rendered object in view space.
*
* Nodes with the *Geometry suffix do not process skinned, morphed, or custom vertex transformations.
*
* @tsl
This is present in other places as well as positionGeometry
.
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.
@sunag Unfortunately, this is leading to more questions for me..
Rather than continuing with this back-and-forth, I'd prefer to merge this PR for now, and then wait for you to add clarifications in a follow-up PR. Hopefully, I will then be able to better understand your intentions. 🙏
As the title says.