PointInstancer class#1537
Conversation
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.
There was a problem hiding this comment.
Yeah, for the most part I'm just aligning to the USD spec here, even if there is a more natural representation in Cortex/Gaffer. I have a suspicion that even in USD, a per-vertex bool might be the better representation when the new sparse array edits are taken into account, but USD is so pervasive that we have little choice but to align with it. And I'm not keen on having multiple representations if we can avoid it.
Part of the plan in Gaffer is to introduce a new PointInstancer node that converts a PointsPrimitive to a PointInstancer, and as you suggest, this would be a natural place to convert from array-of-bools to list-of-ids.
| /// \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.
There was a problem hiding this comment.
I'm open to reintroducing this, but I'm also pretty keen to start simple and only get more complex when forced to be.
| 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()?
There was a problem hiding this comment.
Yeah, I agree.
One thing I haven't dealt with yet is arbitrary-primvars-that-become-per-instance-attributes. When I do that we'll likely need an official mechanism for distinguishing them from the "core" primvars. I'll give this some more thought when I get there.
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.
johnhaddon
force-pushed
the
firstClassPointInstancer
branch
from
d942279 to
292e7db
Compare
|
Thanks for the review @danieldresser-ie. I've merged so I can get start getting the Gaffer-side work into PRs. |
2 participants
This adds the foundations of a first-class PointInstancer object for Gaffer - see GafferHQ/gaffer#6810.