MeshPrimitive : Add subdivision options

[danieldresser-ie]

I think this is all working about right - hooking it up to Gaffer reveals some issues with my MeshTessellate code, but seems to have demonstrated that this is all working properly.

There is one spanner in the works: I've hooked up ClassData as described in the ClassData header, and I'm now getting a segfault on exit. After some debugging, I'm pretty confident that this can be explained by there not being an enforced destruction order between the static ObjectPool used for meshes loaded from files, and the static ClassData. This means that the ClassData can destruct first, resulting in erase being called on ClassData where the ClassData destructor has already removed all entries. I can prevent the crash if I early out from erase if the map is empty, but I have no idea if that's a reasonable solution - everything else in ClassData tries to assert that it is being used properly, rather than allowing any sloppiness.

I did also want to ask the question before we set these names in concrete: should the read function be getFaceVaryingLinearInterpolation instead of just faceVaryingLinearInterpolation? I was modelling the names off of interpolation / setInterpolation, but I hadn't realized initially that we're unhappy with how this naming works in the Python binding.

[johnhaddon]

I've hooked up ClassData as described in the ClassData header, and I'm now getting a segfault on exit.

We don't have to use ClassData, but it is at least useful as a way of identifying all the places we'll want to update when we can break ABI. And any alternative is going to be prone to the same problem.

I can prevent the crash if I early out from erase if the map is empty, but I have no idea if that's a reasonable solution

Sounds like undefined behaviour.

The easiest alternative fix seems to be to "leak" the classdata :

static IECore::ClassData<MeshPrimitive, MeshClassData> *g_classData = new IECore::ClassData<MeshPrimitive, MeshClassData> ;

It's not really a leak because we want it to live for the duration of the process anyway.

Should the read function be getFaceVaryingLinearInterpolation instead of just faceVaryingLinearInterpolation?

That was my expectation. Really old code like MeshPrimitive is a bit nonconformist in places, but since then we've settled pretty uniformly on matching set/get for all mutable members.

We also need to add appropriate conversions of these new properties to/from USD at a minimum, and maybe Alembic if we think it's worth it. I only skimmed the Alembic headers but they seem to have related properties that might or might not map cleanly upon closer investigation. We can merge this PR without USD support, but it'll need to come along right after for this to be of any use. I think it's reasonable to leave any potential Maya/Houdini/Nuke mesh converter updates to the team at IE...

[danieldresser-ie]

Addressed comments, added USD support.

Investigated Alembic, but the Alembic options don't map directly, and don't seem to be well documented ( it seems like Alembic just assumes you have access to old Renderman documentation to understand their parameters? But I'm not sure exactly how old it's supposed to be ... original RI spec didn't support these subdiv parameters, and more recent Renderman now uses the USD standard ). The easiest approach for now is probably to just not support this data in Alembic, since we can't map it 100% anyway.

Thought I was done, then started seeing test failures in seemingly unrelated test cases. Issue seems to be that ClassData isn't thread-safe. I guess the quickest approach will just be to add a global mutex in MeshPrimitive ... I kind of wonder whether that might start to be a noticeable performance hit in some cases, but I guess hopefully this won't exist for too long.

[johnhaddon]

Addressed comments, added USD support.

Thanks Daniel. I've comments on the new code in a couple of spots that require fixes.

The easiest approach for now is probably to just not support this data in Alembic, since we can't map it 100% anyway.

Sounds reasonable.

I guess the quickest approach will just be to add a global mutex in MeshPrimitive

Yeah, that sounds fine. I'd be extremely surprised if it had any impact on performance in practice. We should probably start prepping a new Cortex major version for Gaffer 1.5, so we can tidy up some of this stuff.

Feel free to squash everything ready for merging while you're doing the fixups...

[danieldresser-ie]

Addressed feedback, fixed crashes by adding mutex, squashed.

[johnhaddon]

LGTM. I realised this was targeted to main though, so retargeted it to RB-10.5 with the intention of merging it. But it looks like you based it off main in the first place - could you rebase to be based on RB-10.5 please?

[danieldresser-ie]

Rebased.