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

remove dependencies on old Hydra texture system from hdMaya for USD releases after 20.11 #961

Conversation

mattyjams
Copy link
Contributor

Our Hydra team is working on removing parts of the old Hydra texture system for the next core USD release.

With Luma's recent work, it looks like all of the necessary support for the newer material network-based system is already in place, so the changes here mostly just pre-process the old code away for USD releases after 20.11.

@mattyjams mattyjams added the mtoh Related to legacy Maya to Hydra plugin. label Dec 2, 2020
@mattyjams
Copy link
Contributor Author

@pmolodo: Could I bug you to have a look at this one when you get a chance? Just want to make sure we haven't missed anything here, since we don't make heavy use of mtoh ourselves.

Thanks!

Copy link
Contributor

@pmolodo pmolodo left a comment

Choose a reason for hiding this comment

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

So, I haven't looked at the texture changes coming in 21.02 yet - is it the case that texture resource handling is handled more "automatically", and none of these overrides are required at all in order to get texture support? Or are these changes effectively just disabling texture support for now (until we can add it back in a way that's compatible with 21.02, later?)

lib/usd/hdMaya/adapters/materialAdapter.cpp Show resolved Hide resolved
lib/usd/hdMaya/adapters/materialAdapter.cpp Show resolved Hide resolved
lib/usd/hdMaya/adapters/materialAdapter.h Show resolved Hide resolved
@mattyjams
Copy link
Contributor Author

So, I haven't looked at the texture changes coming in 21.02 yet - is it the case that texture resource handling is handled more "automatically", and none of these overrides are required at all in order to get texture support? Or are these changes effectively just disabling texture support for now (until we can add it back in a way that's compatible with 21.02, later?)

I think it should be the former. I don't actually expect that there will be major changes needed to add support for 21.02, but just that vestiges of the old texture system are being removed, so we want to drop code in hdMaya that used those. The "new" system I was alluding to is actually in 20.11 now, where the scene delegate constructs an HdMaterialNetworkMap and returns that from GetMaterialResource():

https://github.com/PixarAnimationStudios/USD/blob/release/pxr/imaging/hd/sceneDelegate.h#L598

It looks like all of that is already in place in hdMaya now. The actual handling of the texture resources should indeed now be handled auto-magically within Hydra itself based on the information in the material network.

So my hope is that texture support should happily continue working, but I was hoping you could help verify that. :)

@mattyjams mattyjams force-pushed the pr/hdMaya_remove_dependencies_on_old_Hydra_texture_system branch from 886ecd9 to 2e1070f Compare December 3, 2020 21:22
@pmolodo
Copy link
Contributor

pmolodo commented Dec 3, 2020

Oh - if the changes are in 20.11, then why are most of the checks for <= 20.11? Shouldn't they be < 20.11?

@mattyjams
Copy link
Contributor Author

Oh - if the changes are in 20.11, then why are most of the checks for <= 20.11? Shouldn't they be < 20.11?

Good question. I don't think there's any particular reason why the deprecated API was left in for == 20.11 other than that the Hydra team is midway through removing the old API and it made for a somewhat cleaner break. Would you prefer that I change it to < 20.11 instead?

In any case, I rebased these changes and fixed up the #if/#elif stuff you noted, so thanks again for that! One additional change I added drops out the now-unused debug codes that go along with the removed GetTextureResource() and GetTextureResourceID().

@pmolodo
Copy link
Contributor

pmolodo commented Dec 3, 2020

If there's no benefit to supporting both APIs, then I think we should drop the old one in 20.11. That will make the code read better, mean the old stuff will get dropped sooner, and also make it easier for me to test (since I already have a pre-compiled 20.11, but haven't compiled the latest tip of dev yet!)

…ersion 20.11 and later

These functions are used by the old Hydra texture system and are no longer
relevant with the new node-based material network setup in Hydra.

(Internal change: 2127507)
…USD version 20.11 and later

The HDMAYA_DELEGATE_GET_TEXTURE_RESOURCE and
HDMAYA_DELEGATE_GET_TEXTURE_RESOURCE_ID debug codes are only used by
GetTextureResource() and GetTextureResourceID(), which are removed for USD
versions 20.11 and later.
…ter for core USD version 20.11 and later

The "textureResource" light param is no longer relevant with the new Hydra
texture system.

(Internal change: 2127509)
…sion 20.11 and later

(Internal change: 2127611)
@mattyjams mattyjams force-pushed the pr/hdMaya_remove_dependencies_on_old_Hydra_texture_system branch from 2e1070f to 7b65099 Compare December 4, 2020 01:05
@mattyjams
Copy link
Contributor Author

If there's no benefit to supporting both APIs, then I think we should drop the old one in 20.11. That will make the code read better, mean the old stuff will get dropped sooner, and also make it easier for me to test (since I already have a pre-compiled 20.11, but haven't compiled the latest tip of dev yet!)

Cool, works for me! I just rebased and pushed that change.

Let me know how your testing goes! Thanks!

@pmolodo
Copy link
Contributor

pmolodo commented Dec 5, 2020

Ok, I built this and ran some basic tests, and it seems to be working fine!

Copy link
Contributor

@pmolodo pmolodo 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!

@kxl-adsk kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Dec 5, 2020
@mattyjams
Copy link
Contributor Author

@pmolodo: Great, glad to hear it! Thank you, sir!

@kxl-adsk kxl-adsk merged commit 8dff516 into Autodesk:dev Dec 5, 2020
@mattyjams mattyjams deleted the pr/hdMaya_remove_dependencies_on_old_Hydra_texture_system branch December 7, 2020 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mtoh Related to legacy Maya to Hydra plugin. 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