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

MAYA-106075 - USD selection highlighting doesn't take Color Settings into consideration #2033

Merged
merged 5 commits into from
Jan 31, 2022
Merged

MAYA-106075 - USD selection highlighting doesn't take Color Settings into consideration #2033

merged 5 commits into from
Jan 31, 2022

Conversation

vlasovi
Copy link
Collaborator

@vlasovi vlasovi commented Jan 26, 2022

Get up-to-date selection color settings from Maya. This includes 'lead' color as well as curve and surface active colors.

williamkrick
williamkrick reviewed Jan 26, 2022
View reviewed changes
lib/mayaUsd/render/vp2RenderDelegate/basisCurves.cpp
: drawScene.GetWireframeColor());
= (_selectionStatus != kUnselected ? drawScene.GetSelectionHighlightColor(
_selectionStatus == kFullyLead ? nullptr : "curve")
: drawScene.GetWireframeColor());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this little bit of logic deciding which color to use based on the selection status move into a common location? I'm not sure if it makes sense on drawScene, maybe if we do a shared parentClass for rprims this logic could live there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think I will address that as part of my future work on points.

williamkrick
williamkrick suggested changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@williamkrick williamkrick left a comment

Choose a reason for hiding this comment

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

I think a lot of the testVP2RenderDelegate* tests will fail because the image diffs will pick up the different color of the selection.

lib/mayaUsd/render/vp2RenderDelegate/proxyRenderDelegate.cpp
{
MDoubleArray colorResult;
std::lock_guard<std::mutex> _(_mayaCommandEngineMutex);
MGlobal::executeCommand(queryCommand, colorResult);
Copy link
Contributor

Choose a reason for hiding this comment

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

If a caller passes in a className which isn't valid do we get some kind of error we can warn about?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

executeCommand runs with displayEnabled = false, so I guess it will not write any error info into log. I think it would be good to set displayEnabled to true here

lib/mayaUsd/render/vp2RenderDelegate/proxyRenderDelegate.cpp Outdated
// Query and return the selection color.
{
MDoubleArray colorResult;
std::lock_guard<std::mutex> _(_mayaCommandEngineMutex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename the std::lock_guard local variable to something meaningful? I can guess at your thinking that nobody should be doing anything to it after it is created because of the RAII nature of the object so the name doesn't matter, but I don't like the style of naming it underscore. It just looks strange to me and then I spent some time thinking about it.

@vlasovi
- Fixing the build for Maya versions below 2023
08a95c8 
- Applying minor improvements after the code review
@vlasovi
Fixing broken autotests: making sure the old, hardcoded selection hig…
33baf46 
…hlight colors (for Maya 2022 and below) match the properly calculated colors in autotests
williamkrick
williamkrick approved these changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@williamkrick williamkrick left a comment

Choose a reason for hiding this comment

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

Looks great!

@kxl-adsk
Copy link

@vlasovi I run the PF and there is still one error to fix:

15:49:14     S:\jenkins\workspace\ecg-mayausd-branch-preflight-pr-python3-pxrusd-mid2-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\proxyRenderDelegate.cpp(1553): error C2039: 'applyViewTransform': is not a member of 'Autodesk::Maya::OpenMaya20220000::MColorPickerUtilities'
15:49:14     P:\shared\Artifactory\Windows\rundev-master-debug\202104141515\runtime\0443d2a\runTime\include\maya/MColorPickerUtilities.h(72): note: see declaration of 'Autodesk::Maya::OpenMaya20220000::MColorPickerUtilities'
15:49:14     S:\jenkins\workspace\ecg-mayausd-branch-preflight-pr-python3-pxrusd-mid2-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\proxyRenderDelegate.cpp(1554): error C2039: 'kInverse': is not a member of 'Autodesk::Maya::OpenMaya20220000::MColorPickerUtilities'
15:49:14     P:\shared\Artifactory\Windows\rundev-master-debug\202104141515\runtime\0443d2a\runTime\include\maya/MColorPickerUtilities.h(72): note: see declaration of 'Autodesk::Maya::OpenMaya20220000::MColorPickerUtilities'

@vlasovi
Copy link
Collaborator Author

vlasovi commented Jan 31, 2022 •
edited
Loading

@kxl-adsk This error is due to the fact that our preflight is using an old version of Maya Preview Release that doesn't have the API I use. The issue should be fixed by the preflight updated that @seando-adsk has in review now: https://git.autodesk.com/media-and-entertainment/ecg-maya-usd/pull/191

@seando-adsk seando-adsk added ready-for-merge Development process is finished, PR is ready for merge vp2renderdelegate Related to VP2RenderDelegate labels Jan 31, 2022
@seando-adsk seando-adsk merged commit 843aede into dev Jan 31, 2022
@seando-adsk seando-adsk deleted the vlasovi/MAYA-106075 branch January 31, 2022 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamkrick williamkrick williamkrick approved these changes

Assignees

@ecp-maya-devops-adsk ecp-maya-devops-adsk

Labels
ready-for-merge Development process is finished, PR is ready for merge vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vlasovi @kxl-adsk @williamkrick @seando-adsk @ecp-maya-devops-adsk