[ShaderGraph] [2022.1] [Ruby] Fixing Transform Node Issues

[cdxntchou]

Purpose of this PR

Fix for https://fogbugz.unity3d.com/f/cases/1368082/

Errors (and tests of fixes) recorded in this doc:
https://docs.google.com/spreadsheets/d/1P7Bc-2sSsrH40x_kEZDIRC9lQHQ-z2qBeCqxB-g4Rmg/edit#gid=0

Docs jira ticket: https://jira.unity3d.com/browse/GSG-602

This PR also adds user control over the normalization behavior of the direction transforms,
via an added "Normalize" control on the transform node.

When doing a position transform, the normalize control is not shown:

The changes are implemented as an upgrade on the node. Existing Transform nodes will now show up as "deprecated", with an update button to allow a manual user upgrade to the new behavior:

All newly created Transform nodes use the new behavior.

This PR also adds the missing "Normal" conversion type:

We expanded on the PropertyDrawer interface to allow teardown callbacks. This enables us to only show the normalize control when the user has selected a "Direction" transform, by registering with the node to be notified when it changes (and de-registering on teardown).

We split out the code that builds transforms between spaces into it's own helper utility, out of the Transform node. The code is also restructured to make it simpler and easier to build. This allows us to re-use the code to do arbitrary space transforms from any node. Some care was taken to ensure we have identical results from the new code when using version 0 and 1, and the "correct" results when using version 2.

// ignore
Fix for https://fogbugz.unity3d.com/f/cases/1346477/
Triplanar node is hardcoded to world space, is very difficult to adapt to work in object space.

With this change we introduce controls for the user to choose the input and output normal spaces.
Input space should be set to the space used by the input normal vector.
Output space should be set to the desired space for the output (post-triplanar-blend) normal.
// ignore

---

Testing status

Tested modifications to transform node by comparing new results to old results, for all possible transforms. Results verified in scene, in master preview, and on node previews:

• V1, position
• V1, direction
• V0, position
• V0, direction

Tested new V2 behavior:

• All Transforms are invertible: (A->B) then (B->A) produces the original value: Scene view
• All Transforms are invertible: (A->B) then (B->A) produces the original value: Master preview
• All Transforms are invertible: (A->B) then (B->A) produces the original value: Node Preview (this fails for tangent space transforms, it seems to be an issue with preview target)
• All Normalized Direction transforms return a value with length 1.0.

Added Unit Tests for all of the above V2 behaviors, and V1 backwards compatibility.
A->B->C test:

Inverse test:

Legacy V1 behavior test:

Normalization test:

Yamato:

ShaderGraph PR: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1346477/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/9151905/job/pipeline

master: 4e40e2a
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fshadergraph-osx-metal.yml%2523ShaderGraph_OSX_Metal_playmode_mono_Linear_trunk/9110395/job

---

Comments to reviewers

@mmikk -- revew SpaceTransforms.hsll

[github-actions]

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_trunk
With changes to HDRP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

Shader Graph
/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk
Depending on your PR, you may also want
/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_trunk
/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_trunk

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

[Nightmask3]

Did some local testing to verify no regressions:

• Created blackboard items of all property and keyword types and viewed them in Inspector to ensure property drawers all work as expected
• Tested undo/redo with changing property defaults/exposed flag
• Tested interacting with the graph settings and undo/redo of those options

Did find an unrelated bug with the inspector for when a property/keyword is selected and a graph setting change is undo/redo-ed, the inspector switches back to the node settings tab, will investigate and fix that in a separate PR

[jessebarker]

Love these tests.

[jessebarker]

Is the commented out code just for reference? Or something we might need for anticipated test framework chnages?

[cdxntchou]

Oh, I can delete this. Turns out there's a single function that can create an entire path all at once at the system level.. Don't really need this function. :)

[jessebarker]

LGTM

[joshua-davis]

Will this have numerical issues across different gpus by comparing exact pixel values instead of using an epsilon?

[cdxntchou]

It's 8 bit values, so you get an espilon of at least ~(0.5/255) for free. The idea is that the shader is set up to return exact (1,0,0) i.e. (255,0,0) for correct results, which should be pretty darn stable across GPUs.

[cdxntchou]

(would not be true if you are comparing values in the middle of the range, like looking for (128,0,0) -- that would be subject to potential round off error unless you are very careful to center your values so they round correctly.)

[mmikk]

It's so awesome to see this getting cleaned up! Thank you Chris.

I've focused on the functions related to tangent space mostly and what you did looks perfect.

1. In TransformWorldToTangent() and in TransformTangentToWorldDir() in the case where
   doNormalize is false we'd need to divide result by abs(determinant) since the inverse
   matrix would have a scale by 1/determinant but we already have the sign of it applied as sgn.
   So I'd just do a:

else
return result / abs(determinant);

or alternatively skip sgn in the line with the transform and then only use sgn when doNormalize is true
and use 1 / determinant when it's false but one or the other. The point being we don't need the divide if we are normalizing anyway. Then all we need is the sign.

2. I'm wondering if we should add a comment about why we are using the inverse transposed to transform a vector/direction
   rather than to transform a normal which ofc. conceptually is a$$ backwards but the reason is the transformation
   we use to transform a normal is a defined standard (mikkts) used in baking software to store normals and is defined
   to be the quickest way possible to whip up the transformation matrix from the vertex data to minimize overhead at runtime.

So what's technically happening is if M is the mikkts matrix used to transform a normal (interpolated vertex tangent frame with various requirements) then conceptually we can think of M = (~A)^T. In other words we can think of the matrix M we are required to use as the transposed
inverse of some other matrix A. Then naturally A is the matrix we'd use to transform a vector/direction. And since
M = (~A)^T it follows that A = ~(M^T) = (~M)^T. So since M is the matrix we MUST use to transform a normal (for compliance reasons) it follows inverse transpose becomes
what we must use on a direction even though it's kind of a$$ backwards. I hope there's a way to turn some of this rambling into a comment :)

Anyway doing so will allow vectors to maintain their usual relationships to the normal in the new space (ie. preserving their dot product with the normal).

[joshua-davis]

All main issues have been fixed or clarified. Looking good!

[mmikk]

lgtm!