ParameterInspector : Support defaultValue metadata#6944
ParameterInspector : Support defaultValue metadata#6944murraystevenson wants to merge 6 commits into
defaultValue metadata#6944Conversation
|
Thanks Murray! A very worthwhile improvement, and nice to see shaders start to join the great metadata unification. It's a pity that we require an EditScope to edit the not-yet-created parameters, rather than also being able to pop up an editor for the OptionalValuePlug on the light node itself. I think that would require improvements to the Inspector API though - might be one to add to the list. |
Agreed, we do cheat this a bit already in the AttributeInspector where |
johnhaddon
left a comment
johnhaddon
left a comment
There was a problem hiding this comment.
Sorry, yesterday I did the thing where I reviewed and then left all the comments pending. Here are the comments...
| } | ||
| else | ||
| { | ||
| if( const auto defaultValue = Gaffer::Metadata::value( fmt::format( "{}:{}:{}", attribute, shader->getName(), parameter.name.string() ), "defaultValue" ) ) |
There was a problem hiding this comment.
I think it should be shader->getType() here rather than attribute. Not important for lights where the two will be the same, but important for shaders within a network (not the terminal shader) where type is likely to be ai:shader rather than ai:surface.
There was a problem hiding this comment.
Ah yes, good point! I've addressed this in f75a6c4.
| } | ||
|
|
||
| auto attributeHistory = static_cast<const SceneAlgo::AttributeHistory *>( history ); | ||
| if( const auto defaultValue = Gaffer::Metadata::value( fmt::format( "{}:{}:{}", attributeHistory->attributeName.string(), shaderName, m_parameter.name.string() ), g_defaultValue ) ) |
There was a problem hiding this comment.
Another one where I think we want shader->getType() rather than attributeName.
| if( const auto inspection = inspector->inspect() ) | ||
| { | ||
| // We only want to show handles for parameters with authored | ||
| // values, so we omit inspections returning a fallback value. | ||
| if( inspection->fallbackDescription().empty() ) | ||
| { | ||
| m_inspectors[m] = inspector; | ||
| } | ||
| } |
There was a problem hiding this comment.
This gives behaviour that is slightly inconsistent I think :
- Make 2 SphereLights and add a cone angle to 1.
- Select the one with the cone and enable the tool, the handles appear. Good.
- Now add the second light to the selection. The handles remain, but are greyed out. Given the behaviour of other mixed spotlight/non-spotlight selections (e.g. with
ai:spot_lightandai:quad_light) we'd expect the handles to disappear.
I took a stab at addressing this using LightHandle::visible(), and it seems to work :
If that looks OK, then it also has the benefit of not introducing any additional inspections.
There was a problem hiding this comment.
Nice, that is a more elegant solution. I've cherry-picked it and dropped the other commit.
This brings ParameterInspector in line with AttributeInspector and OptionInspector and will allow us to start using `defaultValue` metadata to show the default value of un-authored USD light parameters in the Light Editor.
This dips part of a toe in the waters of a shader parameter metadata registry. This registration is just enough to make the LightEditor more usable when editing previously un-authored parameters on USD lights. We want to expand this metadata in the future to provide enough registrations to replace the USD SchemaRegistry queries in USDShader and USDShaderUI.
With ParameterInspector's new support for returning fallback values from registered "defaultValue" metadata, the SceneViewInspector would display "shaping:cone:angle" and "shaping:cone:softness" inspections for USD lights even when the light didn't have an authored value for the parameter.
This matches the behaviour when creating attribute and option tweaks and is more intuitive now that we could be creating tweaks without an upstream parameter to replace.
murraystevenson
force-pushed
the
parameterDefaultValueMetadata
branch
from
5db1b16 to
f75a6c4
Compare
2 participants
This updates ParameterInspector to support
defaultValuemetadata registrations and adds registrations for USD lights. The primarly motivating factor here is to improve support for editing USD lights in the Light Editor, where we currently require the parameter to exist on the light before it can be edited downstream.One behavioural change worth flagging is the default tweak mode of newly created parameter tweaks has changed from
ReplacetoCreateasReplacemode is not so intuitive now that it's possible to create new parameters with nothing upstream to replace.