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-30 : support faces picking in Hydra viewport #139

Merged
merged 7 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ const std::string selectionHighlightMirrorTag = "_SelectionHighlight";

SdfPath _GetSelectionHighlightMirrorPathFromOriginal(const SdfPath& originalPath)
{
if (originalPath == SdfPath::AbsoluteRootPath()) {
Copy link
Collaborator Author

@lanierd-adsk lanierd-adsk Jun 24, 2024

Choose a reason for hiding this comment

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

Is a bug fix for a warning at maya startup

return originalPath; //Avoid a warning in Hydra
}
return originalPath.ReplaceName(TfToken(originalPath.GetName() + selectionHighlightMirrorTag));
}

Expand Down
64 changes: 48 additions & 16 deletions lib/mayaHydra/hydraExtensions/adapters/renderItemAdapter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

const bool topoChanged = (data._flags & MVS::MVS_changedTopo) || positionsHaveBeenReset;
const bool visibChanged = data._flags & MVS::MVS_changedVisibility;
const bool effectChanged = data._flags & MVS::MVS_changedEffect;
Expand Down Expand Up @@ -192,6 +192,34 @@ void MayaHydraRenderItemAdapter::UpdateFromDelta(const UpdateFromDeltaData& data
static const bool passNormalsToHydra = MayaHydraSceneIndex::passNormalsToHydra();

const int vertexBuffercount = geom ? geom->vertexBufferCount() : 0;

//Temp workaround for a bug in Maya MAYA-134200
if ((!geomChanged && topoChanged) && vertexBuffercount) {
//With face components selection, we have topoChanged which is true but geomChanged is false, but this is wrong, the number of vertices may have changed.
//We want to check here if we also need to update the geometry if the number of vertices is different from what is stored already
for (int vbIdx = 0; vbIdx < vertexBuffercount; vbIdx++) {
MVertexBuffer* mvb = geom->vertexBuffer(vbIdx);
if (!mvb) {
continue;
}

const MVertexBufferDescriptor& desc = mvb->descriptor();
const auto semantic = desc.semantic();
switch (semantic) {
case MGeometry::Semantic::kPosition: {
// Vertices
MVertexBuffer* verts = mvb;
const unsigned int originalVertexCount = verts->vertexCount();
Comment on lines +211 to +212
Copy link
Collaborator

Choose a reason for hiding this comment

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

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();

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 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)

if (_positions.size() != originalVertexCount) {//Is it different ?
geomChanged = true;
vbIdx = vertexBuffercount; //Stop looping
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 changed it the way you said.

}
} break;
default: break;
}
}
}

// Vertices
if (geomChanged && vertexBuffercount) {
//vertexBuffercount > 0 means geom is non null
Expand Down Expand Up @@ -356,21 +384,25 @@ void MayaHydraRenderItemAdapter::UpdateFromDelta(const UpdateFromDeltaData& data
switch (GetPrimitive()) {
case MGeometry::Primitive::kTriangles:{
static const bool passNormalsToHydra = MayaHydraSceneIndex::passNormalsToHydra();
if (passNormalsToHydra){
_topology.reset(new HdMeshTopology(
PxOsdOpenSubdivTokens->none,//For the OGS normals vertex buffer to be used, we need to use PxOsdOpenSubdivTokens->none
UsdGeomTokens->rightHanded,
vertexCounts,
vertexIndices));
} else{
_topology.reset(new HdMeshTopology(
(GetMayaHydraSceneIndex()->GetParams().displaySmoothMeshes
|| GetDisplayStyle().refineLevel > 0)
? PxOsdOpenSubdivTokens->catmullClark
: PxOsdOpenSubdivTokens->none,
UsdGeomTokens->rightHanded,
vertexCounts,
vertexIndices));
if (vertexCounts.size()) {
if (passNormalsToHydra) {
_topology.reset(new HdMeshTopology(
PxOsdOpenSubdivTokens
->none, // For the OGS normals vertex buffer to be used, we need to use
// PxOsdOpenSubdivTokens->none
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

UsdGeomTokens->rightHanded,
vertexCounts,
vertexIndices));
} else {
_topology.reset(new HdMeshTopology(
(GetMayaHydraSceneIndex()->GetParams().displaySmoothMeshes
|| GetDisplayStyle().refineLevel > 0)
? PxOsdOpenSubdivTokens->catmullClark
: PxOsdOpenSubdivTokens->none,
UsdGeomTokens->rightHanded,
vertexCounts,
vertexIndices));
}
}
}
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

//Local headers
#include "mhWireframeColorInterfaceImp.h"
#include "mixedUtils.h"

//Flow viewport headers
#include <flowViewport/colorPreferences/fvpColorPreferences.h>
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

namespace {
PXR_NS::GfVec4f getPreferencesColor(const PXR_NS::TfToken& token) {
PXR_NS::GfVec4f color;
Fvp::ColorPreferences::getInstance().getColor(token, color);
return color;
}
}
namespace MAYAHYDRA_NS_DEF {

MhWireframeColorInterfaceImp::MhWireframeColorInterfaceImp(const std::shared_ptr<Fvp::Selection>& selection
Expand Down
9 changes: 9 additions & 0 deletions lib/mayaHydra/hydraExtensions/mixedUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

#include "mixedUtils.h"

#include <flowViewport/colorPreferences/fvpColorPreferences.h>

#include <mayaHydraLib/adapters/mayaAttrs.h>
#include <mayaHydraLib/hydraUtils.h>

Expand Down Expand Up @@ -172,4 +174,11 @@ bool getIndexedColorPreferenceValue(
return false;
}

PXR_NS::GfVec4f getPreferencesColor(const PXR_NS::TfToken& token)
{
PXR_NS::GfVec4f color;
Fvp::ColorPreferences::getInstance().getColor(token, color);
return color;
}

} // namespace MAYAHYDRA_NS_DEF
9 changes: 9 additions & 0 deletions lib/mayaHydra/hydraExtensions/mixedUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,15 @@ bool getIndexedColorPreferenceValue(
const std::string& tableName,
PXR_NS::GfVec4f& outColor);

/**
* @brief Retrieves a color preference from Maya using the Flow Viewport Color Preferences API.
*
* @param[in] token is a Flow Viewport color preferences token e.g : FvpColorPreferencesTokens->polymeshDormant
*
* @return the color that will be populated if retrieved from Maya.
*/
PXR_NS::GfVec4f getPreferencesColor(const PXR_NS::TfToken& token);

} // namespace MAYAHYDRA_NS_DEF

#endif // MAYAHYDRALIB_MIXED_UTILS_H
4 changes: 2 additions & 2 deletions lib/mayaHydra/hydraExtensions/sceneIndex/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ target_sources(${TARGET_NAME}
mayaHydraCameraDataSource.cpp
mayaHydraLightDataSource.cpp
mayaHydraDefaultLightDataSource.cpp
mayaHydraDefaultMaterialDataSource.cpp
mayaHydraMaterialDataSource.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.

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

mayaHydraMayaDataProducerSceneIndexData.cpp
mayaHydraMayaDataProducerSceneIndexDataConcreteFactory.cpp
mayaHydraSceneIndexDataFactoriesSetup.cpp
Expand All @@ -31,7 +31,7 @@ set(HEADERS
mayaHydraLightDataSource.h
mayaHydraSceneIndexUtils.h
mayaHydraDefaultLightDataSource.h
mayaHydraDefaultMaterialDataSource.h
mayaHydraMaterialDataSource.h
mayaHydraMayaDataProducerSceneIndexData.h
mayaHydraMayaDataProducerSceneIndexDataConcreteFactory.h
mayaHydraSceneIndexDataFactoriesSetup.h
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// limitations under the License.
//

#include "mayaHydraDefaultMaterialDataSource.h"
#include "mayaHydraMaterialDataSource.h"

#include <mayaHydraLib/sceneIndex/mayaHydraSceneIndex.h>
#include <mayaHydraLib/sceneIndex/mayaHydraSceneIndexUtils.h>
Expand All @@ -32,7 +32,7 @@ PXR_NAMESPACE_OPEN_SCOPE

// ----------------------------------------------------------------------------

MayaHydraDefaultMaterialDataSource::MayaHydraDefaultMaterialDataSource(
MayaHydraMaterialDataSource::MayaHydraMaterialDataSource(
const SdfPath& id,
TfToken type,
MayaHydraSceneIndex* sceneIndex)
Expand All @@ -44,15 +44,15 @@ MayaHydraDefaultMaterialDataSource::MayaHydraDefaultMaterialDataSource(


TfTokenVector
MayaHydraDefaultMaterialDataSource::GetNames()
MayaHydraMaterialDataSource::GetNames()
{
TfTokenVector result;
result.push_back(HdMaterialSchemaTokens->material);
return result;
}

HdDataSourceBaseHandle
MayaHydraDefaultMaterialDataSource::Get(const TfToken& name)
MayaHydraMaterialDataSource::Get(const TfToken& name)
{
if (name == HdMaterialSchemaTokens->material) {
return _GetMaterialDataSource();
Expand All @@ -66,7 +66,7 @@ MayaHydraDefaultMaterialDataSource::Get(const TfToken& name)
}

HdDataSourceBaseHandle
MayaHydraDefaultMaterialDataSource::_GetMaterialBindingDataSource()
MayaHydraMaterialDataSource::_GetMaterialBindingDataSource()
{
const SdfPath path = _sceneIndex->GetMaterialId(_id);
if (path.IsEmpty()) {
Expand All @@ -88,7 +88,7 @@ MayaHydraDefaultMaterialDataSource::_GetMaterialBindingDataSource()
}

HdDataSourceBaseHandle
MayaHydraDefaultMaterialDataSource::_GetMaterialDataSource()
MayaHydraMaterialDataSource::_GetMaterialDataSource()
{
VtValue materialContainer = _sceneIndex->GetMaterialResource(_id);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
// limitations under the License.
//

#ifndef MAYAHYDRADEFAULTMATERIALDATASOURCE_H
#define MAYAHYDRADEFAULTMATERIALDATASOURCE_H
#ifndef SCENEINDEX_MAYA_HYDRA_MATERIAL_DATASOURCE_H
#define SCENEINDEX_MAYA_HYDRA_MATERIAL_DATASOURCE_H

#include <pxr/pxr.h>
#include <pxr/usd/sdf/path.h>
Expand All @@ -27,20 +27,20 @@ PXR_NAMESPACE_OPEN_SCOPE
class MayaHydraSceneIndex;

/**
* \brief A container data source representing a default material with USDPreviewSurface
* \brief A generic container data source representing a maya hydra material
*/
class MayaHydraDefaultMaterialDataSource : public HdContainerDataSource
class MayaHydraMaterialDataSource : public HdContainerDataSource
{
public:
HD_DECLARE_DATASOURCE(MayaHydraDefaultMaterialDataSource);
HD_DECLARE_DATASOURCE(MayaHydraMaterialDataSource);

// ------------------------------------------------------------------------
// HdContainerDataSource implementations
TfTokenVector GetNames() override;
HdDataSourceBaseHandle Get(const TfToken& name) override;

private:
MayaHydraDefaultMaterialDataSource(
MayaHydraMaterialDataSource(
const SdfPath& id,
TfToken type,
MayaHydraSceneIndex* sceneIndex);
Expand All @@ -53,8 +53,8 @@ class MayaHydraSceneIndex;
MayaHydraSceneIndex* _sceneIndex = nullptr;
};

HD_DECLARE_DATASOURCE_HANDLES(MayaHydraDefaultMaterialDataSource);
HD_DECLARE_DATASOURCE_HANDLES(MayaHydraMaterialDataSource);

PXR_NAMESPACE_CLOSE_SCOPE

#endif // MAYAHYDRADEFAULTMATERIALDATASOURCE_H
#endif // SCENEINDEX_MAYA_HYDRA_MATERIAL_DATASOURCE_H
Loading