-
Notifications
You must be signed in to change notification settings - Fork 5
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
HYDRA-794 : Support maya lighting modes for USD and dome lights #167
Conversation
…kind of lights actually)
FYI : @debloip-adsk @lilike-adsk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, like the light management scene index, but a bit uncomfortable with the Arnold dome light special case. Can we make this more generic somehow?
@@ -32,7 +32,8 @@ def setUp(self): | |||
# Open simple Maya scene | |||
testFile = mayaUtils.openTestScene( | |||
"testUsdTextureToggle", | |||
"testUsdTextureToggle.ma") | |||
"testUsdTextureToggle.ma", useTestSettings=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about this change. Is it to avoid
maya-hydra/test/testUtils/mayaUtils.py
Line 145 in 3e96b72
resetDefaultLightIntensity() |
? If so, perhaps it would be better to keep useTestSettings as True, then set the default light intensity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the intensity of lights doesn't match, I would rather keep it like that as it works fine.
|
||
void _DisableLight(HdSceneIndexPrim& prim) | ||
{ | ||
//We don't set the intensity to 0 as for domelights this makes disappear the geometry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intensity behavior the same for all dome lights, or specific to the Arnold one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for all dome lights as this is happening in Storm, when the dome light intensity is 0 it is removed, not only ignored.
const Ufe::GlobalSelection::Ptr& globalUfeSelection = Ufe::GlobalSelection::get(); | ||
const Ufe::Selection& ufeSelection = *globalUfeSelection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor and optional (depends on personal preference really): since you don't use globalUfeSelection again, may as well just do
const Ufe::GlobalSelection::Ptr& globalUfeSelection = Ufe::GlobalSelection::get(); | |
const Ufe::Selection& ufeSelection = *globalUfeSelection; | |
const Ufe::Selection& ufeSelection = *Ufe::GlobalSelection::get(); |
case LightingMode::kSelectedLightsOnly: { | ||
const Ufe::GlobalSelection::Ptr& globalUfeSelection = Ufe::GlobalSelection::get(); | ||
const Ufe::Selection& ufeSelection = *globalUfeSelection; | ||
if (0 == ufeSelection.size()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor:
if (0 == ufeSelection.size()) { | |
if (ufeSelection.empty()) { |
* | ||
* @return true if the object is a sky dome light, false otherwise | ||
*/ | ||
bool IsDagPathAnArnoldSkyDomeLight(const MDagPath& dagPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me uncomfortable. Why are we treating Arnold lights in a special way, with a special conditional? Is there no way that our adapter infrastructure can handle this? What if another renderer has a different way of handling dome lights?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specific to maya hydra, not specific to a renderer.
lib/mayaHydra/hydraExtensions/sceneIndex/mayaHydraSceneIndex.cpp
Outdated
Show resolved
Hide resolved
Good work, it would be better to manage the maya built-in lights by the new SceneIndex, lets track it in another ticket. |
All kinds of lights actually