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

Add distance math node #1333

Merged
merged 14 commits into from
May 23, 2023
Merged

Conversation

Cinifreak
Copy link
Contributor

Adding distance math node to measure the distance between two point in(X, Y)

Adding distance math node to measure the distance between two point in(X, Y)
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 19, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Cinifreak / name: Mostafa Azab (7474a95)

@Cinifreak
Copy link
Contributor Author

Screenshot 2023-04-19 152631

@kwokcb
Copy link
Contributor

kwokcb commented Apr 19, 2023

Hi @Cinifreak,

This appears to be a variant on the existing magnitude node, except you have separated out x and y.
Is it reasonable to use the existing node ? Otherwise consistency-wise you only have a variant for only vec2 but not vec3, and vec4.

Or is there a requirement to work with colors ? But vec can be converted to color.

Just unsure of the intent based on your example images. Thanks.

@Cinifreak
Copy link
Contributor Author

@kwokcb Thank you for your feedback, I know there is a magnitude node and it results a vector length (knowing that the reference point is (0, 0) ), but i think we need a distance node too to measure the length between any two points. the other thing is for some reason magnitude node doesn't work with houdini GL, but the distance node work in both cases (Karma & GL).
This screenshot from the GL:
Screenshot 2023-04-19 154146

@Cinifreak
Copy link
Contributor Author

here is an example with distance node, between centered UV and vec2(0.4, 0.3)
Screenshot 2023-04-19 154825

@kwokcb
Copy link
Contributor

kwokcb commented Apr 19, 2023

Sorry, missed that magnitude doesn't have a 2nd point.

I'm wondering if magnitude could be extended or repurposed to have a second point with a default value of the origin (for compatibility). Adding @jstone-lucasfilm , @dbsmythe as it would be nice to not add in another node which is very close to an existing one. Otherwise it would be nice to make magnitude call distance with the origin as 1 of it's points.

@Cinifreak
Copy link
Contributor Author

Cinifreak commented Apr 19, 2023

Sure, you have a point, but as a user, I used to use length node for getting the vector length, and use distance node if I have two vectors. It's easier for the user to see them separately, specially if I don't know how distance function works.

But I agree with you, one node to do both functions will be great for sure.

@kwokcb
Copy link
Contributor

kwokcb commented Apr 19, 2023

I see your point of length separate from distance and magnitude would be overloaded if retains that name.

I guess another point in favour of separation is that these can map directly to shader intrinsics. The graph approach makes it shading language agnostic but it would be nice to use intrinsics if performance is affected. (magnitude is using the length intrinsic.)

BTW, Sorry if I'm sounding nit-picky on this. There's just a lot of nodes already in the library :).

@Cinifreak
Copy link
Contributor Author

my point is to have Magnitude(and naming it length) and distance. or to have one node called distance only. and about you being nit-picky, sure it's good for the library to be organized and matches the user requirements. so, It's great to have your feedback always.

@crydalch
Copy link
Contributor

A dedicated distance node makes sense to me. I don't know that we need to rename magnitude, but I think both make sense for users to have at-hand.

@jstone-lucasfilm
Copy link
Member

@crydalch Agreed, I'd support the idea of having both distance and magnitude nodes in MaterialX, since each has its own natural input signature (derived from the usual meaning of the concept it represents).

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks very promising, @Cinifreak, and I have two suggestions for improvements:

  1. I'd recommend defining the distance node with two vector2 inputs named in1 and in2, which will align with other nodes in the math library, and should extend naturally to vector3 and vector4 distance nodes in the future.
  2. In the implementation of the distance node, I'd recommend using the magnitude node to perform most of the work, so that the definition becomes a simple composition of subtract and magnitude.

As change request I have changed the inputs from 4 float to 2 vec2 and creating a node graph including Magnitude.
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

@Cinifreak This proposal is coming along well, and here are a few additional recommendations that I would make:

  1. Rename the new node from Distance to distance for consistency with other standard nodes in MaterialX.
  2. Rename the two inputs to in1 and in2 for consistency with other math nodes.
  3. Possibly add vector3 and vector4 variants of this node as well? I'm open to ideas on this, but it would seem natural for an artist to expect distance to work on any two vectors of the same type.

Cinifreak and others added 11 commits May 14, 2023 11:19
Changing node name from "Distance" to "distance", inputs from "input" to "input".
Adding vector3 and vector4 to the node.
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @Cinifreak!

@jstone-lucasfilm jstone-lucasfilm changed the title Adding distance math node Add distance math node May 23, 2023
@jstone-lucasfilm jstone-lucasfilm merged commit a52caed into AcademySoftwareFoundation:main May 23, 2023
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
Adding distance math node to measure the distance between two points.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants