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

MAYA-110276 - USD: curves drawing color is the exact same as the maya viewport Gray background #1998

Merged
merged 4 commits into from
Jan 20, 2022

Conversation

vlasovi
Copy link
Collaborator

@vlasovi vlasovi commented Jan 19, 2022

USD curves default color now matches Maya curves default color

if (colorArray.empty()) {
colorArray.push_back(GfVec3f(0.18f, 0.18f, 0.18f));
MDoubleArray curveColorResult;
MGlobal::executeCommand(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this is not safe to call here. MGlobal::executeCommand calls into Maya's command engine which is not threadsafe. This code can be executed in parallel during the USD sync, so if multiple threads got in here together it could crash.

I think the approach should be to move the code that queries the colors over to ProxyRenderDelegate::update(). There we know Maya is executing serially so we should be able to safely call the command engine, save the values and then reference them here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. This may be unsafe. I will make sure to address that.

…nd Engine from USD Sync methods. Use the mutex for getting the curves' default color.
@vlasovi
Copy link
Collaborator Author

vlasovi commented Jan 20, 2022

@williamkrick There is a call to MGlobal::executeCommandStringResult inside HdVP2Material::Sync that we should probably also fix in the same way that this one.

@williamkrick
Copy link
Contributor

@vlasovi You are right I think we should handle that MGlobal call in a similar way, it is nice to encapsulate that kind of thing.

In the HdVP2Material case we won't have a race. The material is an sprim and sprims get their sync calls serially. basisCurve's are rprims and get the parallel sync. You can see the code responsible for that in HdRenderIndex::SyncAll() if you are curious.

I still prefer to encapsulate that global access.

@kxl-adsk kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jan 20, 2022
@kxl-adsk kxl-adsk merged commit eb3b40d into dev Jan 20, 2022
@kxl-adsk kxl-adsk deleted the vlasovi/MAYA-110276 branch January 20, 2022 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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