LumaPictures / usd-qt

Reusable Qt Components for Pixar's USD
Other
153 stars 41 forks source link

Fix #31: Refactor SdfAbstractDataSpecId to SdfPath as per USD 19.11 #32

Closed BigRoy closed 4 years ago

BigRoy commented 4 years ago

This refactors SdfAbstractDataSpecId and updates the SdfAbstractData interface to use SdfPath for compiling against USD 19.11. The USD 19.11 changelog states about this:

Removed SdfAbstractDataSpecId and updated the SdfAbstractData interface to use SdfPath in its place. Existing SdfAbstractData subclasses must be updated to match. See this usd-interest post for more details: https://groups.google.com/forum/#!topic/usd-interest/IVmd1t1GKBA

I've tried to refactor the code similar to the example links the Changelog links to. So hopefully everything works as expected.

I was able to compile with this change, but have not yet tested whether the tools still continue to work. I'm opening this PR already to discuss code changes and have others test it too.

Having others test it is especially useful as I've never used usd-qt before and the extent of my tests or success of using it might be totally related to other things.

BigRoy commented 4 years ago

I can confirm this works fine when compiled with USD 19.11 (I used latest USD/dev). I was able to open the outliner and hierarchyEditor editors as an example just fine. 👍 I was running them inside of Maya 2019.2 on Windows 10 but that should be irrelevant information.

Note: that by merging this it becomes incompatible with versions before USD 19.11 because of the API change.

nrusch commented 4 years ago

Thanks for this. I don't think we're quite ready to drop support for all older versions just yet, so I think we'll need to implement some conditional preprocessor switching in here. I'll play with some ideas later today or on Monday, but if you want to take a crack at it as well, feel free.

BigRoy commented 4 years ago

@nrusch I'm currently solely testing with the latest 0.19.11 so unfortunately I don't think I'll find the time to implement and test the preprocessor conditional switching for the older USD API prior to 0.19.11. However, feel free to push into this PR if you've tested against older versions and I'll be happy to recompile and see if everything is good with the latest.

nrusch commented 4 years ago

OK, I had a change of heart about the need to support older USD versions after thinking it over some more. We talked over it a bit internally, and agree that there's not much of a reason to worry about compatibility that much with this project (at least not yet), so I'll go ahead and merge this. Thanks for the contribution!

BigRoy commented 4 years ago

We talked over it a bit internally, and agree that there's not much of a reason to worry about compatibility that much with this project (at least not yet), so I'll go ahead and merge this. Thanks for the contribution!

Perfect. 🚀

nrusch commented 4 years ago

Should we branch of the current state into a 0.19.10- branch or alike for anyone looking for it? Or maybe tag a commit so it's clear where compatibility changed?

That sounds like it could potentially fit for the master branch, although I'm still a little hesitant.

We use a modified version of the USD dev branch internally, so our notion of USD "versions" is a bit different from the releases on the USD master branch, and I don't think we want to tie UsdQt's master branch to the USD cadence when it comes to tagging releases. Given that A) this project is pretty low-entropy at this stage, and B) USD is still technically not 1.0 software, I'm inclined to take a "wait-and-see" approach: If multi-version compatibility starts to become a thorn in peoples' side, then I would be happy to circle back and try to formalize things a bit more.

That said, I do think keeping the README updated with an indication of the compatibility of the various branches is a great idea. I'll get that updated shortly.