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-121882 Share the cached HdVP2TextureInfo between all materials so that isSRGB #2145

Merged
merged 4 commits into from
Mar 1, 2022

Conversation

williamkrick
Copy link
Contributor

@williamkrick williamkrick commented Feb 25, 2022

and uvScaleOffset and re-used correctly when hitting a texture in the cache.

…B and uvScaleOffset and re-used correctly when hitting a texture in the cache.
@williamkrick
Copy link
Contributor Author

williamkrick commented Feb 25, 2022

@csyshing I'm not sure if this impacting the issues with background material loading but it is in a similar enough area I thought you should be aware of the change.

@JGamache-autodesk we need to save the extra information that is HdVP2TextureInfo so that when another material hits the Maya texture cache we get these values right.

The relevant bit of code is https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/render/vp2RenderDelegate/material.cpp#L1006. Here if we hit the texture manager cache we early exit from the function and don't set the isSRGB value or the uvScaleOffset.

We search the _textureMap at https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/render/vp2RenderDelegate/material.cpp#L2768. This code is only every executed serially. We also call it with async material loading in UpdateLoadedTexture https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/render/vp2RenderDelegate/material.cpp#L2859, but that is also serial.

Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@JGamache-autodesk
Copy link
Collaborator

I strongly suspect this will help with #2142.

csyshing
csyshing previously approved these changes Feb 27, 2022
@csyshing
Copy link
Collaborator

@williamkrick @JGamache-autodesk I will test against #2142 to see how's it going.

…is allows textures to be deleted when all materials using that texture are destroyed.
// Check the local cache again, do not overwrite if same texture has
// been loaded asynchronously
if (_textureMap.find(path) != _textureMap.end()) {
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The early exit here is wrong. It also stops marking the sprim dirty and triggering a refresh.

TfTokenVector _requiredPrimvars; //!< primvars required by this material
HdVP2ShaderUniquePtr _surfaceShader; //!< VP2 surface shader instance
SdfPath _surfaceShaderId; //!< Path of the surface shader
static HdVP2GlobalTextureMap _globalTextureMap; //!< Texture in use by all materials in MayaUSD
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With a single global static structure we'd never delete the textures until the plugin unloads or Maya exits. At that time it is often not safe to try to destroy the texture, and textures were held in the cache beyond file -new.

This way we get the deletion at the right time.

Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

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

Very good.

@williamkrick williamkrick added the ready-for-merge Development process is finished, PR is ready for merge label Mar 1, 2022
@seando-adsk seando-adsk merged commit e31a7b7 into dev Mar 1, 2022
@seando-adsk seando-adsk deleted the krickw/MAYA-121882/share_material_cache branch March 1, 2022 14:03
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 vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants