search

AnimalLogic / AL_USDMaya

This repo is no longer updated. Please see https://github.com/Autodesk/maya-usd
Other
266 stars 69 forks source link

USD dev branch removes UsdImagingGLHdEngine, breaks compilation #128

Closed murphyeoin closed 5 years ago

murphyeoin commented 5 years ago

UsdImagingGL/hdEngine.h was made private as part of a refactor, see:

Currently, AL_USDMaya creates an instance of UsdImagingGLHdEngine. We should update to using UsdImagingGL

@sirpalee thanks for the report

c64kernal commented 5 years ago

Thanks guys! I'd recommend going straight to UsdImagingGLEngine -- skip UsdImagingGL, it's about to disappear too. Let me know if you run into any trouble at all, and I'd be happy to help.

murphyeoin commented 5 years ago

Thanks George (sorry, I hadn't realised it was you!), we'll give it a try and let you know

sirpalee commented 5 years ago

@c64kernal I started updating AL_USDMaya at Luma to get some tests rolling with the latest USD dev branch. I found this commit: https://github.com/PixarAnimationStudios/USD/commit/5b226e0c57191e8fcbf198eba48e57a33c75d142 and AL_USDMaya rely on TestIntersectionBatch for selections in Maya. It doesn't look like that we have any alternatives.

c64kernal commented 5 years ago

Hi @sirpalee -- really sorry about that! It was unused code in our tree so we didn't think anyone else was using it either. The question of how to move forward is interesting. It seems like AL_USDMaya may be the only client of this code at all. I worry that simply introducing it back into the tree would result in someone else coming along and removing it, like I did.

The other option of exposing enough of the innards to let you implement it yourself (basically making the trask controller accessible) might be okay, but I might be met with some (very understandable) resistance about breaking an abstraction.

Let me talk to some more folks tomorrow and see what we can do. In the meantime, if there's anything that you can think about that we could do, please let us know! At this point, I'm going to consider this a release-blocking issue, until I hear from you that you're okay to go.

Thanks Pal, and apologies again for the trouble.

sirpalee commented 5 years ago

Hey @c64kernal, thanks for the quick reply! I don't have much to add here, I feel the same as you, exposing the internals of the UsdImagingGLEngine is not a good idea. If you are worried about the function getting removed again, I can work around this problem by subclassing UsdImagingGLEngine and copy back the function from the old version. That way I still get to access the internals without you messing up the abstraction.

c64kernal commented 5 years ago

That's a good idea, @sirpalee -- and wow, all that stuff is still protected, totally by accident :) I should at least add a comment saying that folks are depending on it being protected now, so that we don't "fix" it and make it all private.

That sounds like a good way to go for now. I'll talk to more folks tomorrow and I'll let you know if anyone else has any better ideas -- and I feel much better that you're unstuck with this proposal.

c64kernal commented 5 years ago

Hi again, @sirpalee -- I talked to the team about this, and it seems like subclassing is the way to go for this release, so that you get unstuck. However, it's not ideal because of the reasons I mentioned above -- so our proposal, is to merge the two APIs into TestIntersection. We'd take a PR for that, if you have time to make one, and then when that gets rolled out you can remove the subclass... does that sound okay with you?

sirpalee commented 5 years ago

@c64kernal sounds good! Let us get this change in al_usdmaya first, and I'll look into adding back functionality upstream for the next release.

c64kernal commented 5 years ago

Thanks very @sirpalee -- really appreciate it.