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

USDLight : Add Arnold-specific parameters #5801

Merged

Conversation

johnhaddon
Copy link
Member

These are defined via additional auto-apply USD schemas. How best to present the Arnold parameters alongside the generic ones is a bit of an open question. There are two approaches in this PR, in 065113c and 0140300, which are the left and right images below respectively. The left one seems most convenient for folks who don't care about Arnold, and the right one seems most convenient for folks who do. The right one will get worse as we add other renderers, and the left one less so. Since our initial use cases involve Arnold, I think I'm inclined to go with the right one, with the intention of adding filtering and maybe replacing the text with icons in future. But I'm open to other opinions and brighter ideas...

image

@johnhaddon johnhaddon self-assigned this Apr 18, 2024
@WangSnowchen
Copy link

From the user's point of view, the left side is better. The right side looks too cluttered, even if there is a hint or a picture to warn about Arnold's parameters, and it would be easier to manage the parameters in a collapsed menu.

@johnhaddon
Copy link
Member Author

I agree the the left side is "neater", but my concern is that users will be forced to jump backwards and forwards between two different sections to control things that are strongly related - like USD's width/height and Arnold's roundness, or USD's diffuseMultiplier and Arnold's indirect contribution.

@LichiMan
Copy link

I like the idea of having all the related parameters together, but I think it will be soon overloaded when you start adding other renderer parameters. Could it be possible to filter the parameters listed based on render:defaultRenderer?

@johnhaddon
Copy link
Member Author

Could it be possible to filter the parameters listed based on render:defaultRenderer?

It should be pretty doable to add filtering in future, but I don't think it would be based on render:defaultRenderer, because that's not something the USDLight node has access to.

@boberfly
Copy link
Collaborator

I am leaning towards the one on the right if you can filter renderers out easily like what @LichiMan mentioned (with metadata?) but I don't mind the left one - in Solaris if I remember correctly they would just store your different renderers as tabs at the top and just throw all renderer-specific stuff into that so it's much like the left one only that it is in a dedicated tab.

One advantage of the right one is that renderers with very similar/overlapping parameters like ray flags can all be side-by-side to be able to match better where possible for the crazy ones (me included) who want multiple renderers working at once (and in future if USDLux decides on making ray flags renderer-agnostic then the UI will probably be in a similar spot, maybe...)

@tomc-cinesite
Copy link
Contributor

tomc-cinesite commented Apr 19, 2024

I think I'd lean to the right too - as it makes it easier for less technical users to find the parameters they are looking for and work holistically on qualities of a light without having to worry about whether it is a USD native concept or not. In many of the creative lighting scenarios, is it important to the user whether it is a USD or Arnold parameter?

It should be pretty doable to add filtering in future, but I don't think it would be based on render:defaultRenderer, because that's not something the USDLight node has access to.

What about user driven filtering? Eg a manual control at the top of the node, that you could use the userDefaults mechanism to set appropriate defaults for based on the project? Or maybe something at the script level?

@johnhaddon
Copy link
Member Author

What about user driven filtering? Eg a manual control at the top of the node, that you could use the userDefaults mechanism to set appropriate defaults for based on the project? Or maybe something at the script level?

Yeah, that's more or less what I'm picturing - I think it would be useful for the NodeEditor in general. And as a special case for the USDLight node, I think the filter-text-field could have some little buttons next to it so you could add and remove arnold:* with one click.

It wouldn't be userDefault because that applies to a plug, but the filter could definitely be specified by node metadata, so you could define a value for it in a startup file, and then it'd be overridden per-node as the user interacted with it.

To be clear, I'm not proposing to do any of the filtering work in this PR. That would come later when/after we add extensions for other renderers.

Copy link
Contributor

@murraystevenson murraystevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! I've heard a fair few opinions advocating for one way or the other, but I'm also inclined to start with the right, putting us on the hook for filtering as soon as we introduce the next renderer. I'm not a huge fan of the repeated (Arnold)s at the end of the parameter labels, but as you suggest we could potentially replace those with an icon in the future.

Filtering would be useful anyway as there are base usdLuxLight parameters that aren't applicable to all (or any) of our currently supported renderers and it may make sense to not present them if they're not functional for the chosen renderer.

shadow:color - Arnold only
shadow:distance - None
shadow:falloff - None 
shadow:falloffGamma - None
shaping:focus - None
shaping:focusTint - None

If the mixed with filtering presentation proves to be unwieldy as we add more renderers, we could also try grouping by renderer under each section. We did experiment with this initially, but it didn't make a lot of sense with a single renderer implemented as you'd have sections such as Sampling that contain no common plugs and just another Arnold section to open. I mocked this up and it makes a little more sense with some Cycles and 3Delight parameters in the mix (all sections expanded on the left, just the Arnold sections on the right).

usdLightMixedSections

python/GafferUSDUI/USDShaderUI.py Outdated Show resolved Hide resolved
startup/gui/lightEditor.py Show resolved Hide resolved
usdSchemas/GafferArnold.usda Show resolved Hide resolved
usdSchemas/GafferArnold.usda Show resolved Hide resolved
src/IECoreArnold/ShaderNetworkAlgo.cpp Show resolved Hide resolved

# Put Arnold parameters last. We can't do that using the schemas in GafferArnold.usda
# because they provide no control over property ordering.
Gaffer.Metadata.registerValue( GafferUSD.USDLight, "parameters.arnold:*", "layout:index", -1 )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need to come up with a consistent ordering for the Arnold parameters. Currently the order changes randomly on startup, which had me questioning my sanity until I realised what was going on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that hackily in d385bb1, after spending a good amount of time convincing myself that yes, USD was going to return things in a random order, no matter what I did. We might want to revisit when the next renderer gets the UsdLux extension treatment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, though I'm having trouble locating the reordering commit. The one linked is the portal_mode displayGroup fixup and I'm not seeing anything else in the latest push. I also double checked the branch on your fork to rule out any GitHub weirdness (or at least one level of weirdness).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh. It was Haddon rather than GitHub weirdness - I had failed to push the final commit. It's there in e291cb7 now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! My very last pedantic comment would be that having max_bounces in the middle of the of the other Refine parameters feels a bit odd, though I imagine it's there because camera and transmission are shuffled to the end because they are not present for all light types?

As an option, here's an alternate ordering roughly based on the "usual" Arnold presentation.

"aov", "aov_indirect", "portal_mode", "spread", "roundness", "soft_edge", "camera",
"transmission", "sss", "indirect", "volume", "max_bounces", "cast_volumetric_shadows",
"samples", "volume_samples", "resolution"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine it's there because camera and transmission are shuffled to the end because they are not present for all light types?

Yeah, and based my ordering on what I assumed might eventually be possible with pure USD - ordering within each schema, but not across schemas. But that's just an assumption and not a reason to do better in the meantime.

Squashed your suggestion into 2525dc7, and rebased/squashed everything else in preparation for merging.

@johnhaddon
Copy link
Member Author

If the mixed with filtering presentation proves to be unwieldy as we add more renderers, we could also try grouping by renderer under each section...it makes a little more sense with some Cycles and 3Delight parameters in the mix

I suspect it's possible that we'd want to use filtering with this approach as well, so folks aren't seeing irrelevant collapsed sections all the time. Which would bring us back to the "top level section with only an Arnold subsection" conundrum.

Copy link
Contributor

@murraystevenson murraystevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to merge once the Changes.md conflict is resolved.

At this point, this should be giving exactly the same results as loading them from the SdrRegistry (which is populated from the schemas in the first place). The reason for changing is that the SdrRegistry intentionally ignores auto-apply schemas used to add renderer-specific properties to lights, and we don't want to ignore them. By loading from the schemas directly ourselves, we'll be able to use auto-apply schemas in an upcoming commit.

It may be that we'll be able to load everything (including regular shaders) from schemas in future, according to a [comment from spiffmon](PixarAnimationStudios/OpenUSD#2497 (comment)) :

> I'd like to really explore a future where Sdr is primarily a rendering-focused subsystem...but enable USD clients/tools to remain unaware of it. In other words, push forward on what we started in our integration of RenderMan into USD, where `usdgenschemafromsdr` can be leveraged to produce schemas not just for "shader like" RenderMan objects/API's, but for shaders in general.
This provides UI metadata for UsdLux lights now that they are loaded from the Schema registry rather than the Sdr registry. Pleasingly, more information is available this way, so we can remove some of our custom metadata from USDLightUI, and we now get help text.

This also adds support for `page` and `label` metadata for shaders loaded from the Sdr registry.
I've named these to match the USD convention rather than the Arnold one - "Refine" rather than "Contribution" and "Geometry" rather than "Shape". Hopefully that makes sense.
This involved moving the whole Arnold section below the USD one, so that the tab comes last in the tab group.
And use a label suffix to make it clear that they are Arnold-specific. It seems like this might be preferable for users of Arnold, because the parameters are closer to hand. But it probably won't scale very well as we add schemas for other renderers. I think it might be preferable though, at least in the short term, as our first big use case is trying to replace Arnold lights in an Arnold pipeline.

It feels like this could also be improved aesthetically if we used icons instead of a textual suffix.
I'd initially left these in as a "warts and all" approach to exposing UsdLux, but to a user it's not clear that the TODO is for Usd and not for Gaffer.
@johnhaddon johnhaddon merged commit b4b15d4 into GafferHQ:1.4_maintenance Apr 30, 2024
5 checks passed
@johnhaddon
Copy link
Member Author

Looks good to merge once the Changes.md conflict is resolved.

Resolved and merged...

@johnhaddon johnhaddon deleted the arnoldUsdLuxExtensions branch May 1, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants