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

HYDRA-1175 : Use LightsManagement scene index instead of removing the… #249

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

lanierd-adsk
Copy link
Collaborator

… light prims for maya native data, update the unit tests

… light prims for maya native data, update the unit tests
@lanierd-adsk lanierd-adsk self-assigned this Feb 13, 2025
@lanierd-adsk lanierd-adsk self-assigned this Feb 18, 2025
const std::set<PXR_NS::SdfPath>& activeLightsPrims)
{
if (_disabledLightsPrims != activeLightsPrims) {
_disabledLightsPrims = activeLightsPrims;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bit confused on the nomenclature here, shouldn't the parameter name be disabledLightPrims and not activeLightsPrims? The prims passed here are the non-active ones, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I started by naming this function SetActiveLightsPrims, this is a leftover from that naming, I'll fix it.

@@ -151,7 +196,7 @@ createNode aiImagerDenoiserOidn -s -n "defaultArnoldDenoiser";
rename -uid "F3377A5A-4A07-7C62-07C5-D68E104F2CB2";
createNode file -n "file1";
rename -uid "6E5F111E-40E1-79BC-D1C7-2588F65A88E1";
setAttr ".ftn" -type "string" "./UVChecker.png";
setAttr ".ftn" -type "string" "D:/GIT/maya-hydra-opensource/test/testSamples/testMayaLightingModes/UVChecker.png";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if it's going to cause issues, but can this path be made relative?

@@ -33,8 +33,11 @@ def IMAGE_DIFF_FAIL_PERCENT(self):
# Only the "all lights with shadows" check fails, and shadows with point light only is skipped.
if platform.system() == "Darwin":
return 3
if mayaUtils.mayaMajorVersion() == 2025:
return 2 #There are more pixels different in maya 2025 for this test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just making sure, but I'm guessing the difference is small enough that it doesn't warrant separate reference images?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No I had an error of about 2440 pixels (~1.5%).
I did an image compare to see the differences and it was sparse pixels here and there, nothing important and only in maya 2025 under Windows and Linux...

@debloip-adsk
Copy link
Collaborator

Few questions, overall looking good, thanks!

@@ -71,9 +71,13 @@ class LightsManagementSceneIndex : public PXR_NS::HdSingleInputFilteringSceneInd
FVP_API
LightingMode GetLightingMode()const {return _lightingMode;}

protected:
FVP_API
void SetDisabledLightsPrims(const std::set<PXR_NS::SdfPath>& activeLightsPrims);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The parameter should also be renamed here

@lanierd-adsk lanierd-adsk removed their assignment Feb 19, 2025
@lanierd-adsk lanierd-adsk self-assigned this Feb 19, 2025
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.

2 participants