HYDRA-30 : support faces picking in Hydra viewport #139

lanierd-adsk · 2024-06-20T14:19:26Z

No description provided.

lanierd-adsk · 2024-06-24T13:13:38Z

lib/flowViewport/sceneIndex/fvpWireframeSelectionHighlightSceneIndex.cpp

@@ -70,6 +70,9 @@ const std::string selectionHighlightMirrorTag = "_SelectionHighlight";

 SdfPath _GetSelectionHighlightMirrorPathFromOriginal(const SdfPath& originalPath)
 {
+    if (originalPath == SdfPath::AbsoluteRootPath()) {


Is a bug fix for a warning at maya startup

lanierd-adsk · 2024-06-24T13:14:20Z

lib/mayaHydra/hydraExtensions/mhWireframeColorInterfaceImp.cpp

@@ -29,14 +30,6 @@

 PXR_NAMESPACE_USING_DIRECTIVE

-// An implementation for maya of the WireframeColorInterface to get the wireframe color from a prim for selection highlighting


Has been moved to mixedUtils.h to make this common to multiple cpp files

lanierd-adsk · 2024-06-24T13:15:01Z

lib/mayaHydra/hydraExtensions/sceneIndex/CMakeLists.txt

@@ -11,7 +11,7 @@ target_sources(${TARGET_NAME}
    mayaHydraCameraDataSource.cpp
    mayaHydraLightDataSource.cpp
    mayaHydraDefaultLightDataSource.cpp
-    mayaHydraDefaultMaterialDataSource.cpp
+    mayaHydraMaterialDataSource.cpp


mayaHydraDefaultMaterialDataSource has been converted to a generic mayaHydraMaterialDataSource class since nothing special in it was for the default material.

lanierd-adsk · 2024-06-24T13:16:26Z

lib/mayaHydra/hydraExtensions/sceneIndex/mayaHydraSceneIndex.cpp

@@ -696,26 +740,41 @@ bool MayaHydraSceneIndex::AddPickHitToSelectionList(
    const HdxPickHit& hit,
    const MHWRender::MSelectionInfo& /* selectInfo */,
    MSelectionList& selectionList,
-    MPointArray& worldSpaceHitPts)
+    MPointArray& worldSpaceHitPts,
+    bool& isOneNodeInComponentsPickingMode)


This new parameter is to handle when a node is being set in component selection

lanierd-adsk · 2024-06-24T13:17:17Z

lib/mayaHydra/mayaPlugin/renderOverride.cpp

@@ -345,15 +345,27 @@ inline bool areDifferentForOneOfTheseBits(unsigned int val1, unsigned int val2,
    return ((val1 & bitsToTest) != (val2 & bitsToTest));
 }

+inline bool isInComponentsPickingMode(const MHWRender::MSelectionInfo& selectInfo)


Is used to detect the global component picking mode

lanierd-adsk · 2024-06-24T13:18:30Z

@lilike-adsk @debloip-adsk

…ou changed it in the UI.

…omponents picking mode on nodes

debloip-adsk

Thanks for this work! Just a couple of very minor things and questions

debloip-adsk · 2024-06-26T15:32:27Z

lib/mayaHydra/hydraExtensions/adapters/renderItemAdapter.cpp

@@ -142,7 +142,7 @@ void MayaHydraRenderItemAdapter::UpdateFromDelta(const UpdateFromDeltaData& data
    // const bool isNew = flags & MViewportScene::MVS_new;  //not used yet
    const bool visible          = data._flags & MVS::MVS_visible;
    const bool matrixChanged    = data._flags & MVS::MVS_changedMatrix;
-    const bool geomChanged      = (data._flags & MVS::MVS_changedGeometry) || positionsHaveBeenReset;
+          bool geomChanged      = (data._flags & MVS::MVS_changedGeometry) || positionsHaveBeenReset;//Non const as we may modify it later


If MAYA-134200 gets fixed, should this be const again? If so I'd probably mention the issue number here as well so we don't forget it

debloip-adsk · 2024-06-26T15:33:48Z

lib/mayaHydra/hydraExtensions/adapters/renderItemAdapter.cpp

+                MVertexBuffer*     verts = mvb;
+                const unsigned int originalVertexCount = verts->vertexCount();


Super nitpicky, but is there a reason we create a separate variable to point to the same object? As this could just be
const unsigned int originalVertexCount = mvb->vertexCount();

I am not sure what you mean ? We use the vertexCount in 2 places only in this function and keeping a variable for it wouldn't be useful IMO.

It's more that the mvb and verts variables are the same thing, so I wasn't sure why the verts variable was created when we could just re-use mvb. But it's nothing major, I'll approve the PR regardless :)

debloip-adsk · 2024-06-26T15:38:48Z

lib/mayaHydra/hydraExtensions/adapters/renderItemAdapter.cpp

+                const unsigned int originalVertexCount = verts->vertexCount();
+                if (_positions.size() != originalVertexCount) {//Is it different ?
+                    geomChanged = true;
+                    vbIdx = vertexBuffercount; //Stop looping


Optional/Thinking out loud : personally I would have instead gone with adding a check in the for loop's condition that checks for geomChanged not being true, i.e. for (int vbIdx = 0; vbIdx < vertexBuffercount && !geomChanged; vbIdx++).
Feels a bit closer to the intent, though it adds an extra comparison in potentially performance-sensitive code, so I don't know how much of an impact it would have.

You're right, I changed it the way you said.

debloip-adsk · 2024-06-26T15:41:07Z

lib/mayaHydra/hydraExtensions/adapters/renderItemAdapter.cpp

+                        PxOsdOpenSubdivTokens
+                            ->none, // For the OGS normals vertex buffer to be used, we need to use
+                                    // PxOsdOpenSubdivTokens->none


Minor autoformat nitpick, but breaking up the -> statement from its deref'd pointer feels a bit weird to me, I'd probably have the comment as a line above so autoformat doesn't split it

debloip-adsk · 2024-06-26T15:51:43Z

lib/mayaHydra/hydraExtensions/sceneIndex/mayaHydraSceneIndex.cpp

+            _tokens
+                ->MayaFacesSelectionMaterial); // Is an absolute path, not linked to a scene index


Same minor autoformat nitpick about breaking up the -> statement in two, I'd probably have the comment as a line above here as well

debloip-adsk · 2024-06-26T16:48:47Z

lib/mayaHydra/mayaPlugin/renderOverride.cpp

@@ -1690,22 +1700,41 @@ void MtohRenderOverride::_PopulateSelectionList(
    const HdxPickHitVector&          hits,
    const MHWRender::MSelectionInfo& selectInfo,
    MSelectionList&                  selectionList,
-    MPointArray&                     worldSpaceHitPts)
+    MPointArray&                     worldSpaceHitPts, 
+    bool&                            isOneMayaNodeInComponentsPickingMode)


Slight inconsistency : the variable is named isOneMayaNodeInComponentsPickingMode whilst the same conceptual value/variable is named isOneNodeInComponentsPickingMode elsewhere. I think it'd be clearer if everywhere else also used the Maya-explicit name as here, i.e. isOneMayaNodeInComponentsPickingMode

debloip-adsk · 2024-06-26T16:52:29Z

test/lib/mayaUsd/render/mayaToHydra/CMakeLists.txt

@@ -46,6 +46,7 @@ set(INTERACTIVE_TEST_SCRIPT_FILES
    testDataProducerSelHighlight.py|skipOnPlatform:osx
    testPassingNormalsOnMayaNative.py
    testViewportFilters.py
+	testMayaComponentsPicking.py


There is an extra indent here

debloip-adsk · 2024-06-26T16:54:22Z

test/testSamples/testMayaComponentsPicking/testMayaComponentsPicking.ma

+//Maya ASCII 2026ff01 scene
+//Name: componentsPicking.ma
+//Last modified: Wed, Jun 19, 2024 06:08:46 PM
+//Codeset: 1252
+requires maya "2026ff01";


Maya version years need to be changed

This still needs to be changed to a released Maya version, e.g. 2025

debloip-adsk · 2024-06-26T16:58:29Z

lib/mayaHydra/mayaPlugin/renderOverride.cpp

+            // Maya does not create Hydra instances, so if the pick hit instancer
+            // ID isn't empty, it's not a Maya pick hit.
+            if (_mayaHydraSceneIndex && pickInput.pickHit.instancerId.IsEmpty() && _mayaHydraSceneIndex->IsPickedNodeInComponentsPickingMode(pickInput.pickHit)){


Question : I'm assuming the instancerId check is just to accelerate things?

Yes it was a test used by Pierre to double check this is a maya node that is being picked, I did a copy /paste of that same test

debloip-adsk · 2024-06-26T17:02:12Z

lib/mayaHydra/mayaPlugin/renderOverride.cpp

+    //isOneNodeInComponentsPickingMode will be true if one of the picked node is in components picking mode
+    bool isOneNodeInComponentsPickingMode = false;
+    _PopulateSelectionList(outHits, selectInfo, selectionList, worldSpaceHitPts, isOneNodeInComponentsPickingMode);
+    if (isOneNodeInComponentsPickingMode){
+        return false;//When being in components picking on a node, returning false will use maya/OGS for components selection
+    }


Question : is component-picking not simply handled by the if (isInComponentsPickingMode(selectInfo)) check earlier? I think I remember a mention that there were different cases of component picking but I can't remember the other ones (other than right-click a selected object and go into component selection mode)

You're right that this needs clarification, so I've added the following comments to the code :
/*

There are 2 modes of selection picking for components in maya :

You can be in components picking mode, this setting is global.This is detected in the function "isInComponentsPickingMode(selectInfo)"

The second mode is when you right click on a node and choose a component to pick it (e.g : Face), this is

where we use the variable "isOneNodeInComponentsPickingMode" to detect that case, later in this function.
*/

debloip-adsk · 2024-06-27T13:40:11Z

test/testSamples/testMayaComponentsPicking/testMayaComponentsPicking.ma

+//Maya ASCII 2026ff01 scene
+//Name: componentsPicking.ma
+//Last modified: Wed, Jun 19, 2024 06:08:46 PM
+//Codeset: 1252
+requires maya "2026ff01";


This still needs to be changed to a released Maya version, e.g. 2025

debloip-adsk · 2024-06-27T13:42:26Z

lib/mayaHydra/hydraExtensions/adapters/renderItemAdapter.cpp

+                MVertexBuffer*     verts = mvb;
+                const unsigned int originalVertexCount = verts->vertexCount();


It's more that the mvb and verts variables are the same thing, so I wasn't sure why the verts variable was created when we could just re-use mvb. But it's nothing major, I'll approve the PR regardless :)

HYDRA-30 : support faces picking in Hydra viewport

8a28384

lanierd-adsk self-assigned this Jun 20, 2024

Fix a bug found by the tests.

108f8cb

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Jun 21, 2024

HYDRA-30 Support components picking mode and hilite list for nodes

f8472ea

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Jun 24, 2024

lanierd-adsk commented Jun 24, 2024

View reviewed changes

lanierd-adsk requested a review from ppt-adsk June 24, 2024 13:17

Fix : color preference for polygons faces not updated in Hydra when y…

a39367a

…ou changed it in the UI.

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Jun 25, 2024

Remove from the pickhandler the maya specific function to check for c…

5147fac

…omponents picking mode on nodes

lanierd-adsk requested review from debloip-adsk and removed request for ppt-adsk June 26, 2024 08:20

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Jun 26, 2024

debloip-adsk requested changes Jun 26, 2024

View reviewed changes

Fixes from code review.

d16fb80

lanierd-adsk removed their assignment Jun 27, 2024

lanierd-adsk requested a review from debloip-adsk June 27, 2024 07:32

lanierd-adsk self-assigned this Jun 27, 2024

debloip-adsk requested changes Jun 27, 2024

View reviewed changes

Fix scene maya version

3698207

lanierd-adsk removed their assignment Jun 27, 2024

debloip-adsk approved these changes Jun 27, 2024

View reviewed changes

lanierd-adsk self-assigned this Jun 27, 2024

lilike-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jun 27, 2024

lilike-adsk merged commit 33857b5 into dev Jun 27, 2024
10 checks passed

lilike-adsk deleted the lanierd/HYDRA-30 branch June 27, 2024 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HYDRA-30 : support faces picking in Hydra viewport #139

HYDRA-30 : support faces picking in Hydra viewport #139

lanierd-adsk commented Jun 20, 2024

lanierd-adsk Jun 24, 2024 •

edited

Loading

lanierd-adsk Jun 24, 2024

lanierd-adsk Jun 24, 2024

lanierd-adsk Jun 24, 2024

lanierd-adsk Jun 24, 2024

lanierd-adsk commented Jun 24, 2024

debloip-adsk left a comment

debloip-adsk Jun 26, 2024

debloip-adsk Jun 26, 2024

lanierd-adsk Jun 27, 2024

debloip-adsk Jun 27, 2024

debloip-adsk Jun 26, 2024

lanierd-adsk Jun 27, 2024

debloip-adsk Jun 26, 2024

lanierd-adsk Jun 27, 2024

debloip-adsk Jun 26, 2024

debloip-adsk Jun 26, 2024

debloip-adsk Jun 26, 2024

debloip-adsk Jun 26, 2024 •

edited

Loading

debloip-adsk Jun 27, 2024

debloip-adsk Jun 26, 2024

lanierd-adsk Jun 27, 2024

debloip-adsk Jun 26, 2024

lanierd-adsk Jun 27, 2024

debloip-adsk Jun 27, 2024

debloip-adsk Jun 27, 2024

		@@ -29,14 +30,6 @@

		PXR_NAMESPACE_USING_DIRECTIVE

		// An implementation for maya of the WireframeColorInterface to get the wireframe color from a prim for selection highlighting

		MVertexBuffer* verts = mvb;
		const unsigned int originalVertexCount = verts->vertexCount();

		_tokens
		->MayaFacesSelectionMaterial); // Is an absolute path, not linked to a scene index

HYDRA-30 : support faces picking in Hydra viewport #139

HYDRA-30 : support faces picking in Hydra viewport #139

Conversation

lanierd-adsk commented Jun 20, 2024

lanierd-adsk Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanierd-adsk commented Jun 24, 2024

debloip-adsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debloip-adsk Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanierd-adsk Jun 24, 2024 •

edited

Loading

debloip-adsk Jun 26, 2024 •

edited

Loading