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

USD pick with kind support, with test. #115

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

ppt-adsk
Copy link
Collaborator

@ppt-adsk ppt-adsk commented Apr 3, 2024

USD defines a prim-level metadatum called Kind, to establish a model hierarchy, for organization and ease of navigation.
https://openusd.org/release/glossary.html#usdglossary-kind
This pull request implements Kind support on pick.

@ppt-adsk ppt-adsk requested a review from debloip-adsk April 3, 2024 17:04
@ppt-adsk ppt-adsk self-assigned this Apr 3, 2024
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realize that this file is becoming a picking kitchen sink. Will think about extracting this in a later pull request.

//! \return A Kind token (https://graphics.pixar.com/usd/docs/api/kind_page_front.html). If the
//! token is empty or non-existing in the hierarchy, the exact prim that gets picked
//! in the viewport will be selected.
TfToken GetSelectionKind()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -47,8 +47,9 @@ set(INTERACTIVE_TEST_SCRIPT_FILES
cpp/testSceneCorrectness.py
cpp/testPrimInstancing.py
cpp/testPicking.py
cpp/testPointInstancePicking.py
cpp/testUsdPointInstancePicking.py
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rename for consistency: these tests pick USD data.

testPointInstancePicking.cpp
testUsdNativeInstancePicking.cpp
testUsdPointInstancePicking.cpp
testUsdPicking.cpp
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The USD native instance picking C++ test code was not specific to native instancing, so renamed it for generality.

@ppt-adsk ppt-adsk assigned ppt-adsk and unassigned ppt-adsk Apr 4, 2024
@@ -38,6 +38,7 @@ target_compile_definitions(${TARGET_NAME}
$<$<BOOL:${IS_LINUX}>:LINUX>
# Not sure if msvcc sets this automatically, but won't hurt to redefine
$<$<BOOL:${IS_WINDOWS}>:_WIN32>
$<$<BOOL:${MayaUsd_FOUND}>:MAYAHYDRALIB_MAYAUSDAPI_ENABLED>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question : The three added lines in this file are already present in the CMakeLists of mayaHydraLib/hydraExtensions, should we consider making them public there instead of duplicating them here, or do we want to keep it separate? (same goes for UFE, where we also repeat some include directories and link libraries across ufeExtensions, hydraExtensions and mayaPlugin)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it helps to keep the dependencies explicit, but I don't have a strong opinion here.

lib/mayaHydra/mayaPlugin/renderOverride.cpp Show resolved Hide resolved
Comment on lines 501 to 534
const auto snMayaPath = (instanceNdx >= 0) ?

// Point instance: add the instance index to the path. Appending a
// numeric component to the path to identify a point instance
// cannot be done on the picked SdfPath, as numeric path components
// are not allowed by SdfPath. Do so here with Ufe::Path, which
// has no such restriction.
(pickedMayaPath + std::to_string(instanceNdx)) :

// Not an instance: adjust picked path for selection kind.
// As per https://stackoverflow.com/questions/46114214
// structured bindings cannot be captured by a lambda in C++ 17,
// so pass in pickedUsdPath as a lambda argument.
[&pickedMayaPath, &registration](const SdfPath& pickedUsdPath) {
auto snKind = GetSelectionKind();
if (snKind.IsEmpty()) {
return pickedMayaPath;
}

// Get the prim from the stage and path, to access the
// UsdModelAPI for the prim.
auto proxyShapeObj = registration->dagNode.object();
if (proxyShapeObj.isNull()) {
TF_FATAL_ERROR("No mayaUsd proxy shape object corresponds to USD pick");
return pickedMayaPath;
}

MayaUsdAPI::ProxyStage proxyStage{proxyShapeObj};
auto prim = proxyStage.getUsdStage()->GetPrimAtPath(pickedUsdPath);
prim = GetPrimOrAncestorWithKind(prim, snKind);
const auto usdPath = prim ? prim.GetPath() : pickedUsdPath;

return usdPathToUfePath(registration, usdPath);
}(pickedUsdPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion : I find the lambda nested in a tertiary a bit hard to read personally, I would propose moving the (instanceNdx >= 0) case inside the lambda as well and handle it with an early return like the other cases, so that it's a bit more consistent and snMayaPath is directly the result of the lambda.

debloip-adsk
debloip-adsk previously approved these changes Apr 5, 2024
lib/mayaHydra/mayaPlugin/renderOverride.cpp Show resolved Hide resolved
@ppt-adsk ppt-adsk assigned ppt-adsk and unassigned ppt-adsk Apr 5, 2024
@ppt-adsk ppt-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Apr 5, 2024
@roopavr-adsk roopavr-adsk merged commit 110034b into dev Apr 5, 2024
10 checks passed
@roopavr-adsk roopavr-adsk deleted the tremblp/HYDRA-316/pick_kind_support branch April 5, 2024 18:53
@bjorn-siegert-adsk bjorn-siegert-adsk added the enhancement New feature or request label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants