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

Adds a Color Correct node #1086

Merged

Conversation

crydalch
Copy link
Contributor

@crydalch crydalch commented Sep 23, 2022

Wraps up existing adjustment nodes into an artist-friendly color correction node.

Here are some example results from the Houdini docs, using Karma:

hmtlxcolorcorrect_hue
hmtlxcolorcorrect_exposure

Wraps up existing adjustment nodes into an artist-friendly color correction node.
@jstone-lucasfilm
Copy link
Member

Thanks for this great proposal, @crydalch!

My one question is about the addition of individual channel outputs for "r", "g", and "b", since we've been imagining that this need would be covered by the "channel" attribute in MaterialX 1.39 (originally called "channels" in 1.38).

Here is the current language for the "channel" attribute in 1.39:

A node input must generally be connected to outputs of the same type, but float inputs may also be connected to any single channel within a multi-channel data types by adding an integer "channel" attribute, indicating the channel number (0-3) to extract from the input:

    <constant name="c3" type="color3">
      <input name="value" type="color3" value="0.1, 0.2, 0.3"/>
    </constant>
    <constant name="v4" type="vector4">
      <input name="value" type="vector4" value="0.1, 0.2, 0.3, 0.4"/>
    </constant>
    <add name="mult1" type="float">
      <input name="in1" type="float" nodename="c3" channel="0"/>
      <input name="in2" type="float" nodename="v4" channel="3"/>
    </add>

The "channel" attribute is valid in any non-token element that allows a "nodename" attribute.

The motivation behind this feature is to enable graph editor packages to connect individual channels of outputs to scalar inputs, without every MaterialX node interface needing to handle this functionality separately. Do you believe this would be sufficient for the colorcorrect node as well?

@crydalch
Copy link
Contributor Author

Thanks @jstone-lucasfilm! I think the biggest concern is the channels approach is not supported by UsdShade. I'm also not sure it's necessarily saving the artist from making more nodes or setting more controls; it's just shifting the work since you'd still have to pick the channel after making the connection. (or insert a separate node to separate). Having explicit, additional scalar outputs simplifies the operation to just connecting an output to another node, and seems like less work to me. But I could be missing something. Or are maybe there artist-facing tools that make this vector-->float(channel) connection as simple as connecting two nodes?

Having said all of that, we can just remove the float outputs if it's a point of concern; having the node sooner is probably better for perceived artist-readiness. And the additional float outputs could be added later too of course, even though MtlX's convention/preference seems to heavily favor no multi-output nodes (artistic_ior being an exception).

We should also probably update the node to add a color4 signature, which would then call for adding a 4th scalar output to account for alpha (if those stuck around).

@jstone-lucasfilm
Copy link
Member

@crydalch Yes, the thinking was that artist-facing tools would present the "channel" feature as simple connections of individual channels of outputs to scalar inputs. From this perspective, the UI would be the same as if every MaterialX node had individual "r", "g", "b" outputs defined, but without the overhead of providing these individual named outputs in the specification and data libraries.

Let me know how this more general approach sounds to you, and whether it might make sense to work with the USD team to support this feature directly. This would remove the need to amend all of our high-level nodes with multiple named outputs, and would instead provide this commonly-needed functionality in a universal way.

@crydalch
Copy link
Contributor Author

crydalch commented Sep 26, 2022

@jstone-lucasfilm We can remove the float outputs, even if there isn't a way in USD to use the MtlX channels output. I wasn't trying to propose/suggest every color/vector-output node get additional floats outputs, just raising cases where it sometimes makes sense, and why channels don't (based on my understanding) save time/clicks for specific cases; but we can discuss more in tomorrow's mtg (just saw this added to the agenda). Thanks!

@dbsmythe
Copy link
Contributor

My 2c on this: I agree we do not want to have explicit single-channel outputs in every MaterialX node, and that whenever a "channels" attribute is used, each Application would handle this in whatever way was appropriate for it, perhaps by inserting an node. In fact the node in v1.39 uses an integer channel number to indicate which channel to extract, exactly to support this kind of operation. And we wanted to switch from using string channel names like "r" or "x" and toward integer channel indexes as the latter are much easier to deal with in shading languages.

@kwokcb
Copy link
Contributor

kwokcb commented Sep 27, 2022

Some addition notes to this:

  • I believe the assumed premise is separation of what is required for runtime workflows and what you need for storage / interop -- the definition is for the latter.
  • At runtime I would also vote for a direct port->port connection but for storage: extraction on the input port is the current approach.
  • Believe the same case can be made to have 1,2,3,4 channel and vec vs color variants for definition storage but at runtime this is unwieldy so would not be presented that way.

however

  • Extraction on the output is partially supported and is the trickier one handle -- so here I'd add a vote for explicit extraction when it's for a common enough workflow.
  • This does however require some conversion process to make implicit channel extraction explicit.
  • BTW @dbsmythe , I had put up the rgb/a extract node which is a 1.39 suggestion you added but put this on hold after talking with @jstone-lucasfilm . I think this is another use case to consider. This or other variants for common enough cases I think could remove the desire to add extraction on every node with a vec/color output.

crydalch and others added 2 commits September 27, 2022 10:31
The separated scalar outputs for RGB have been removed, both because of conversations around "channels" and to simplify adding color4 support.
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 is looking great, and I've written up just one note to resolve.

libraries/stdlib/stdlib_defs.mtlx Outdated Show resolved Hide resolved
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 @crydalch!

@jstone-lucasfilm jstone-lucasfilm merged commit 38669ab into AcademySoftwareFoundation:main Oct 14, 2022
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
Wraps up existing adjustment nodes into an artist-friendly color correction node.
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