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 code generation hints support #1954

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

kwokcb
Copy link
Contributor

@kwokcb kwokcb commented Jul 29, 2024

Implementation for Specification of Input "Hints"

Interface changes

Add in support for accessing "hints" on inputs as follows:

  1. Add has/get/get hint methods on Inputs
  2. Add a get to return a list of { input name, hint string } pairs.

Issue Address

Fixes #1946

Test

  1. Add transparency and opacity hints to OpenPBR.
  2. Updates the transparency check logic to include checking for hints on a node's nodedef and adds those items to the input list to check.
  3. Add test case to RTS with a test for transparency and opacity:
<?xml version="1.0"?>
<materialx version="1.39" colorspace="lin_rec709">
  <surfacematerial name="geom_hint" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="geometric_opacity_hint" />
  </surfacematerial>
  <open_pbr_surface name="geometric_opacity_hint" type="surfaceshader">
    <input name="geometry_opacity" type="float" value="0.5" />
  </open_pbr_surface>
  <surfacematerial name="transp_hint" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="transparency_weight_hint" />
  </surfacematerial>
  <open_pbr_surface name="transparency_weight_hint" type="surfaceshader">
    <input name="transmission_weight" type="float" value="0.8" />
  </open_pbr_surface>
</materialx>

Results

Opacity Test Transparency Test
image image

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 like a good improvement to me, thanks @kwokcb!

@jstone-lucasfilm jstone-lucasfilm merged commit 1d066bb into AcademySoftwareFoundation:main Aug 1, 2024
34 checks passed
@ld-kerley
Copy link
Contributor

Super happy this is moving forward. I was a bit late looking this over, as I just got back from SIGGRAPH. Reading the concrete code made me start to think about potential future hints, and if we think hints might ever might not be mutually exclusive... By which I mean, do we expect a comma separate list to exist in the "hint" attribute. The specification doesn't really spell this out.

We could:

  1. Update the specification to explicitly state one and only one "hint" is allowed. If we needed overlapping hints we could still support them, but there might be a combinatorial problem if this got out of hand.
  2. Update this code to tokenize the hints.
  3. Ignore this until we hit a case where we need overlapping hints, and address it later.

@jstone-lucasfilm
Copy link
Member

@ld-kerley I would agree that the extension to multiple hints will likely be needed in the future, though the current set of supported hints are unlikely to overlap in practice. Maybe your suggestion (3) is the right approach, where we should keep this idea in reserve until it's required?

@kwokcb
Copy link
Contributor Author

kwokcb commented Aug 3, 2024

For 3., this is should not require any API interface changes if you leave the tokenization up to the caller -- akin to how enum attributes are externally parsed.

Finding conflicting hints is another matter. I guess if you wanted it data-driven you could allow for a new element to denote hint incompatibilities. May be a bit too much, but fully user controllable .

<hintconflict hint1="opacity" hint2="transparency"/>

Maybe leave a new issue for future discussion ?

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.

isTransparentSurface() does not work for OpenPBR
3 participants