usdGeom: avoid de-referencing iterators of an empty set when validating subset families with no indices

[mattyjams]

This avoids undefined behavior when UsdGeomSubset::ValidateFamily() is called to validate a subset family that does not actually have any indices authored. In this case, we wind up with an empty set and the index bounds checking would try to de-reference iterators on it.

I think subset families with no indices would be considered invalid, so the function now deems them as so and adds a descriptive reason. If that's not appropriate, I can easily remove that part and only skip the bounds checking.

I also added newline characters to two of the existing reason messages to ensure that each reason is on its own line in the case of multiple failures.

• I have verified that all unit tests pass with the proposed changes
• I have submitted a signed Contributor License Agreement

[mattyjams]

I don't think there's any utility in a subset family with no indices, so they get marked invalid here, but I'm happy to take this out if anyone thinks otherwise.

The important part is that we avoid the de-reference of the iterators on lines 649 and 659 below.

[spiffmon]

Oh wow, @mattyjams ... in looking over the code above here to answer your question, I realized the logic here (reviewed by me!) is deeply problematic (and ValidateSubsets() above has a similar problem) in that it assumes that the faceVertexIndices property is not animated, which is a bad assumption. This method is already set up pretty well to accommodate the proper logic, though ValidateSubsets() is trickier because it is relying on the caller to pass in "the" faceCount, then doing its own iteration over indices timeSamples.

But to your question, I don't think it should necessarily be invalid to have no participating indices... it was the case of animation and varying topology that I was thinking about. It seems perfectly valid to me that you might have a Family that some of the time has Subsets that modify the base Mesh, but at some other points in time, leave it unmodified? Correctness is the first goal of validation, but alerting the user to possibly surprising/unintended things is also useful, so maybe it makes sense to consider the Family invalid if there are no points in time at which there are non-empty (valid) indices in one or more Subsets?

If you'd be willing to fix up at least this method wrt varying topology, we'd be eager to take it in, as we have a project coming up to extend the allowable elementTypes for the physics community.

Thanks alot, @mattyjams !

[mattyjams]

Thanks for the info @spiffmon!

I took a more aggressive pass at fixing up ValidateFamily() so that it properly accounts for varying topology. The original fix to avoid de-referencing iterators of an empty set is in e648a6a, and the bulk of the interesting new stuff is in 4d32257, but I broke up the commits so you can see the progression along the way.

One other issue I noticed was that for invalid subsets where all of the indices are negative, we would properly identify it as invalid but append an incorrect reason due to some integer overflow casting an int to a size_t. The fix for that is in 7bea2ff.

Let me know how this is looking now!

Thanks!

[jesschimein]

Filed as internal issue #USD-8832

[mattyjams]

Looks like this was merged as of 6575a26, and I see it was already extended in da4df84 to add support for point subsets, which is awesome.

Thanks all!