Skip to content

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.

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
sunag merged 1 commit into from
Jul 6, 2025
Merged

Normal.js: Improve JSDoc #31341

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 11 additions & 11 deletions src/nodes/accessors/Normal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ import { positionView } from './Position.js';
import { directionToFaceDirection } from '../display/FrontFacingNode.js';

/**
* TSL object that represents the normal attribute of the current rendered object.
* TSL object that represents the normal attribute of the current rendered object in local space.
*
* @tsl
* @type {Node<vec3>}
*/
export const normalGeometry = /*@__PURE__*/ attribute( 'normal', 'vec3' );

/**
* TSL object that represents the vertex normal in local space of the current rendered object.
* TSL object that represents the vertex normal of the current rendered object in local space.
*
* @tsl
* @type {Node<vec3>}
Expand All @@ -34,15 +34,15 @@ export const normalLocal = /*@__PURE__*/ ( Fn( ( builder ) => {
}, 'vec3' ).once() )().toVar( 'normalLocal' );

/**
* TSL object that represents the flat vertex normal in view space of the current rendered object.
* TSL object that represents the flat vertex normal of the current rendered object in view space.
*
* @tsl
* @type {Node<vec3>}
*/
export const normalFlat = /*@__PURE__*/ positionView.dFdx().cross( positionView.dFdy() ).normalize().toVar( 'normalFlat' );

/**
* TSL object that represents the 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.
*
* @tsl
* @type {Node<vec3>}
Expand All @@ -66,7 +66,7 @@ export const normalViewGeometry = /*@__PURE__*/ ( Fn( ( builder ) => {
}, 'vec3' ).once() )().toVar( 'normalViewGeometry' );

/**
* TSL object that represents the vertex normal in world space of the current rendered object.
* TSL object that represents the vertex normal of the current rendered object in world space.
*
* @tsl
* @type {Node<vec3>}
Expand All @@ -86,7 +86,7 @@ export const normalWorldGeometry = /*@__PURE__*/ ( Fn( ( builder ) => {
}, 'vec3' ).once() )();

/**
* 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.
Copy link
Collaborator Author

@WestLangley WestLangley Jul 5, 2025

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?

Copy link
Collaborator

@sunag sunag Jul 5, 2025 •
edited
Loading

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.

Copy link
Collaborator Author

@WestLangley WestLangley Jul 5, 2025 •
edited
Loading

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.

Copy link
Collaborator

@sunag sunag Jul 5, 2025

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.

Copy link
Collaborator Author

@WestLangley WestLangley Jul 5, 2025 •
edited
Loading

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."

Copy link
Collaborator

@sunag sunag Jul 6, 2025

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.

Copy link
Collaborator Author

@WestLangley WestLangley Jul 6, 2025

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. 🙏

*
* @tsl
* @type {Node<vec3>}
Expand Down Expand Up @@ -118,15 +118,15 @@ export const normalView = /*@__PURE__*/ ( Fn( ( { subBuildFn, material, context
}, 'vec3' ).once( [ 'NORMAL', 'VERTEX' ] ) )().toVar( 'normalView' );

/**
* TSL object that represents the transformed vertex normal in world space of the current rendered object.
* TSL object that represents the vertex normal of the current rendered object in world space.
*
* @tsl
* @type {Node<vec3>}
*/
export const normalWorld = /*@__PURE__*/ normalView.transformDirection( cameraViewMatrix ).toVar( 'normalWorld' );

/**
* TSL object that represents the transformed clearcoat vertex normal in view space of the current rendered object.
* TSL object that represents the clearcoat vertex normal of the current rendered object in view space.
*
* @tsl
* @type {Node<vec3>}
Expand Down Expand Up @@ -200,7 +200,7 @@ export const transformNormalToView = /*@__PURE__*/ Fn( ( [ normal ], builder ) =
// Deprecated

/**
* TSL object that represents the transformed vertex normal in view space of the current rendered object.
* TSL object that represents the transformed vertex normal of the current rendered object in view space.
*
* @tsl
* @type {Node<vec3>}
Expand All @@ -214,7 +214,7 @@ export const transformedNormalView = ( Fn( () => { // @deprecated, r177
} ).once( [ 'NORMAL', 'VERTEX' ] ) )();

/**
* TSL object that represents the transformed vertex normal in world space of the current rendered object.
* TSL object that represents the transformed vertex normal of the current rendered object in world space.
*
* @tsl
* @type {Node<vec3>}
Expand All @@ -228,7 +228,7 @@ export const transformedNormalWorld = ( Fn( () => { // @deprecated, r177
} ).once( [ 'NORMAL', 'VERTEX' ] ) )();

/**
* TSL object that represents the transformed clearcoat vertex normal in view space of the current rendered object.
* TSL object that represents the transformed clearcoat vertex normal of the current rendered object in view space.
*
* @tsl
* @type {Node<vec3>}
Expand Down