-
Notifications
You must be signed in to change notification settings - Fork 352
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
base: main
Are you sure you want to change the base?
Initial version of ramp implementation for discussion. #1884
Conversation
Looks good! Great improvement over 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! |
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 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. |
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 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 |
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). |
This is coming along nicely @feldstj, I love the new interpolation! Here are a few thoughts: I think just informing in the docs that Also this node only has a 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 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. |
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. |
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?
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 |
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? |
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. |
These are good points, @feldstj, though I'll offer two counterpoints that may lend support to the idea of starting with 10 control points:
|
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 |
@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 For example, this version of |
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 To relate this back to something I have planned for a USD proposal around Ramps in shaders was to describe them as such:
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, |
@JamesPedFoundry For the array-based version of the |
@jstone-lucasfilm thanks for confirmation around future array inputs, makes a lot of sense to me. |
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 Because these interfaces have no overlap, it would always remain clear which version of |
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. |
@jstone-lucasfilm |
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. |
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. |
@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 @ld-kerley If we were to propose a naming distinction between fixed-length and variable-length ramp nodes, what naming convention would you suggest? |
I don't have super strong opinions on what the names should be, but perhaps...
I also wonder if for the fixed version we end up having multiple nodes with differing number of inputs if we might want
This leaves the future door open for a clean move to a unified |
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!