Skip to content
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

VisualShader: Add a CoordinateSpaceHelper node and add proper mat4 * vec4 operation to TransformVecMult node #97215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tetrapod00
Copy link
Contributor

@tetrapod00 tetrapod00 commented Sep 20, 2024

Partially implements godotengine/godot-proposals#7666.
Resolves godotengine/godot-proposals#8376.

Problem summary:
The current visual shader node TransformVecMult is trying to serve two purposes: a way to transform vectors between coordinate spaces, and also a general vec4 * mat4 node. It's failing at both purposes:

  • It has correct but obscure dropdown names like A x B (3 x 3), making it hard to understand if you don't already know matrix math.
  • It can't actually do general-purpose vec4 * mat4 multiplication correctly, since it accepts a vec3 and autofills the w component of a vec4 with either 1.0 or 0.0.
  • It doesn't output a vec4, so you can't use the w component of the output. (Useful for some clip space operations).
  • It's not beginner or artist friendly. Other engines have dedicated coordinate space transform nodes, but godot requires you to know at least some matrix math.

Solution:
Add a new, dedicated node to transform positions and directions between model, world, view, and clip space.

  • Accepts and returns vec3. This node is only for transforming positions and directions. If you need to use the w component of the output, use the old node.
  • Uses artist-friendly names of Position and Direction for the vector types.
  • Automatically fills in the matrix needed internally. But, as an artist-friendly node, you don't need to know that.

Adjust the old TransformVecMult node. It now:

  • Always outputs a vec4. If you only want a vec3, you can plug the vec4 into a vec3 input or swizzle the vec4.
  • Keeps its four old modes, A x B, B x A, A x B (3x3), B x A (3x). These still accept a vec3 input, but they now output the whole vec4. These modes function the same as before, except for outputting w.
  • Adds two new modes (operations), A x B (Vec4) and B x A (Vec4). These accept a vec4 input and return a vec4 output. If you want to use these modes to do the coordinate space transform, you'll need to fill in the correct w on the input.
  • Backwards compatible, except for one edge case. If you are currently using the vec3 output, plugging it into a vec4 input, and assuming that the w component is 0.0, your shader may change.

New node:
From spaces: Model to..., World to..., View to..., Clip to...
To spaces: Model, World, View, Clip
Vector types: Position, Direction
godot windows editor dev x86_64_JBbHJ9swWh

Visual summary of the two old and new node:
godot windows editor dev x86_64_L1OT8vxS5m

View to world snippet, old and new:
godot windows editor dev x86_64_i106PuUFHv

TransformVecMult compatibility

Images of current compatibility

4.3: Shader that uses all four current operater modes, and connects to both vec3 and vec4 slots:

Godot_v4 3-stable_win64_0emenX7GeH
This PR: Same shader, opened with this PR. All connections are still present. The only edge case is that the vec3 output is now connected to a vec4 slot. If your shader assumed that the w component is 0.0, it will break.

godot windows editor dev x86_64_E5X1InPsZf

If 100% compatibility is needed, there are ways to do so. We could:

  1. Output both a vec3 and vec4 for all operator modes of TransformVecMult, with the only difference being the w field. IIRC Unity did this for some of its nodes. It makes the node more cluttered, though.
  2. Output either a vec3 or a vec4, dynamically switched with another enum dropdown. Again, more clutter.
  3. For the old operators, output a vec3. For the new vec4 * mat4 operators, output a vec4. Old operators remain exactly as they are, and the correct vec4 * mat4 operator is now available if you need w or for other uses.
  4. Alternately, we could use a different compatibility strategy. If I was designing the nodes today, without compatibility concerns, I would make two nodes: CoordinateSpaceHelper (as described in this PR) and a TransformVector4Multiply node, which only does vec4 * mat4 or mat4 * vec4 multiplications. The CoordinateSpaceHelper does artist-friendly transforms, and the TransformVector4Multiply node matches text shader behavior (since you can't do vec3 * mat4 or mat4 * vec3 in a text shader).
    So we could add those two nodes, and deprecate the current node. Users of the current TransformVecMult should switch to either CoordinateSpaceHelper for the common use case, or TransformVector4Multiply for more any other use case.

Tasks:

  • Decide on the new node's name. "CoordinateSpaceHelper" is intentionally not the final name.
  • Decide on names for new enums. The names for TransformVecMult's operators should maybe be changed, too.
  • Test old node for compatibility.
  • Test more thoroughly that each coordinate space conversion works in vertex(), fragment(), and light().
  • Write docs.

Copy link
Member

@Chaosus Chaosus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, A minor compatibility breakage is acceptable in that case (I think)

@tetrapod00
Copy link
Contributor Author

tetrapod00 commented Sep 20, 2024

We probably should find a way to actually keep 100% compatibility. Having a single node that both does mat4 * vec4 and mat4 * vec4(vec3, 1.0) is already something of a compromise from the ideal set of nodes (described in option 4 in the OP). So if we're not going with compatibility option 4, we should probably pick option 1, 2, or 3. I think 3 is my favorite.

@tetrapod00 tetrapod00 changed the title VisualShader: Add a CoordinateSpaceHelper node and expand TransformVecMult node VisualShader: Add a CoordinateSpaceHelper node and add proper mat4 * vec4 operation to TransformVecMult node Sep 20, 2024
Working version

Fix copy paste error

Change TransformVecMult to output vec4, generate docs

Some more docs.

Suggestion: remove unnecessary default

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[VisualShader] Add w output to TransformVectorMult, improve dropdown entries
3 participants