HYDRA-845 : Selection highlighting for point instancing of meshes #132
Conversation
There was a problem hiding this comment.
Not much to add that isn't already in the Markdown doc or in code comments, except for the following two questions :
- I've renamed
SceneIndexPathtoConvertUfePathToHydraSelections, but would we prefer keeping both? Since sometimes we may not really care about the selections themselves. Note that differentUfe::Paths could end up mapping to the sameSdfPaths though, for example in the case of point instancer vs. instance selection, where theUfe::Paths would differ but theSdfPaths would both end up pointing to the instancer. - Should we rename
Fvp::PathInterface, since it's not strictly about paths anymore? I would think how we name it also depends on if we keepSceneIndexPathor not, and what other functionality we could end up putting in it.
There was a problem hiding this comment.
Fantastic work, pretty impressive how much additional complexity all this tracking requires... Minor comments from me in this pull request.
As for the path interface, I'm still uncertain as to whether it's a selection-centric class, or a path mapper class? Is there another application besides selection that we don't yet know about? In the end though, this kind of fundamental questioning is outside the scope of this pull request, so we have time to ponder this.
| #if HD_API_VERSION < 66 | ||
| const_cast<HdInstancerTopologySchema&>(originalInstancerTopology).GetMask()->GetTypedValue(0); | ||
| #else | ||
| originalInstancerTopology.GetMask()->GetTypedValue(0); | ||
| #endif |
There was a problem hiding this comment.
Maybe factor this out in an unnamed namespace function?
| #if HD_API_VERSION < 66 | |
| const_cast<HdInstancerTopologySchema&>(originalInstancerTopology).GetMask()->GetTypedValue(0); | |
| #else | |
| originalInstancerTopology.GetMask()->GetTypedValue(0); | |
| #endif | |
| inline HdInstancerTopologySchema& topologySchema(HdInstancerTopologySchema& t) | |
| #if HD_API_VERSION < 66 | |
| return const_cast<HdInstancerTopologySchema&>(t); | |
| #else | |
| return t; | |
| #endif |
There was a problem hiding this comment.
I'm not sure I see what value this brings. If the method takes in a const reference, and does whatever treatment it needs to return a non-const reference, isn't that just a more complicated way of doing a flat-out const_cast? And if it takes in a non-const reference, then there would be no need to use const_cast in the first place? To me this preprocessor bit is mostly to confine the const_casting to the old Hydra versions, and use the new ones for the current & future (which would also allow us to remove the old API usage at a future point should we wish to do so)
There was a problem hiding this comment.
It was just a suggestion for readability and being closer to Don't Repeat Yourself. You have the const_cast ifdef twice, and inside the ifdef you're repeating code. No big deal though.
lib/flowViewport/sceneIndex/fvpWireframeSelectionHighlightSceneIndex.cpp
Outdated
Show resolved
Hide resolved
| #if HD_API_VERSION < 66 | ||
| const_cast<HdInstancerTopologySchema&>(originalInstancerTopology).GetMask()->GetTypedValue(0); | ||
| #else | ||
| originalInstancerTopology.GetMask()->GetTypedValue(0); | ||
| #endif |
There was a problem hiding this comment.
It was just a suggestion for readability and being closer to Don't Repeat Yourself. You have the const_cast ifdef twice, and inside the ifdef you're repeating code. No big deal though.
debloip-adsk
added
the
ready-for-merge
lilike-adsk
deleted the
debloip/HYDRA-845/mesh-point-instancing-selection-highlight
branch
This PR implements point instance selection highlighting of meshes for point instancer, instance & prototype selections.
Note that as part of this PR, the
Fvp::PathInterface::SceneIndexPathmethod was renamed toConvertUfePathToHydraSelections, and now requires returning a vector of a new structPrimSelectionInfo, each containing anSdfPathand a selection data source.Following this, the lead object path tracker now also supports multiple lead prim paths.