USD: import scenegraph instances. #115076

Merged
Michael Kowalski merged 20 commits from makowalski/blender:usd-scene-instancing-import into main 2023-12-28 19:08:32 +01:00

Added support for importing USD instanceable primitives into Blender
as collection instances.

  • Added a new USDInstanceReader class for importing USD
    instances as Blender objects that instance collections containing
    prototype data.

  • Extended the USDStageReader to read USD prototype prims into
    collections that are instanced on the objects created by the instance
    readers.

  • Removed the Import Instance Proxies import option.

  • Importing instances is enabled with a new Scene Instancing import
    option, which is true by default. If this option is off, instances will be
    imported as copies (which is the functionality previously enabled by
    the Import Instance Proxies option).

  • Removed calls to UsdSkelBindingAPI::Apply() in the skeleton and
    blend shape import code, as these calls were unnecessary and were
    generating errors when importing instance prototypes with UsdSkel
    data.

  • Nested instancing and animated prototypes are supported.

Please see the USD documentation on the scenegraph instancing specifications and API.

Discussion

USD supports two approaches to instancing:

  • Point instancer primitives, which allow instancing prototypes on points in a geometry.
  • Scene graph instances, where prototype primitives can be referenced as instances in the scene.

This pull request does not handle point instancing. As per the discussion in #96747, point instancers are probably best represented by the Instance on Points geometry node, and there is already work in progress on implementing this in #113107.

In this proposal, scene graph instances are imported as Blender objects that instance prototype collections. This design was chosen as it seems to be an intuitive approach to referencing parts of the scene graph in arbitrary nodes in the hierarchy, while allowing prototype geometry to be easily accessed, edited and transformed. Moreover, in the future each prototype collection could potentially be associated with a USD file on disk (as per #68933), allowing for more control over scene composition and incremental updates of prototype assets.

However, I'm very open to modifying this design if there are better approaches I might have overlooked.

Testing

When testing this feature, be sure that the Scene Instancing
option is on. (If this toggle is off, instances will be imported as "real"
Blender objects.)

The Pixar Kitchen Set example includes a Kitchen_set_instanced.usd
file that may be used for testing.

The attached nested_instances.usda and cragInstance.usd demonstrate
nested instances and an animating prototype, respectively.

Added support for importing USD instanceable primitives into Blender as collection instances. - Added a new `USDInstanceReader` class for importing USD instances as Blender objects that instance collections containing prototype data. - Extended the `USDStageReader` to read USD prototype prims into collections that are instanced on the objects created by the instance readers. - Removed the `Import Instance Proxies` import option. - Importing instances is enabled with a new `Scene Instancing` import option, which is true by default. If this option is off, instances will be imported as copies (which is the functionality previously enabled by the `Import Instance Proxies` option). - Removed calls to `UsdSkelBindingAPI::Apply()` in the skeleton and blend shape import code, as these calls were unnecessary and were generating errors when importing instance prototypes with `UsdSkel` data. - Nested instancing and animated prototypes are supported. Please see the [USD documentation](https://openusd.org/release/api/_usd__page__scenegraph_instancing.html) on the scenegraph instancing specifications and API. ### Discussion USD supports two approaches to instancing: - [Point instancer](https://openusd.org/release/api/class_usd_geom_point_instancer.html#details) primitives, which allow instancing prototypes on points in a geometry. - [Scene graph instances](https://openusd.org/release/api/_usd__page__scenegraph_instancing.html), where prototype primitives can be referenced as instances in the scene. This pull request does not handle point instancing. As per the discussion in #96747, point instancers are probably best represented by the `Instance on Points` geometry node, and there is already work in progress on implementing this in #113107. In this proposal, scene graph instances are imported as Blender objects that instance prototype collections. This design was chosen as it seems to be an intuitive approach to referencing parts of the scene graph in arbitrary nodes in the hierarchy, while allowing prototype geometry to be easily accessed, edited and transformed. Moreover, in the future each prototype collection could potentially be associated with a USD file on disk (as per #68933), allowing for more control over scene composition and incremental updates of prototype assets. However, I'm very open to modifying this design if there are better approaches I might have overlooked. ### Testing When testing this feature, be sure that the `Scene Instancing` option is on. (If this toggle is off, instances will be imported as "real" Blender objects.) The Pixar [Kitchen Set](https://openusd.org/release/dl_kitchen_set.html) example includes a `Kitchen_set_instanced.usd` file that may be used for testing. The attached `nested_instances.usda` and `cragInstance.usd` demonstrate nested instances and an animating prototype, respectively.
Michael Kowalski added 2 commits 2023-11-18 01:48:17 +01:00
54da0841cc USD: import scene graph instances.
Initial implementation importing native USD into Blender as
collection instances.

Added a new USDInstanceReader class for importing USD instances as
Blender objects that instance collections.

Extended the USDStageReader to read USD prototype prims into
collections that are instanced on the objects created by the instance
readers.

Importing instances is enabled automatically if the
'Import Instance Proxies' import option is off, which is now
the default behavior.
buildbot/vexp-code-patch-coordinator Build done. Details
becc3d5fb0
USD scene instance import: added comments.
Also fixed formatting.
Michael Kowalski added the
Interest
USD
Interest
Pipeline, Assets & IO
labels 2023-11-18 01:48:51 +01:00
Michael Kowalski added this to the USD project 2023-11-18 01:48:59 +01:00
Michael Kowalski requested review from Bastien Montagne 2023-11-18 01:49:33 +01:00
Michael Kowalski requested review from Jesse Yurkovich 2023-11-18 01:49:46 +01:00
Michael Kowalski requested review from Matt McLin 2023-11-18 01:50:10 +01:00
Author
Member

@blender-bot build

@blender-bot build
Jesse Yurkovich reviewed 2023-11-22 11:11:58 +01:00
Jesse Yurkovich left a comment
Member

It looks like the convert_to_z_up processing isn't being triggered for the original geometry because they're not root xforms. So viewing the original "crag" geo you'll see him laying on his back in the scene. Same for the nested torus example as well. [edit] Also the import Scale adjustment is affected too and can be seen in the Kitchen Set example.

The nested example also happens to hit unfortunate collection naming collisions. The outer instances use "Instance0" as well as the inner instances etc. which leads to "Instance0.001". But the weird thing is that the ".001" suffix occurs in both lists randomly depending on the order of collection creation. I'm wondering if it would be possible to disambiguate the names somehow?

It looks like the `convert_to_z_up` processing isn't being triggered for the original geometry because they're not root xforms. So viewing the original "crag" geo you'll see him laying on his back in the scene. Same for the nested torus example as well. [edit] Also the import Scale adjustment is affected too and can be seen in the Kitchen Set example. The nested example also happens to hit unfortunate collection naming collisions. The outer instances use "Instance0" as well as the inner instances etc. which leads to "Instance0.001". But the weird thing is that the ".001" suffix occurs in both lists randomly depending on the order of collection creation. I'm wondering if it would be possible to disambiguate the names somehow?
@ -55,0 +67,4 @@
Collection *coll = BKE_collection_add(bmain, parent, name);
if (coll) {
id_fake_user_set(&coll->id);

I want to say that setting the fake user for collections isn't really necessary but will need Bastien to weigh in. I believe they'll always have a real user because there's always a 'parent' collection available.

I want to say that setting the fake user for collections isn't really necessary but will need Bastien to weigh in. I believe they'll always have a real user because there's always a 'parent' collection available.
Author
Member

I removed the call to add the fake user. Thanks for catching this.

I removed the call to add the fake user. Thanks for catching this.
makowalski marked this conversation as resolved
Author
Member

Thanks for the comments! In reply to the first issue:

It looks like the convert_to_z_up processing isn't being triggered for the original geometry because they're not root xforms. So viewing the original "crag" geo you'll see him laying on his back in the scene. Same for the nested torus example as well. [edit] Also the import Scale adjustment is affected too and can be seen in the Kitchen Set example.

This is a consequence of the fact that up-axis and scale conversions are implemented by applying global transforms on the root objects of the imported scene hierarchy, of course. But, as far as I can tell, we can't apply these same root transforms to objects in the prototype collections because then we'd get double transforms when the collection is instanced in the scene.

But perhaps there is a way to do this that I'm missing. I'm definitely open to suggestions for improving the workflow.

Thanks for the comments! In reply to the first issue: > It looks like the `convert_to_z_up` processing isn't being triggered for the original geometry because they're not root xforms. So viewing the original "crag" geo you'll see him laying on his back in the scene. Same for the nested torus example as well. [edit] Also the import Scale adjustment is affected too and can be seen in the Kitchen Set example. This is a consequence of the fact that up-axis and scale conversions are implemented by applying global transforms on the root objects of the imported scene hierarchy, of course. But, as far as I can tell, we can't apply these same root transforms to objects in the prototype collections because then we'd get double transforms when the collection is instanced in the scene. But perhaps there is a way to do this that I'm missing. I'm definitely open to suggestions for improving the workflow.
Author
Member

I'm attaching instanced_teapots.usda as an example of instancing in a Z-up scene, for comparison.

I'm attaching `instanced_teapots.usda` as an example of instancing in a Z-up scene, for comparison.
Author
Member

The nested example also happens to hit unfortunate collection naming collisions. The outer instances use "Instance0" as well as the inner instances etc. which leads to "Instance0.001". But the weird thing is that the ".001" suffix occurs in both lists randomly depending on the order of collection creation. I'm wondering if it would be possible to disambiguate the names somehow?

Unfortunately, name collision of objects imported from USD is fairly common, because we're converting from USD's hierarchical namespace (where nested prim names may be duplicated) to Blender's flat naming structure. I'm not sure how to work around this without deviating from the original names too much.

For what it's worth, I think these particular examples are probably worst-case scenarios for this issue. The instances were created in Houdini using the copy-to-points operator with the default base name "Instance" for all the instances. I think in many real world examples the base names would probably be more descriptive and varied, with fewer collisions.

> The nested example also happens to hit unfortunate collection naming collisions. The outer instances use "Instance0" as well as the inner instances etc. which leads to "Instance0.001". But the weird thing is that the ".001" suffix occurs in both lists randomly depending on the order of collection creation. I'm wondering if it would be possible to disambiguate the names somehow? > Unfortunately, name collision of objects imported from USD is fairly common, because we're converting from USD's hierarchical namespace (where nested prim names may be duplicated) to Blender's flat naming structure. I'm not sure how to work around this without deviating from the original names too much. For what it's worth, I think these particular examples are probably worst-case scenarios for this issue. The instances were created in Houdini using the copy-to-points operator with the default base name "Instance" for all the instances. I think in many real world examples the base names would probably be more descriptive and varied, with fewer collisions.

Unfortunately, name collision of objects imported from USD is fairly common, because we're converting from USD's hierarchical namespace (where nested prim names may be duplicated) to Blender's flat naming structure. I'm not sure how to work around this without deviating from the original names too much.

Yeah, I thought about using the path to the prim in the collection name but Collection names are limited to just 62 bytes and I actually couldn't find a way to fetch what I wanted out from the USD API to prototype it even.

As for scale/z-up, it looks like usdview handles it and I'd be surprised if other software didn't also? I don't immediately know where to place the transform for Blender to handle this correctly though.

> Unfortunately, name collision of objects imported from USD is fairly common, because we're converting from USD's hierarchical namespace (where nested prim names may be duplicated) to Blender's flat naming structure. I'm not sure how to work around this without deviating from the original names too much. Yeah, I thought about using the path to the prim in the collection name but Collection names are limited to just 62 bytes and I actually couldn't find a way to fetch what I wanted out from the USD API to prototype it even. As for scale/z-up, it looks like `usdview` handles it and I'd be surprised if other software didn't also? I don't immediately know where to place the transform for Blender to handle this correctly though.
Matt McLin approved these changes 2023-12-14 02:04:31 +01:00
Matt McLin left a comment
Member

Looks good to me! Just a couple minor comments to consider.

Looks good to me! Just a couple minor comments to consider.
@ -659,3 +659,3 @@
"import_instance_proxies",
true,
false,
"Import Instance Proxies",
Member

Changing this behavior to false is clearly the correct thing to do, but I think the wording of this could be confusing. The way it is currently worded seems to imply that the proxies are something that exists in the asset being imported, and by turning the option off, you are choosing to not import some part of the asset. Some users may be tempted to just enable all the check boxes to ensure they are getting everything, which would of course be silly in this case.

Consider this alternative: "Convert instances to proxies"
Or, maybe even "Convert instances to copies", as I'm not sure proxy is really the right term here either.

Then it is clear that you are choosing to avoid a conversion step rather than omitting some part of the asset.

Changing this behavior to false is clearly the correct thing to do, but I think the wording of this could be confusing. The way it is currently worded seems to imply that the proxies are something that exists in the asset being imported, and by turning the option off, you are choosing to not import some part of the asset. Some users may be tempted to just enable all the check boxes to ensure they are getting everything, which would of course be silly in this case. Consider this alternative: "Convert instances to proxies" Or, maybe even "Convert instances to copies", as I'm not sure proxy is really the right term here either. Then it is clear that you are choosing to avoid a conversion step rather than omitting some part of the asset.
Author
Member

I like Convert Instances to Copies. I agree it's much clearer. Thanks for the suggestion!

I like `Convert Instances to Copies`. I agree it's much clearer. Thanks for the suggestion!
makowalski marked this conversation as resolved
@ -342,1 +400,3 @@
collect_readers(bmain, root);
collect_readers(bmain, root, readers_);
if (!params_.import_instance_proxies) {
Member

Not so important to me, but just observation that now that scene instances are supported, this reads rather strange, and it may be a good idea to change the name and meaning of the underlying variable.

E.g.,

if (params_.support_scene_instancing) {

Not so important to me, but just observation that now that scene instances are supported, this reads rather strange, and it may be a good idea to change the name and meaning of the underlying variable. E.g., `if (params_.support_scene_instancing) {`
Author
Member

You make a good point.

If I understand your idea correctly, we'd still have just one import option related to instancing: "Convert Instances to Copies", as you suggested above.

But the params_.import_instance_proxies variable would be renamed to params_.support_scene_instancing, which would be set to true if the "Convert Instances to Copies" option is off.

Is that correct?

You make a good point. If I understand your idea correctly, we'd still have just one import option related to instancing: "Convert Instances to Copies", as you suggested above. But the `params_.import_instance_proxies` variable would be renamed to `params_.support_scene_instancing`, which would be set to `true` if the "Convert Instances to Copies" option is off. Is that correct?
Author
Member

Per discussion with Matt today, I will replace the "Convert Instances to Copies" option with a "Support Scene Instancing" option in the UI and import params.

Per discussion with Matt today, I will replace the "Convert Instances to Copies" option with a "Support Scene Instancing" option in the UI and import params.
Author
Member

I replaced the Import Instance Proxies with a new Scene Instancing option.

I replaced the `Import Instance Proxies` with a new `Scene Instancing` option.
makowalski marked this conversation as resolved
Michael Kowalski added 3 commits 2023-12-14 20:01:45 +01:00
Michael Kowalski added 1 commit 2023-12-14 20:33:05 +01:00
Michael Kowalski added 2 commits 2023-12-15 01:11:37 +01:00
28689ccde9 USD: do not set fake user on prototype collection
Per review comments, no loger setting a fake user on collections
created for prototype objects.
3395c14a92 USD: increment use count on instance collections
This prevents ID user decrement error when deleting the
instancing object.
Michael Kowalski added 2 commits 2023-12-19 03:56:52 +01:00
ad01efd513 USD: error applying SkelBindingAPI on import.
No longer calling UsdSkelBindingAPI::Apply() on import
as this is unnecessary and causes errors when attempting
to bind the API to instancing prototypes.
ca32ceef44 USD import: Scene Instancing option
Replaced "Convert Instances to Copies" option with
"Scene Instancing" option.
Michael Kowalski changed title from WIP: USD: import scenegraph instances. to USD: import scenegraph instances. 2023-12-19 16:52:46 +01:00
Author
Member

I believe I've addressed the requested changes and have removed the WIP label.

This pull request is ready for further review.

I believe I've addressed the requested changes and have removed the WIP label. This pull request is ready for further review.
Michael Kowalski added 1 commit 2023-12-19 20:19:40 +01:00
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
7ca8893fa2
Merge branch 'main' into usd-scene-instancing-import
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski added 1 commit 2023-12-24 21:18:28 +01:00
00b3b5fea3 USD import: fix prototype collection use counts.
Fixed issue where collections on instance readers in prototypes were
getting set twice.  This was causing the collection use counts to be
incremented additional times incorrectly, resulting in errors when
the collections were deleted.
Michael Kowalski added 1 commit 2023-12-26 17:58:47 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
2d7de624e1
Merge branch 'main' into usd-scene-instancing-import
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski added 2 commits 2023-12-27 02:46:06 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
99937e22f4
USD import: fix blend shape amination query.
Author
Member

@blender-bot build

@blender-bot build
Jesse Yurkovich reviewed 2023-12-27 23:53:01 +01:00
@ -0,0 +10,4 @@
#include "DNA_collection_types.h"
#include "DNA_object_types.h"
#include <iostream>

unused include

unused include
Author
Member

Thanks for spotting this!

Thanks for spotting this!
makowalski marked this conversation as resolved
Jesse Yurkovich approved these changes 2023-12-27 23:53:08 +01:00
Jesse Yurkovich left a comment
Member

This is good from my side now. Left 1 minor comment which doesn't require another review.

This is good from my side now. Left 1 minor comment which doesn't require another review.
Michael Kowalski added 2 commits 2023-12-28 02:00:45 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
a073035659
USD: remove unused include.
Author
Member

@blender-bot build

@blender-bot build
Bastien Montagne approved these changes 2023-12-28 13:12:52 +01:00
Bastien Montagne left a comment
Owner

Will trust the other reviewers for the general behavior testing.

Only did a quick code check, LGTM besides a few minor points noted below.

Once these are addressed, no extra review should be needed from me.

Will trust the other reviewers for the general behavior testing. Only did a quick code check, LGTM besides a few minor points noted below. Once these are addressed, no extra review should be needed from me.
@ -55,0 +66,4 @@
Collection *coll = BKE_collection_add(bmain, parent, name);
if (coll) {

Is that depsgraph tagging actually needed? Afaik newly created data does not exist in the depsgraph, so it is automatically fully evaluated on the next update?

In fact, I think it would be much more correct to tag the parent collection, since that one sees its hierarchy modified (and also rather use ID_RECALC_HIERARCHY).

Is that depsgraph tagging actually needed? Afaik newly created data does not exist in the depsgraph, so it is automatically fully evaluated on the next update? In fact, I think it would be much more correct to tag the _parent_ collection, since that one sees its hierarchy modified (and also rather use `ID_RECALC_HIERARCHY`).
makowalski marked this conversation as resolved
@ -55,0 +94,4 @@
instance_reader->set_instance_collection(it->second);
}
else {
std::cerr << "WARNING: Couldn't find prototype collection for " << instance_reader->prim_path()

Should use CLOG instead of 'raw' prints.

Should use `CLOG` instead of 'raw' prints.
makowalski marked this conversation as resolved
@ -479,0 +607,4 @@
pair.first);
if (it == proto_collection_map.end()) {
std::cerr << "WARNING: Couldn't find collection when adding objects for prototype "

Use CLOG instead.

Use `CLOG` instead.
makowalski marked this conversation as resolved
Michael Kowalski added 3 commits 2023-12-28 18:23:36 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
9a0c9d319a
USD: don't tag depsgraph for new collections.
Tagging the parent collection instead, per review comment.
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski merged commit ea89e11e01 into main 2023-12-28 19:08:32 +01:00
Michael Kowalski deleted branch usd-scene-instancing-import 2023-12-28 19:08:34 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#115076
No description provided.