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

Initial version of ramp implementation for discussion. #1884

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

feldstj
Copy link
Contributor

@feldstj feldstj commented Jun 11, 2024

This change includes the ramp nodedef, ramp gradient nodedef and subsequent nodegraph implementations for these two nodedefs. The ramp node includes support for both linear, smoothstep and step interpolation. The ramp also supports up to 30 control points and the following ramp types: standard, radial, circular and box.

Feel free to reach out with any questions. Looking forward to the discussion!

Copy link

linux-foundation-easycla bot commented Jun 11, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@rafalSFX
Copy link
Contributor

Looks good! Great improvement over ramplr and ramptb shaders.

It supports a constant and linear interpolation, and I wonder what it would take to also support a smooth interpolation (eg, bezier).

Also, in one of the MtlX meetings we talked about a user friendly UI that could tie all the interval and color inputs of the shader into a single gradient-looking color bar. I think there was a mention of a new UI hint in MaterialX. How would that be expressed (ie, what would that hint be)?

@feldstj
Copy link
Contributor Author

feldstj commented Jun 13, 2024

Looks good! Great improvement over ramplr and ramptb shaders.

It supports a constant and linear interpolation, and I wonder what it would take to also support a smooth interpolation (eg, bezier).

Also, in one of the MtlX meetings we talked about a user friendly UI that could tie all the interval and color inputs of the shader into a single gradient-looking color bar. I think there was a mention of a new UI hint in MaterialX. How would that be expressed (ie, what would that hint be)?

Actually it currently supports linear and smoothstep interpolation. Doesn't support constant yet, although that would be very nice to add!

@ld-kerley
Copy link
Contributor

This is looking good to me too - I'm looking forward to a MaterialX world where we have more fully featured ramps :)

I do have one meta-observation/thought. Aside from debating the number of inputs, I think the node definition interface for ND_ramp looks pretty good and I don't think is controversial at all. (though I do think there may also be a place for a sister ramp node in the future that just takes in a varying array - as we discussed in the TSC meeting). But, ND_ramp_gradient seems very specific to the implementation of ND_ramp, and I wonder if it is really useful as a standard library node itself? It's almost as if we need a way to tag node definitions as "private" - or perhaps there is some other mechanism (node graphs?) that could enshrine the logic internal to the implementation of the node.

Also, as raised in the TSC I wonder if we will at some point have a need for a higher level of continuity than can be offered by piece-wise inspection of the ramp entries, which would mean this approach of chaining these "private" nodes together might need to evolve in the future.

@feldstj
Copy link
Contributor Author

feldstj commented Jun 13, 2024

This is looking good to me too - I'm looking forward to a MaterialX world where we have more fully featured ramps :)

I do have one meta-observation/thought. Aside from debating the number of inputs, I think the node definition interface for ND_ramp looks pretty good and I don't think is controversial at all. (though I do think there may also be a place for a sister ramp node in the future that just takes in a varying array - as we discussed in the TSC meeting). But, ND_ramp_gradient seems very specific to the implementation of ND_ramp, and I wonder if it is really useful as a standard library node itself? It's almost as if we need a way to tag node definitions as "private" - or perhaps there is some other mechanism (node graphs?) that could enshrine the logic internal to the implementation of the node.

Also, as raised in the TSC I wonder if we will at some point have a need for a higher level of continuity than can be offered by piece-wise inspection of the ramp entries, which would mean this approach of chaining these "private" nodes together might need to evolve in the future.

Thanks for the feedback! In the original version of the ramp I wrote up, I didn't have the gradient node. When we decided to expand the number of control points this seemed way easier by introducing this node. I 100% love your idea of being able to somehow have private nodes that can be used in circumstances like this. Perhaps in editors for example this way they could be easily flagged as nodes you shouldn't be able to create. I think the only reason someone would want access to this node would be if they wanted to implement their own ramp that supported a different number of control points.

@ld-kerley
Copy link
Contributor

Yeah I totally see the benefit of having the node available to allow for varying the number of inputs really easily, or potentially allowing others to create their own ramp implementations with different number of inputs.

I'm not sure the "private" node idea implementation can just be left to the UI though, otherwise we run the risk of different DCCs respecting it or not. I do wonder if its currently possible (or we could make changes to support this) that we could just define a nodegraph in stdlib_ng.mtlx for the ramp gradient, and then just use instances of this nodegraph in the implementation of the ramp node. This would let us leverage the re-use, without committing the project to maintain the ramp gradient node as part of the public specification.

We should also remember to not left perfect be the enemy of good here! Making a better ramp is going really benefit the whole MaterialX community

@feldstj
Copy link
Contributor Author

feldstj commented Jun 13, 2024

Your instancing nodegraph idea is interesting, just don't know if this is on anyone's radar. I just want to avoid having 30 copies of the nodegraph in the definition (thus the nodedef for now).

@crydalch
Copy link
Contributor

crydalch commented Jul 5, 2024

This is coming along nicely @feldstj, I love the new interpolation!

Here are a few thoughts:

I think just informing in the docs that ramp_gradient is a building-block for ramp, and can probably be hidden from average artist workflows, is plenty?

Also this node only has a color4 signature, but most of the color nodes tend to default to color3? Should color3 be available, along with float?

This node might also have some expectations documented around the UI. Because different application often handle ramp controls and node networks differently, it might be good to have some hints for the implementors. For example, whether or not all of these inputs must to be shown if they aren't in use by the ramp widget? Or whether or not intervals must be evenly spaced?

I know it may seem obvious, and maybe the "up to" mention in the docs is fine, but I think it'd be good to remove as much ambiguity as possible.

Especially because transporting materials in .mtlx or .usd between DCCs is/will be increasingly common, so having enough signatures and/or conventions/expectations to edit in various places seems important. For example, should a DCC reading a .mtlx/.usd network always interpret ND_ramp-->ND_convert_vector4_float as a float ramp, even if a user creates those nodes manually?

Definitely agree with @ld-kerley that we don't want perfect the enemy of good enough, but when we created a ramp node similar to this, the UI/interchange aspects were what tripped everyone up.

@feldstj
Copy link
Contributor Author

feldstj commented Jul 5, 2024

Thanks for the feedback @crydalch, appreciate it!

Adding docs to the ramp_gradient node that suggest that it is simply a building block for the ramp node certainly makes sense and seems easy enough to do. Did you want this in the MaterialX document doc string, or just in the MaterialX specification documentation?

Your point about supporting color3 and float versions of the ramp node makes sense as well. My only concern is that it may take some time to implement as creating the initial version did take some time and unfortunately this isn't the only project I'm working on at the moment.

With respect to the ramp docs that try to clarify what ramp implementors need to do, I'm mixed on this topic. On the one hand, I think it's true that ports like num_intervals don't need to be exposed to users of a ramp they are more needed internally to the ramp implementor. That said, I'm worried that most people care about ramp docs for usage information, not so much about implementation information. To me this feels like it should live elsewhere although I'm not quite sure where.

@crydalch
Copy link
Contributor

crydalch commented Jul 5, 2024

Adding docs to the ramp_gradient node that suggest that it is simply a building block for the ramp node certainly makes sense and seems easy enough to do. Did you want this in the MaterialX document doc string, or just in the MaterialX specification documentation?

Yeah I think the spec doc makes sense? Might be a good point to ask @jstone-lucasfilm about this sort of detail. Should it go in the spec, or just as a note in the node's documentation?

Your point about supporting color3 and float versions of the ramp node makes sense as well. My only concern is that it may take some time to implement as creating the initial version did take some time and unfortunately this isn't the only project I'm working on at the moment.

I think this should be easy/straightforward conceptually: node signatures can actually use an existing signature in their nodegraph. I remember hand-editing to do this, but that might have been before the MtlX Node Editor was working? This approach means the color3/float nodegraphs can actually use the color4 signature, and convert/extract as expected; so that should simplify the implementation of other signatures? I have been trying to find an example, I'm positive I've made one but I haven't found it so far :(

@jstone-lucasfilm
Copy link
Member

I really like this feature proposal, @feldstj, and my one thought is that we may want to limit this initial implementation to 10 control points, keeping the door open to an array-based node with an unlimited number of control points in the future.

That would then give artists the best of both worlds, where the fixed graph logic of this node handles the vast majority of cases, and a future array-based node handles less common cases requiring scores or hundreds of control points.

What are your thoughts?

@feldstj
Copy link
Contributor Author

feldstj commented Aug 2, 2024

Hi @jstone-lucasfilm, I'm torn on this. On the one hand, 10 control points is simpler and a nice round number. On the other hand Nikola had me increase the original 10 count to 30 because he thought it was important to provide a larger number of control points to users. Yes, the array-based approach will provide a larger number of control points, but the problem is still that those control points can't be connected to such as with textures as is possible with this version. I'm certainly open to it, but we should make sure we're ok with the consequences.

@jstone-lucasfilm
Copy link
Member

These are good points, @feldstj, though I'll offer two counterpoints that may lend support to the idea of starting with 10 control points:

  • Starting with 10 control points in an initial release of ramp, it's always possible to increase the number in future releases, providing full backwards compatibility for older documents. The reverse approach would not be possible if we commit to 30 control points in the initial release.
  • Artists can use your ramp_gradient helper node to construct custom ramp nodes supporting any number of control points that they wish, and they can bundle these with their assets to make the functionality universally accessible.

@ld-kerley
Copy link
Contributor

I'm also with @jstone-lucasfilm with starting out at 10 for all his great reasons, but also because I'm a little concerned about the generated code size as well. The 30 input version is likely to be one of the larger nodegraph based nodes we have in MaterialX, and in my previous production experience, once an artist is using a ramp for something procedural, its not uncommon to find them using many ramps combined together in "interesting" ways, and so that large network size could potentially multiply rapidly in heavy production cases.

Also thinking forward to a future world where we might have ramp nodes that have fixed inputs that allow connections like these, but also varying input ramps (potentially not allowing input connections), I wonder a little about node definition naming. Maybe we want to call this something like ND_ramp_fixed, or perhaps even ND_ramp_fixed_10 allowing for performance critical applications to opt between a 10 knot ramp and a potential 30 knot ramp in the future too. That would allow for the varying ramp to be named something like ND_ramp_varying in the future.

@jstone-lucasfilm
Copy link
Member

@ld-kerley I'm very open to recommendations for the name of this new node, and we might look to existing implementations of this concept in industry tools for guidance.

For the differentiation between fixed and varying versions of this node, we may not need to include fixed and varying in the node name itself, as long as the interfaces of the different versions are clear and distinct.

For example, this version of ramp takes a series of colorN and intervalN inputs, while a varying ramp would likely have single colorarray and intervalarray inputs, in which case the inputs to a ramp node would be sufficient to determine which variant was intended (excepting the trivial case of a ramp node with no inputs, in which case our precedence rules would take over).

@JamesPedFoundry
Copy link

JamesPedFoundry commented Aug 6, 2024

Hey All,

I'm going to prefix this with the fact that I'm relatively new to MaterialX, and be making some assumptions (which I'll state).

My first assumption: There is a current limitation (and one I can see as being "impossible" due to the differing supported shading languages) that we can't have a node graph that can have an array of connections, with an array of indices indicating which knot they relate to?

A question with the current proposal, is what would happen if I had the values (red, green) connected to the first(color1) and last input(color30), with intervals of 0 and 0.5, would the 2nd input be ignored because the 2nd input has an interval of 1.0 (therefore greater than color30). Such that I'd end up with a ramp from red -> white from 0 -> 1.0 intervals? This could make UI tricky to understand is my current thoughts, although I'm happy to have intrepreted the graph incorrectly - again not the best at reading MaterialX yet!

The other note, is that typically there's two ramps I've seen, float and Color, again multiple Color (4 or 3 values, float or double), I'd hope that we'd have differently named MatX nodegraphs for each. I like the above idea of naming them along the lines of ND_ramp_color3f_fixed30, ND_ramp_color3d_fixed30 ,ND_ramp_color4f_fixed10, ND_ramp_float_fixed10, etc... This way we can easily add change how ramps are handled in a non-destructive manner. Essentially there is no one "ramp shader", there are many different typed ramp shaders and we should name them as such.

To relate this back to something I have planned for a USD proposal around Ramps in shaders was to describe them as such:

float[] inputs:<someValue>:knot:position (or interval if you prefer) 
token[] inputs:<someValue>:knot:interpolations (per knot interpolation values) - could also be indexed? linear if not specified or clamp settings across future knots, unsure.
rel[] <someValue>:knot:connections // a sparse connection list of connected attributes
int[] <someValue>:knot:connections:indices // indicies to match the connections to a particular index in the positions/intervals
color3f[] (or other array type) inputs:<someValue>:knot:values // default values for the connected values would be stored here as well, such that if the relationship fails, we have a value to use. 

A bit of a brain dump, but trying to make sure we can also align as to how this information will be stored in the future to avoid having to revisit this again.

Cheers,
James

@jstone-lucasfilm
Copy link
Member

@JamesPedFoundry For the array-based version of the ramp node, this should eventually be possible in MaterialX using inputs of type color3array and floatarray, though we'll need a bit more work in our shader generation system to support this across languages. We're thinking of this as a future, complementary partner to the fixed-count ramp node that @feldstj has proposed for the current version of MaterialX.

@JamesPedFoundry
Copy link

JamesPedFoundry commented Aug 9, 2024

@jstone-lucasfilm thanks for confirmation around future array inputs, makes a lot of sense to me.
I guess then my only change request would be the name of this node, making it specifically named. Such that, other ramp types are available, and it is clear which should be used when. Let's not limit ourselves for the future case where the array based nodes might exist.

@jstone-lucasfilm
Copy link
Member

@feldstj @ld-kerley

Now that MaterialX 1.39.1 is released, I wanted to follow up on this proposal, to see what additional steps we will need before this can be merged.

If it meets the approval of our Autodesk colleagues, I'd recommend limiting this node to 10 control points, motivated by the discussions above.

Additionally, a few folks have suggested changing the name of this node, though I'll node that ramp could have both graph-based and array-based versions, as long as the inputs to these two nodes have distinct and clear names. For example, the graph-based version could used inputs named color1, interval1, color2, interval2, and so forth, while the array-based version could use inputs named colorarray and intervalarray.

Because these interfaces have no overlap, it would always remain clear which version of ramp is intended by an artist, so we would not strictly need two different node names.

@feldstj
Copy link
Contributor Author

feldstj commented Sep 7, 2024

@jstone-lucasfilm

I'll try to follow up with some Autodesk colleagues about the 10 control points. I think the other thing that was requested was to have float and color3 versions of the ramp. So something like ND_ramp_float, ND_ramp_color3 and ND_ramp_color4.

@feldstj
Copy link
Contributor Author

feldstj commented Sep 11, 2024

@jstone-lucasfilm
Spoke with Nikola and Jerry. Nikola still seems to really want 30 control points. Jerry mentioned you may have discussed this at the ASWF meeting this week. Any conclusions?

@ld-kerley
Copy link
Contributor

I still think that we might be happy in the long run If we were to name the fixed size ramp and the variable ramp case differently. We do still have some outstanding issues regarding ambiguous node resolution, if the inputs are under specified, which does manifest as concrete difficulties if the MaterialX document is converted to OpenUSD.

I'm not sure that there is a strong disadvantage to having separate node categories for the different ramp types. One other consideration might be that the different ramp types may possibly end up providing different implementation behavior?

Finally, if this all turns out to be a mistake, it would be a lot easier to write an upgrade that renamed a number of different ramp node names, to a single name, than to reverse this decision in the opposite direction.

@JamesPedFoundry
Copy link

JamesPedFoundry commented Sep 12, 2024

To confirm, my concerns on the naming of just a "ramp" is less for between array or interval based ramps, but more the difference between a color3, color4 and float types of ramp shader. With none of these being the defacto "ramp", it'd make sense to name them all separately.

Also how this would then fit into something like USD where it wouldn't necessarily support arbitrarily having its inputs be either a color3, color4 or float, it accepts one or the other in the schema definition.

I also echo Lee's comment, it is far easier to go from many names to one, than from one to many names.

@jstone-lucasfilm
Copy link
Member

@JamesPedFoundry Just as a point of comparison, I'll note that MaterialX traditionally uses a single name for all type variations of a single node category, including ramp-related nodes such as remap, range, and smoothstep:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/documents/Specification/MaterialX.Specification.md#adjustment-nodes

@ld-kerley If we were to propose a naming distinction between fixed-length and variable-length ramp nodes, what naming convention would you suggest?

@ld-kerley
Copy link
Contributor

I don't have super strong opinions on what the names should be, but perhaps...

  • <ramp_fixed>
  • <ramp_varying> or <ramp_array>

I also wonder if for the fixed version we end up having multiple nodes with differing number of inputs if we might want

  • <ramp_fixed_10>
  • <ramp_fixed_30>
  • etc....

This leaves the future door open for a clean move to a unified <ramp> in the future, without favoring either flavor.

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.

6 participants