-
Notifications
You must be signed in to change notification settings - Fork 1.4k
NVIDIA: Change delegate to use new predicate for prototypes #3830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
NVIDIA: Change delegate to use new predicate for prototypes #3830
Conversation
|
Filed as internal issue #USD-11483 (This is an automated message. See here for more information.) |
|
Some hopefully quick questions:
|
|
Thanks for pointing that out. I'll see if I can add more test coverage.
|
|
@roggiezhang-nv , would it also make sense to update the guidance in UsdGeomPointInstancer itself for situating prototypes? After all these years, and the troubles that folks run into in trying to use |
|
@spiffmon I agree with that suggestion-- with the caveat that we should alert users that |
|
That's reasonable, @nvmkuruc , though tricky to interpret, since I don't think we want to be stamping USD versions into the core/dev documentation? Beyond time-boxing how long that warning remains in the documentation, any other things we might do, there? |
|
On Sep 30, 2025, at 12:34, F. Sebastian (spiff) Grassia ***@***.***> wrote:
spiffmon
left a comment
(PixarAnimationStudios/OpenUSD#3830)
<#3830 (comment)>
@roggiezhang-nv <https://github.com/roggiezhang-nv> , would it also make sense to update the guidance in UsdGeomPointInstancer itself for situating prototypes <https://github.com/PixarAnimationStudios/OpenUSD/blob/ecea8d840073bb0d2482730242ddf6883b843737/pxr/usd/usdGeom/schema.usda#L2108-L2121>? After all these years, and the troubles that folks run into in trying to use over as the protector, it seems like we might want to update the guidance on using class instead, while acknowledging that you can also use over ?
As a person who has been using this pattern for I think at least a decade after you pointed me to that doc, Spiff - are you saying we should *not* being doing that going forward and instead use “class”?
And do people really have trouble with this pattern? I always point folks to the doc, and show them a piece of code that implements it, and it always seems to go fine.
For example, this is how we export PointInstancers out of Blender as of 4.5, and it works swell.
… —
Reply to this email directly, view it on GitHub <#3830 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM2AOWFIVT2JMNPX5NACOL3VLLMPAVCNFSM6AAAAACHXWZWRCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGNJTGUZTCNZXGM>.
You are receiving this because you are subscribed to this thread.
|
|
Hi @drwave! I can say that we did find some friction which is why we started exploring using As far as whether or not the example should be updated, I'd be happy if we let the change land without changing the example until there's more consensus and less concerns about backwards compatibility. |
|
@drwave , the "troubles" I was thinking of come from how UsdStage's |
|
Also, just wanted to note that creating concrete, defined prototypes and protecting them by making them invisible will incur both a performance and memory penalty in Hydra and some renderers - notable Storm, because we may then process the prototypes twice, and we do extra work and caching for invisible geometry to ensure that you can switch them to visible quickly in a viewport. |
|
Yea, that’s totally fair, spiff.
And that is exactly how that code works in all the things I point folks to.
Here’s the comment I have in my OG implementation of this at the end:
// we're using a pattern from:
// https://graphics.pixar.com/usd/docs/api/class_usd_geom_point_instancer.html
// note that it is vital that we set the specifier *after* we
// have specified all our children, as we expect them to be using
// "def" with abandon. We do it here, after we have done all the
// modifications to the stage that have these component definitions.
//
// NOTE: It's vital to do this *AFTER* we calculate the extents, as
// the bounding box calculation will SKIP OVER an "over"...
And then I do:
prototypesSchema.GetPrim().SetSpecifier(pxr::SdfSpecifierOver);
So yes, more clarity on this would be great, but I agree with nvmkuruc’s previous comment
"As far as whether or not the example should be updated, I'd be happy if we let the change land without changing the example until there's more consensus and less concerns about backwards compatibility.”
… On Oct 1, 2025, at 11:41, F. Sebastian (spiff) Grassia ***@***.***> wrote:
spiffmon left a comment (PixarAnimationStudios/OpenUSD#3830)
@drwave , the "troubles" I was thinking of come from how UsdStage's DefinePrim() (which also gets used by the various schema API's) ensures that the prim you are creating is defined according to UsdPrim::IsDefined(), which requires that all of the prim's ancestors also have defining specifiers. So if you use the UsdStage API's to build your scene and create an over, then underneath it begin to define your prototypes, the ancestral over gets changed/overridden into a def, which is a gotcha and can cause the prototypes to be rendered themselves. So I imagine the Blender code needs to then go "fix" the def back to an over once done defining prototypes. That step would not be necessary if you started with a class instead of an over. The over remains a bit of a landmine for downstream editors of the scene - if anyone augments a prototype, the same thing will happen again!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@unhyperbolic Btw. You can use the following example and you'll see the sphere with this PR, but you cannot see it without it. |
8c06be4 to
408860c
Compare
|
408860c to
41889dd
Compare
Description of Change(s)
See #3829 for details as this PR is based on that to apply the new prim flags to filter prototypes.
Link to proposal (if applicable)
Fixes Issue(s)
Checklist
I have created this PR based on the dev branch
I have followed the coding conventions
I have added unit tests that exercise this functionality (Reference:
testing guidelines)
I have verified that all unit tests pass with the proposed changes
I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)