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 4 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

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 :(

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.

None yet

4 participants