[usdAbc] texture coordinates' topology (e.g. indices) is now preserved

[ix-dcourtois]

Description of Change(s)

This pull request fixes texture coordinates loaded through the Alembic plugin. Previously the UVs were expanded, meaning that the topology information (basically the indices) were lost, resulting in bad UVs when using subdivisions.

The pull request introduces a way to read indices from indexed Alembic properties, and use it on texture coordinates.

Fixes Issue(s)

• No issue on GitHub, but the subject was raised in the Google Groups forum: https://groups.google.com/forum/#!topic/usd-interest/ZdWkMKTHZu4

[spiffmon]

I'm not super familiar with the Alembic API, but if there's any chance that Cast() can fail, maybe test the result before dereferncing?

[spiffmon]

This is a nice chunk of code that:
a) is duplicated below
b) since UsdGeom and AbcGeom both support indexing for all primvars, we might want to apply to other primvars in the future...

Can you factor it out into a templated helper function, please?

[spiffmon]

There's a UsdGeomTokens->indices, which you can use in combination with SdfPath::JoinIdentifier

[spiffmon]

Hi Damien!
A couple of notes in the code, and also it would be really really great to add a simple unit test for this with a small abc file as input - it can be a simple baseline compare with a usda file, or a manual expectations test. Also please make sure you run your change against the existing tests in usdAbc in case it induces (desirable!) baseline diffs.

Btw, did you notice we have introduced an SdfValueTypeRole TexCoord? We're guarding the use of the new role/typeNames behind an environment variable until the reader code has been out awhile, but it's now available in 0.8.5!

Thank you,
--spiff

[jtran56]

Filed as internal issue #161427.

[ix-dcourtois]

Hi @spiffmon

Thanks for taking the time to review the code. I knew it was going to need a lot of modifications, but I didn't know where to start :)

• about the duplication of code and generic primvars: are you suggesting a _ReadGenricPrimvars generic method that I could just invoke in both _ReadPolyMesh and _ReadSubd and would take care of indices ?

• about the dereferencing, it should already be taken care of, see : https://github.com/PixarAnimationStudios/USD/blob/722956654a7090c23ac33d6e52ae93e05e38f94b/pxr/usd/plugin/usdAbc/alembicReader.cpp#L472

• Yes, I did notice the arrival of the texture coordinate role, greatly appreciated :) But I didn't notice a role API in the Alembic plugin. If it's not too complicated, I could add an optional role to the _ReadGenricPrimvars helper ?

• And for the tests, I'll add one. It's just that I'm building USD against our libs, and I do not have Python support enabled (we're using a static boost build, which causes a lot of problems on Windows with Python)

Thanks again for the feedback (not sure how long it will take to fix the aforementioned problems, I do not have much time allocated for this issue currently, but I'll do my best !)

[spiffmon]

- about the duplication of code and generic primvars: are you suggesting a _ReadGenricPrimvars generic method that I could just invoke in both _ReadPolyMesh and _ReadSubd and would take care of indices ? ​Yes, or even a method on the context, like context->AddPrimvar(), which

would always check for indexing?​

- - about the dereferencing, it should already be taken care of, see : https://github.com/PixarAnimationStudios/USD/blob/ 7229566/pxr/usd/plugin/ usdAbc/alembicReader.cpp#L472 <https://github.com/PixarAnimationStudios/USD/blob/722956654a7090c23ac33d6e52ae93e05e38f94b/pxr/usd/plugin/usdAbc/alembicReader.cpp#L472> ​Ah, yes - thanks for pointing that out!​ - - Yes, I did notice the arrival of the texture coordinate role, greatly appreciated :) But I didn't notice a role API in the Alembic plugin. If it's not too complicated, I could add an optional role to the _ReadGenricPrimvars helper ? ​It's taken care of by _GetUVTypeName()​ - - And for the tests, I'll add one. It's just that I'm building USD against our libs, and I do not have Python support enabled (we're using a static boost build, which causes a lot of problems on Windows with Python) ​Got it. As long as you can generate the USD file corresponding to your

abc test asset, you can just follow the pattern of testUsdAbcInstancing, which does a baseline comparison, and we can make any corrections needed to ensure the test runs properly. ​ ​--spiff​

[ix-dcourtois]

Hi @spiffmon

I updated my pull request. I went the _ReadProperty way because it seemed to follow more closely the existing code (I tried creating a AddPrimvar method to the reader context, but it was forcing me to move code around a lot to be able to check if the Alembic property was indexed, and also the code was a lot different than the other Add* methods)

But if you feel this should be the way to go, let me know, I'll modify it.

I also added the test, and I could successfully run them (I had to switch to Linux)
It's named testUsdAbcIndexedProperties because even though for the moment only the uv's are using this, in the future user Alembic properties might also use this, and more tests might be needed

Also, if you prefer the commits to be squashed, I can do that too.

[spiffmon]

Looks awesome, @ix-dcourtois - thanks much! We'll get this in the PR queue!