Light editor column registration #5857
Conversation
|
I'm marking this as draft for now - the ability to edit the original nodes of a light's shader network is still a work in progress. I think I have enough functionality there to talk through my approach and validate it or not. But there are a number of edge cases still to be addressed like handling spline plug connections, component connections, etc. Also note that the first two commits are from #5846 and will be dropped when that is merged. |
ericmehl
force-pushed
the
lightEditorColumnRegistration
branch
from
b921d97
to
0c0c74b
Compare
|
After talking through it with @johnhaddon, we decided to use the existing I think I've gotten the gist of what we talked about right, and the tests I added for the initial round are passing and interactive tests are working. I didn't do anything with the concept of adding syntax to tweak plugs to allow something like But I think it's ready for a new look, especially for the API that I've added in abbfde2 |
| { | ||
| if( const auto shaderPlug = runTimeCast<ShaderPlug>( plug ) ) | ||
| { | ||
| return const_cast<ValuePlug *>( shaderPlug->parameterSource( m_parameter ) ); |
There was a problem hiding this comment.
This is my main concern for the new API - is this const_cast a valid solution? The NetworkBuilder works with const plugs throughout, which does seem right considering what NetworkBuilder is meant to do.
Just to see how it would go, I tried making the return plug non-const, but the ripple effects of that went all the way up to the Shader API. Which doesn't seem like a good thing.
There was a problem hiding this comment.
I think a const_cast is a valid solution, but I don't think this is the place for it. You're right that NetworkBuilder should continue to work with const plugs, because it must never modify the graph. And it's also right that ShaderPlug::parameterSource() const should return a const result (if the caller only has const access to ShaderPlug, they should only be able to use it to get const access to the rest of the graph). But here we have non-const access to ShaderPlug, so ShaderPlug should also provide us with a non-const overload for parameterSource() that returns us a non-const plug.
| { | ||
| if( const auto shaderPlug = runTimeCast<ShaderPlug>( plug ) ) | ||
| { | ||
| return const_cast<ValuePlug *>( shaderPlug->parameterSource( m_parameter ) ); |
There was a problem hiding this comment.
I think a const_cast is a valid solution, but I don't think this is the place for it. You're right that NetworkBuilder should continue to work with const plugs, because it must never modify the graph. And it's also right that ShaderPlug::parameterSource() const should return a const result (if the caller only has const access to ShaderPlug, they should only be able to use it to get const access to the rest of the graph). But here we have non-const access to ShaderPlug, so ShaderPlug should also provide us with a non-const overload for parameterSource() that returns us a non-const plug.
Previously we were only considering parameter names when looking for a `TweakPlug` to use as the inspection source. Considering the shader name allows inspections to recognize and modify tweaks to shaders other than the output shader.
This opens the way for the `ParameterInspector` to be able to inspect any parameter within a shader network. Because node names and shader names are deduplicated independently, the best way to figure out what node generated a `ShaderNetwork` handle is build the `ShaderNetwork` and query the `NetworkBuilder` internal shader map. Future optimizations could be made to only generate the network elements that are needed for this query (for example, not populating parameter values).
This allows registration of columns for parameters that are not on the light itself, but in a shader connected to the light. This commit requires the use of an Edit Scope to make changes to such parameters in the Light Editor.
ericmehl
force-pushed
the
lightEditorColumnRegistration
branch
from
0c0c74b
to
6ffbc98
Compare
|
I addressed all the notes above, thanks for all the improvements @johnhaddon. I didn't mark them "fixed" individually because I also squashed them into the original commits as I went, but I can note them if that makes the review easier. The commit sequence is hopefully easier to follow now, and I reworked the tests to put the majority of new tests in a single test in Marking as ready for (another) review! |
|
Thanks Eric! |
This adds the ability to register a column in the Light Editor for editing a parameter anywhere within a light's shader network. Previously only parameters on the light itself were editable.
Fixes #5561.
Checklist