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.

Sign up for GitHub

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

Jump to bottom

HYDRA-307: Add the FootPrint node as an Hydra example #75

Merged
merged 14 commits into from
Mar 6, 2024
Merged

HYDRA-307: Add the FootPrint node as an Hydra example #75

merged 14 commits into from
Mar 6, 2024

Conversation

lanierd-adsk
Copy link
Collaborator

No description provided.

@lanierd-adsk lanierd-adsk self-assigned this Feb 15, 2024
@lanierd-adsk
Copy link
Collaborator Author

FYI : @lilike-adsk @debloip-adsk @roopavr-adsk @vlasovi

ppt-adsk
ppt-adsk requested changes Feb 15, 2024
View reviewed changes
Copy link
Collaborator

@ppt-adsk ppt-adsk left a comment

Choose a reason for hiding this comment

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

Great work David! Some changes and questions sprinkled in.

lib/mayaHydra/flowViewportAPIExamples/footPrintNode/CMakeLists.txt Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/CMakeLists.txt Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/CMakeLists.txt Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/CMakeLists.txt Outdated Show resolved Hide resolved
test/lib/mayaUsd/render/mayaToHydra/testFootPrintNode.py Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/mhFootPrintNode.cpp Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/mhFootPrintNode.cpp Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/mhFootPrintNode.cpp Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/mhFootPrintNode.cpp Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/mhFootPrintNode.cpp Outdated Show resolved Hide resolved
@lanierd-adsk lanierd-adsk added the fvp-toolkit Flow Viewport Toolkit label Feb 20, 2024
ppt-adsk
ppt-adsk reviewed Feb 20, 2024
View reviewed changes
Copy link
Collaborator

@ppt-adsk ppt-adsk left a comment

Choose a reason for hiding this comment

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

Looks pretty good, but I'd still like to explore the use of the compute() method to do scene index work.

lib/mayaHydra/flowViewportAPIExamples/footPrintNode/mhFootPrintNode.cpp Outdated Show resolved Hide resolved
test/lib/mayaUsd/render/mayaToHydra/testFootPrintNode.py Show resolved Hide resolved
test/lib/mayaUsd/render/mayaToHydra/testFootPrintNode.py Outdated Show resolved Hide resolved
test/lib/mayaUsd/render/mayaToHydra/testFootPrintNode.py Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/mhFootPrintNode.cpp Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/mhFootPrintNode.cpp Outdated Show resolved Hide resolved
lib/mayaHydra/flowViewportAPIExamples/footPrintNode/mhFootPrintNode.cpp Outdated Show resolved Hide resolved
ppt-adsk
ppt-adsk requested changes Feb 22, 2024
View reviewed changes
lib/mayaHydra/flowViewportAPIExamples/flowViewportAPIMayaLocator/flowViewportAPIMayaLocator.cpp Show resolved Hide resolved
test/lib/mayaUsd/render/mayaToHydra/testFlowViewportAPI.py Outdated
@@ -102,7 +102,7 @@ def test_AddingPrimitives(self):
self.setHdStormRenderer()
self.assertSnapshotClose("add_VP2AndThenBackToStorm.png", self.IMAGE_DIFF_FAIL_THRESHOLD, self.IMAGE_DIFF_FAIL_PERCENT)

#Finish by a File New command
#Finish by a File New command to check that it's not crashing when cleaning up everything
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, should have caught this earlier, since you're calling file new after each test, you should factor this out into a tearDown() method:
https://docs.python.org/3/library/unittest.html#unittest.TestCase.tearDown

test/lib/mayaUsd/render/mayaToHydra/testFootPrintNode.py Outdated Show resolved Hide resolved
ppt-adsk
ppt-adsk reviewed Feb 26, 2024
View reviewed changes
test/lib/mayaUsd/render/mayaToHydra/testFlowViewportAPI.py
@@ -38,6 +38,11 @@ class TestFlowViewportAPI(mtohUtils.MtohTestCase): #Subclassing mtohUtils.MtohTe
IMAGE_DIFF_FAIL_THRESHOLD = 0.1
IMAGE_DIFF_FAIL_PERCENT = 2

@classmethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not equivalent to what you had before: previously, you did a file new at the end of each test method. As I suggested in
https://github.com/Autodesk/maya-hydra/pull/75/files/e3184227bf913bbbb1f48fb2e75ad2b8b5944239..eff7ba8689bcfe5db288d514517fba122b1e90a5#r1499561088
the proper equivalent is tearDown():
https://docs.python.org/3/library/unittest.html#unittest.TestCase.tearDown
If you're O.K. with tearDownClass() then that's fine too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I want to use tearDown.

test/lib/mayaUsd/render/mayaToHydra/testFootPrintNode.py Outdated Show resolved Hide resolved
@lanierd-adsk lanierd-adsk requested a review from ppt-adsk March 4, 2024 10:45
ppt-adsk
ppt-adsk requested changes Mar 4, 2024
View reviewed changes
Copy link
Collaborator

@ppt-adsk ppt-adsk left a comment

Choose a reason for hiding this comment

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

Looks really good, don't forget to change tearDownClass() to tearDown() in testFlowViewportAPI.py!

test/lib/mayaUsd/render/mayaToHydra/testFootPrintNode.py Show resolved Hide resolved
@lanierd-adsk
Copy link
Collaborator Author

Looks really good, don't forget to change tearDownClass() to tearDown() in testFlowViewportAPI.py!

Yes this is being made in another PR : #85

@ppt-adsk
Copy link
Collaborator

ppt-adsk commented Mar 5, 2024

Looks really good, don't forget to change tearDownClass() to tearDown() in testFlowViewportAPI.py!

Yes this is being made in another PR : #85

O.K., change is not in that pull request yet, but if that's where you want to make it sounds good.

@lanierd-adsk lanierd-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Mar 6, 2024
@roopavr-adsk roopavr-adsk merged commit b4fb348 into dev Mar 6, 2024
10 of 15 checks passed
@roopavr-adsk roopavr-adsk deleted the lanierd/HYDRA-307 branch March 6, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppt-adsk ppt-adsk ppt-adsk approved these changes

Assignees

@lanierd-adsk lanierd-adsk

Labels
fvp-toolkit Flow Viewport Toolkit ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lanierd-adsk @ppt-adsk @roopavr-adsk