PointInstancer class#1537
Conversation
They don't mutate the view in any way.
It's worth calling out a few design decisions here... Inheritance from PointsPrimitive -------------------------------- This isn't analogous to UsdGeomPointInstancer, which isn't even a UsdGeomGprim. But it gives us strong backwards compatibility when loading from USD, and lets us use all our existing PrimitiveVariable-based algo and nodes. Query classes ------------- Most queries need to do a name-based lookup to get PrimitiveVariables. The query classes allow us to do the lookup once in the constructor, so that it isn't repeated for every point. Providing a point-based API allows client code to do threading however it wants, and to copy values directly into renderer buffers without generating intermediate arrays. The VisibilityQuery class is separate from TransformQuery because it has greater construction costs, that you might not want to pay if you don't need visibility queries. This also makes it logical for us to have additional query classes in future, which allows us to extend queries without ABI issues. No Inactive IDs, only Invisible IDs ----------------------------------- USD has both, but this choice is due to USD constraints (the former isn't animateable, the latter is). Since we have no way of preventing animation in Cortex or Gaffer, it doesn't make sense to have both in Cortex. We'll merge the two on loading from USD, and then we can write back to `invisibleIds` successfully no matter whether or not it is animated in Cortex/Gaffer. This does sacrifice round-tripping, but makes life simpler in Cortex/Gaffer. The counter-argument here might be that the distinction might be useful in a PromotePointInstances node in Gaffer. Inactive IDs wouldn't be output at all, but invisible ones could be output as scene locations with `scene:visible = false`. I don't know how useful that would be though.
Start with a copy of the input rather than an empty PointsPrimitive.
…TYPES Always load prototype paths as we want them - relative, and without a `./` prefix.
This allows us to add writing as well, since we can identify PointInstancers from regular PointsPrimitives.
Hopefully fixes linking errors on Windows.
danieldresser-ie
left a comment
danieldresser-ie
left a comment
There was a problem hiding this comment.
Looks good to me ... though I guess the hard part is how this actually integrates in Gaffer, and I haven't deeply thought through that. If you're confident in this approach, it probably makes sense to merge this first, so that the Gaffer PR will be able to build against this.
| private : | ||
|
|
||
| PrimitiveVariable::IndexedView<int64_t> m_id; | ||
| std::unordered_set<int64_t> m_invisibleIds; |
There was a problem hiding this comment.
You've probably already thought this through, and it's probably fine, but just to document my thoughts:
The current instancer gives the option for invisibleIds to be either a list of ids, or a per-vertex vector of booleans, where each boolean corresponds to one id. It probably makes sense that we don't support the latter here, since it isn't part of the USD spec. It often is the more natural thing to author in Gaffer ( for example, using an OSLObject to output a boolean for anything outside a bounding sphere ), but I guess we would just convert to this form if we supported converting an instancer like that to a PointInstancer.
| /// \todo Gaffer's Instancer class uses a `normalizedIfNeeded()` function | ||
| /// that avoids normalizing quaternions that can't get any closer to normalized. | ||
| /// Is there any value in doing that here? | ||
| result = m_orientation[pointIndex].normalized().toMatrix44() * result; |
There was a problem hiding this comment.
Just to record the context here, the comment in Instancer says:
Using Orientation::normalizedIfNeeded avoids modifying quaternions that are already
normalized. It's better for consistency to not be pointlessly changing the values
slightly at the limits of floating point precision, when they're already as close to
normalized as they can get, and this saves 4% runtime on InstancerTest.testBoundPerformance.
The performance difference isn't significant, but I guess I would still argue that the consistency is slightly better using normalizedIfNeeded. If the orientation is already normalized, it seems like a possible source of confusion to shift it around by a few floating point epsilons.
| const boost::container::flat_set<std::string> g_exportedAsAttributes = { | ||
| "prototypeRoots", "prototypeIndex", "P", "scale", "orientation", | ||
| "id", "invisibleIds" | ||
| }; |
There was a problem hiding this comment.
This feels kinda inelegant to me, though I don't immediately have any better solution.
The accessors on PointInstancer feel like they're trying to hide the choice of primitive variable names, and make it part of PointInstancer, but here we need an explicit list of prim var names. Maybe this list should be made part of PointInstancer, so that it's more obvious that this list needs to be maintained alongside the names which are hardcoded in functions like getScale()?
2 participants
This adds the foundations of a first-class PointInstancer object for Gaffer - see GafferHQ/gaffer#6810.