USD PointInstancer import support #113107

Merged
Michael Kowalski merged 29 commits from expenses/blender:usd-read-pointinstancer-2 into main 2024-02-01 15:37:54 +01:00
Contributor

See #96747. Takes over from #112777. As instancing isn't supported on the main branch yet, I think I've made as much progress as I can with the nodes stuff. I'm not sure whether I should remove the node code or just leave it as-is.

Added support for static point instancing import.

Added a new USDPointInstancerReader class to import UsdGeomPointInstancer
primitives as Point Clouds with geometry node modifiers that use
Instance on Points geometry nodes to instance collections containing
prototype geometry.

Added logic to the USDStageReader class to traverse the USD stage to
create readers for point instancer prototypes.

Extended USDStageReader::collect_readers() to take arguments to include
undefined prims and to exclude a set of specified prims when traversing
the stage. Allowing traversing undefined prims is necessary because
prototype prims may be specfied as "overs" in the USD stage.

Added a USDPrimReader::is_in_instancer_proto_ boolean flag which
identifies readers of prims that are in point instancer prototypes.
The flag simplifies management of collections and is used to
determine whether global transforms should be applied to root objects.

Limitation: point cloud animation is not yet supported. Animation can be
added once #115623 is
merged.

This partially addresses #96747.
(That issue requires point cloud animation support to be fully addressed.)

See #96747. Takes over from https://projects.blender.org/blender/blender/pulls/112777. As instancing isn't supported on the `main` branch yet, I think I've made as much progress as I can with the nodes stuff. I'm not sure whether I should remove the node code or just leave it as-is. Added support for static point instancing import. Added a new USDPointInstancerReader class to import UsdGeomPointInstancer primitives as Point Clouds with geometry node modifiers that use Instance on Points geometry nodes to instance collections containing prototype geometry. Added logic to the USDStageReader class to traverse the USD stage to create readers for point instancer prototypes. Extended USDStageReader::collect_readers() to take arguments to include undefined prims and to exclude a set of specified prims when traversing the stage. Allowing traversing undefined prims is necessary because prototype prims may be specfied as "overs" in the USD stage. Added a USDPrimReader::is_in_instancer_proto_ boolean flag which identifies readers of prims that are in point instancer prototypes. The flag simplifies management of collections and is used to determine whether global transforms should be applied to root objects. Limitation: point cloud animation is not yet supported. Animation can be added once https://projects.blender.org/blender/blender/pulls/115623 is merged. This partially addresses https://projects.blender.org/blender/blender/issues/96747. (That issue requires point cloud animation support to be fully addressed.)
Ashley added 1 commit 2023-10-01 09:16:47 +02:00
Brecht Van Lommel reviewed 2023-10-06 17:44:26 +02:00
@ -0,0 +78,4 @@
positions_span[i] = float3(positions[i][0], positions[i][1], positions[i][2]);
}
auto scales_attribute = point_cloud->attributes_for_write().lookup_or_add_for_write_only_span<float3>("scales", ATTR_DOMAIN_POINT);

The orientations and scales are not required properties, so it should skip those if they don't exist.

The orientations and scales are not required properties, so it should skip those if they don't exist.
Author
Contributor

So I think the attributes need to be created even if the properties aren't set in the usd file as they are used in the geometry node setup.

So I think the attributes need to be created even if the properties aren't set in the usd file as they are used in the geometry node setup.

So if I'm understanding correctly, this requires code from the usd branch that would allow mapping of proto paths to Object* and making a collection out of them?

I guess either a PR could be made to bring the entire instancing support to main, or just adding the (maybe small?) part of it that makes this mapping possible to this PR.

So if I'm understanding correctly, this requires code from the usd branch that would allow mapping of proto paths to `Object*` and making a collection out of them? I guess either a PR could be made to bring the entire instancing support to main, or just adding the (maybe small?) part of it that makes this mapping possible to this PR.
Michael Kowalski added the
Interest
Pipeline, Assets & IO
Interest
USD
labels 2023-10-06 21:19:27 +02:00
Michael Kowalski added this to the USD project 2023-10-06 21:19:38 +02:00
Author
Contributor

@brecht For full support, yes we'd need that. I'm also happy to just merge the point-cloud creation code and leave that to a further PR though.

@brecht For full support, yes we'd need that. I'm also happy to just merge the point-cloud creation code and leave that to a further PR though.

I can see how at least importing the pointcloud is more useful than doing nothing.

Since @makowalski has the other instancing supported planned as a target for 4.1, I'll let him decide what the most convenient way to land this is.

I can see how at least importing the pointcloud is more useful than doing nothing. Since @makowalski has the other instancing supported planned as a target for 4.1, I'll let him decide what the most convenient way to land this is.
Michael Kowalski requested review from Michael Kowalski 2023-10-12 15:55:05 +02:00
Michael Kowalski requested review from Charles Wardlaw 2023-10-12 15:55:20 +02:00

Hi there,

The code for the patch is looking great! Unfortunately, the current patch causes segfaults on the example file provided in #96747. Near as I can tell, what's happening is that you're requesting point positions at time 0 (usd default time), getting no points, then making assumptions later in the code that require at least one point to be present. In my case, it's crashing on usd_reader_pointinstancer.cc line 100 because while you have plenty of data in from the USD proto_instances array, the span for the Blender-side attribute write depends on points being available and as such is getting created empty.

If you could add some more checks around size assumptions and post back that'd be great. I can continue testing. Please do test against the original provided example file as it gives you a great broken edge case right out the gate. :)

Incidentally, my recommendation is that you should load data from the first time sample when animated, but you do also need to account for times when zero points are present due to simulation run-up, etc.

Hi there, The code for the patch is looking great! Unfortunately, the current patch causes segfaults on the example file provided in #96747. Near as I can tell, what's happening is that you're requesting point positions at time 0 (usd default time), getting no points, then making assumptions later in the code that require at least one point to be present. In my case, it's crashing on usd_reader_pointinstancer.cc line 100 because while you have plenty of data in from the USD proto_instances array, the span for the Blender-side attribute write depends on points being available and as such is getting created empty. If you could add some more checks around size assumptions and post back that'd be great. I can continue testing. Please do test against the original provided example file as it gives you a great broken edge case right out the gate. :) Incidentally, my recommendation is that you should load data from the first time sample when animated, but you do also need to account for times when zero points are present due to simulation run-up, etc.
Author
Contributor

@CharlesWardlaw cool, thanks for the feedback! I'll implement those changes soon. I've mostly been testing on PointInstancers that I exported from geometry nodes via a script: 5243e2b989/instances_as_points.py. I've uploaded a file as an example. I'll start testing on the sample provided as well.

@CharlesWardlaw cool, thanks for the feedback! I'll implement those changes soon. I've mostly been testing on PointInstancers that I exported from geometry nodes via a script: https://github.com/expenses/usd-mtx-scripts/blob/5243e2b989ee4ffddd9078b20affee9736ca8298/instances_as_points.py. I've uploaded a file as an example. I'll start testing on the sample provided as well.
Ashley added 1 commit 2023-10-20 02:06:25 +02:00
Author
Contributor

I've pushed a new commit that bounds the iterations using the size of the positions with std::min(positions.size(), scales.size()) etc. This seemed like the most flexible way of doing things. The sample file loads fine now. To get the first time sample to display I had to fetch the time samples array of the positions and use the first time if provided. I'm not sure if it's possible for point clouds in blender to be time sampled? Should we be using ComputeInstanceTransformsAtTime?

I also commented out the geometry node setup as it prevents points from being rendered by-default.

I've pushed a new commit that bounds the iterations using the size of the positions with `std::min(positions.size(), scales.size())` etc. This seemed like the most flexible way of doing things. The sample file loads fine now. To get the first time sample to display I had to fetch the time samples array of the positions and use the first time if provided. I'm not sure if it's possible for point clouds in blender to be time sampled? Should we be using [`ComputeInstanceTransformsAtTime`](https://openusd.org/release/api/class_usd_geom_point_instancer.html#a619b5f21f6c0fc26e6bd25c86c82b390)? I also commented out the geometry node setup as it prevents points from being rendered by-default.
Ashley added 2 commits 2023-10-20 03:14:04 +02:00
Ashley added 1 commit 2023-10-20 04:59:24 +02:00
Author
Contributor

I've pushed a new commit that bounds the iterations using the size of the positions with std::min(positions.size(), scales.size()) etc. This seemed like the most flexible way of doing things.

I've shifted on this and am using default values when they're not specified.

> I've pushed a new commit that bounds the iterations using the size of the positions with `std::min(positions.size(), scales.size())` etc. This seemed like the most flexible way of doing things. I've shifted on this and am using default values when they're not specified.

Better! I'm getting the proper look for that first frame now. 👍

It looks like because you're subclassing USDXformReader instead of USDGeomReader, you're missing out on the animation streamer function in usd_reader_geom.cc:

void USDGeomReader::add_cache_modifier()
{
  ModifierData *md = BKE_modifier_new(eModifierType_MeshSequenceCache);
  BLI_addtail(&object_->modifiers, md);

  MeshSeqCacheModifierData *mcmd = reinterpret_cast<MeshSeqCacheModifierData *>(md);

  mcmd->cache_file = settings_->cache_file;
  id_us_plus(&mcmd->cache_file->id);
  mcmd->read_flag = import_params_.mesh_read_flag;

  STRNCPY(mcmd->object_path, prim_.GetPath().GetString().c_str());
}

Unfortunately, I don't see anything for a sequence cache of any kind for the point cloud object. I think the current modifier on Meshes could be modified to work on point clouds, and then you'd have a similar setup where the points and their values change over time.

@brecht Is there a cache modifier already in the works for point clouds?

Otherwise, I think you could move on to uncommenting your node graph and sorting that side of things out. I made a quick one and it looks good:

image

Better! I'm getting the proper look for that first frame now. 👍 It looks like because you're subclassing USDXformReader instead of USDGeomReader, you're missing out on the animation streamer function in usd_reader_geom.cc: ``` void USDGeomReader::add_cache_modifier() { ModifierData *md = BKE_modifier_new(eModifierType_MeshSequenceCache); BLI_addtail(&object_->modifiers, md); MeshSeqCacheModifierData *mcmd = reinterpret_cast<MeshSeqCacheModifierData *>(md); mcmd->cache_file = settings_->cache_file; id_us_plus(&mcmd->cache_file->id); mcmd->read_flag = import_params_.mesh_read_flag; STRNCPY(mcmd->object_path, prim_.GetPath().GetString().c_str()); } ``` Unfortunately, I don't see anything for a sequence cache of any kind for the point cloud object. I think the current modifier on Meshes could be modified to work on point clouds, and then you'd have a similar setup where the points and their values change over time. @brecht Is there a cache modifier already in the works for point clouds? Otherwise, I think you could move on to uncommenting your node graph and sorting that side of things out. I made a quick one and it looks good: ![image](/attachments/f247f7a7-6140-4fe5-94a5-121b48905bd1)
394 KiB

Also @brecht Is there a benefit to using the PointCloud object here instead of a Mesh? The mesh would get automatic point animation support, and the point cloud attributes could be pushed as generic Attributes.

Also @brecht Is there a benefit to using the PointCloud object here instead of a Mesh? The mesh would get automatic point animation support, and the point cloud attributes could be pushed as generic Attributes.

@expenses @CharlesWardlaw I haven't had a chance to look at this PR yet, but please be aware there is already work in progress on Point Cloud import:

#106398

See also the work by @kevindietrich here:

https://projects.blender.org/kevindietrich/blender/src/branch/abc_usd_geometry_sets

See the comment here:

#106398 (comment)

@CharlesWardlaw Kevin's abc_usd_geometry_sets branch has a modifier that will work on point clouds.

@expenses @CharlesWardlaw I haven't had a chance to look at this PR yet, but please be aware there is already work in progress on Point Cloud import: https://projects.blender.org/blender/blender/issues/106398 See also the work by @kevindietrich here: https://projects.blender.org/kevindietrich/blender/src/branch/abc_usd_geometry_sets See the comment here: https://projects.blender.org/blender/blender/issues/106398#issuecomment-940313 @CharlesWardlaw Kevin's abc_usd_geometry_sets branch has a modifier that will work on point clouds.
Ashley added 1 commit 2023-10-22 03:24:23 +02:00
Author
Contributor

Also @brecht Is there a benefit to using the PointCloud object here instead of a Mesh? The mesh would get automatic point animation support, and the point cloud attributes could be pushed as generic Attributes.

I've tested using a mesh object in this branch: https://projects.blender.org/expenses/blender/compare/usd-read-pointinstancer-2...expenses/blender:usd-read-pointinstancer-mesh. It seems like it could be the way to go if point clouds aren't cutting it.

> Also @brecht Is there a benefit to using the PointCloud object here instead of a Mesh? The mesh would get automatic point animation support, and the point cloud attributes could be pushed as generic Attributes. I've tested using a mesh object in this branch: https://projects.blender.org/expenses/blender/compare/usd-read-pointinstancer-2...expenses/blender:usd-read-pointinstancer-mesh. It seems like it could be the way to go if point clouds aren't cutting it.

It would be great to finally get that geometry sets branch merged, so we can properly import hair and point clouds. Any help with that would be appreciated.

I think it would be better to use a point cloud than a mesh if we can, it's just a better fit without unnecessary data and better visualization. But the same node graph probably works with both meshes and point clouds, so it seems reasonable to go that way first if the geometry sets are not done soon.

It would be great to finally get that geometry sets branch merged, so we can properly import hair and point clouds. Any help with that would be appreciated. I think it would be better to use a point cloud than a mesh if we can, it's just a better fit without unnecessary data and better visualization. But the same node graph probably works with both meshes and point clouds, so it seems reasonable to go that way first if the geometry sets are not done soon.
Michael Kowalski reviewed 2023-11-16 02:44:44 +01:00
@ -0,0 +153,4 @@
collection_info_node->locx = 100.0f;
collection_info_node->locy = 100.0f;
bool seperate_children = true;
nodeFindSocket(collection_info_node, SOCK_IN, "Separate Children")->default_value = (void*)&seperate_children;

FYI, I uncommented the node creation code for testing, and it looks great!

However this line was causing a crash. For one thing, this is assigning a pointer to the local variable separate_children, which goes out of scope at the end of the function.

More importantly, I believe you need to perform an additional cast of bNodeSocket::default_value so you can assign the bool to bNodeSocketValueBoolean::value:

bNodeSocket *separate_children_sock = nodeFindSocket(
    collection_info_node, SOCK_IN, "Separate Children");
((bNodeSocketValueBoolean *)separate_children_sock->default_value)->value = true;

I realize this code was commented out, so this might not have been intended for review, but I just wanted to mention this in case you weren't aware of the issue.

FYI, I uncommented the node creation code for testing, and it looks great! However this line was causing a crash. For one thing, this is assigning a pointer to the local variable `separate_children`, which goes out of scope at the end of the function. More importantly, I believe you need to perform an additional cast of `bNodeSocket::default_value` so you can assign the bool to `bNodeSocketValueBoolean::value`: ``` bNodeSocket *separate_children_sock = nodeFindSocket( collection_info_node, SOCK_IN, "Separate Children"); ((bNodeSocketValueBoolean *)separate_children_sock->default_value)->value = true; ``` I realize this code was commented out, so this might not have been intended for review, but I just wanted to mention this in case you weren't aware of the issue.
makowalski marked this conversation as resolved

I meant to say earlier, this is very nice work!

@expenses I'm aiming to submit a pull request for scene graph instancing this week, which will have the logic for creating collections for prototypes. I think that logic can be reused and extended to work with your point instancer code. I'll let you know when I've posted the code and we can discuss further.

I meant to say earlier, this is very nice work! @expenses I'm aiming to submit a pull request for scene graph instancing this week, which will have the logic for creating collections for prototypes. I think that logic can be reused and extended to work with your point instancer code. I'll let you know when I've posted the code and we can discuss further.
Author
Contributor

@makowalski Okay awesome :)

I do want to say that I'm rather confused about how best to write the node-generating code, and while I do have time to spare for this PR I don't think I have enough to fully dive into that system. If anyone wants to take over just to fix up that code I'd be more than grateful.

@makowalski Okay awesome :) I do want to say that I'm rather confused about how best to write the node-generating code, and while I do have time to spare for this PR I don't think I have enough to fully dive into that system. If anyone wants to take over just to fix up that code I'd be more than grateful.

I do want to say that I'm rather confused about how best to write the node-generating code, and while I do have time to spare for this PR I don't think I have enough to fully dive into that system. If anyone wants to take over just to fix up that code I'd be more than grateful.

@expenses I'd be happy to help with this. I think you're most of the way there with the node generation. As a first step, I can add the logic for creating prototype collections, since this would be similar to what I already implemented for scene graph instancing. I can then look into extending the node creation code you already have to finish setting up the Collection Info node and the mapping of prototype indices. Would that be okay?

> I do want to say that I'm rather confused about how best to write the node-generating code, and while I do have time to spare for this PR I don't think I have enough to fully dive into that system. If anyone wants to take over just to fix up that code I'd be more than grateful. @expenses I'd be happy to help with this. I think you're most of the way there with the node generation. As a first step, I can add the logic for creating prototype collections, since this would be similar to what I already implemented for scene graph instancing. I can then look into extending the node creation code you already have to finish setting up the Collection Info node and the mapping of prototype indices. Would that be okay?

@expenses Just FYI, I added #115076 for scenegraph instancing import.

I'm working on adding the collection support for the pointinstancer import. I'll create a branch with my changes that you can merge into your pull request, if you find it useful.

@expenses Just FYI, I added https://projects.blender.org/blender/blender/pulls/115076 for scenegraph instancing import. I'm working on adding the collection support for the pointinstancer import. I'll create a branch with my changes that you can merge into your pull request, if you find it useful.
Ashley added 5 commits 2023-12-02 10:22:27 +01:00
Author
Contributor

Would that be okay?

Hey @makowalski that sounds great, sorry for the late reply.

> Would that be okay? Hey @makowalski that sounds great, sorry for the late reply.

As an update, I was pulled away for a while working on other pull requests, but I'm back to working on this now. I'm sorry about the delay.

As an update, I was pulled away for a while working on other pull requests, but I'm back to working on this now. I'm sorry about the delay.

@expenses I think I got the collections and nodes working. Here is my branch:

https://projects.blender.org/makowalski/blender/src/branch/usd-read-pointinstancer-2

with the following updates:

  • Added usd_reader_pointinstancer.h to CMakeLists.
  • Creating readers for point instancer prototypes.
  • Creating collections containing point instancer prototype objects.
  • Finished setup of the point cloud geometry nodes modifier node tree, as follows: Creating Named Attribute nodes for reading the "orientations", "scales" and "proto_indices" point attributes; setting the required flags on the Collection Info and Instance On Points nodes; setting the prototype collection object on the Collection Info node.
  • Added a USDPrimReader::is_in_instancer_proto_ boolean flag which identifies readers of prims that are in point instancer prototypes. The flag simplifies management of collections and is used to determine whether global transforms should be applied to root objects.
  • Fixed formatting.

This is what the node tree looks like:

image

Here is the OpenUSD City Set example:

image

Please let me know if you'd like to merge these changes in yourself, or if you'd like me to push them to your branch directly. Either way is fine with me.

@expenses I think I got the collections and nodes working. Here is my branch: https://projects.blender.org/makowalski/blender/src/branch/usd-read-pointinstancer-2 with the following updates: - Added usd_reader_pointinstancer.h to CMakeLists. - Creating readers for point instancer prototypes. - Creating collections containing point instancer prototype objects. - Finished setup of the point cloud geometry nodes modifier node tree, as follows: Creating Named Attribute nodes for reading the "orientations", "scales" and "proto_indices" point attributes; setting the required flags on the Collection Info and Instance On Points nodes; setting the prototype collection object on the Collection Info node. - Added a USDPrimReader::is_in_instancer_proto_ boolean flag which identifies readers of prims that are in point instancer prototypes. The flag simplifies management of collections and is used to determine whether global transforms should be applied to root objects. - Fixed formatting. This is what the node tree looks like: ![image](/attachments/18d685e0-5eb1-4cf0-8af1-100ed8fc9207) Here is the OpenUSD [City Set](https://openusd.org/release/dl_city_set.html) example: ![image](/attachments/1c8e7ff1-4c39-4e77-8864-f752df0fb7b5) Please let me know if you'd like to merge these changes in yourself, or if you'd like me to push them to your branch directly. Either way is fine with me.

The above changes allow reading the nested point instancing example provided by @Matt-McLin:

image

The above changes allow reading the nested point instancing example provided by @Matt-McLin: ![image](/attachments/0f480f1f-e761-4590-9a8a-0602406becd5)
748 KiB
Ashley added 6 commits 2024-01-19 05:22:02 +01:00
Added usd_reader_pointinstancer.h to CMakeLists.

Creating readers for point instancer prototypes.

Creating collections containing point instancer prototype objects.

Finished setup of the point cloud geometry nodes modifier node tree,
as follows: Creating Named Attribute nodes for reading the
"orientations", "scales" and "proto_indices" point attributes;
setting the required flags on the Collection Info and Instance On
Points nodes; setting the prototype collection object on the
Collection Info node.

Added a USDPrimReader::is_in_instancer_proto_ boolean flag which
identifies readers of prims that are in point instancer prototypes.
The flag simplifies management of collections and is used to
determine whether global transforms should be applied to root objects.

Fixed formatting.
Merge branch 'makowalski-usd-read-pointinstancer-2' into usd-read-pointinstancer-2
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
39d134824f
Hans Goudey reviewed 2024-01-19 14:30:17 +01:00
Hans Goudey left a comment
Member

I have no knowledge of the USD side of this, but the geometry nodes side of things looks reasonable. I just left some comments when reading through.

Usually attribute names in Blender (at least builtin attributes) have singular names. For example "position" and "scale" instead of "scales" Not sure how important these names are, but maybe that would be nice here too.

I have no knowledge of the USD side of this, but the geometry nodes side of things looks reasonable. I just left some comments when reading through. Usually attribute names in Blender (at least builtin attributes) have singular names. For example "position" and "scale" instead of "scales" Not sure how important these names are, but maybe that would be nice here too.
@ -0,0 +38,4 @@
/**
* Create a node to read a geometry attribute of the given name and type.
*/
static bNode *add_input_named_attrib_node(bNodeTree *ntree, const char *name, int8_t prop_type)
Member

Might as well put this in the blender::io::usd namespace too. I'd suggest a few tweaks too, just for general cleanup.

static bNode *add_input_named_attrib_node(bNodeTree *ntree, const StringRef name, int8_t prop_type)
{
  bNode *node = nodeAddStaticNode(NULL, ntree, GEO_NODE_INPUT_NAMED_ATTRIBUTE);
  auto *storage = reinterpret_cast<NodeGeometryInputNamedAttribute *>(node->storage);
  storage->data_type = prop_type;

  bNodeSocket *socket = nodeFindSocket(node, SOCK_IN, "Name");
  bNodeSocketValueString *str_value = static_cast<bNodeSocketValueString *>(socket->default_value);
  BLI_strncpy(str_value->value, name.data(), name.size());
  return node;
}

I thought the asserts weren't really necessary since the pointers are de-referenced on the following line, which will generally give the same effect and show the same thing to the reader. Also, auto can be used here since the type name is already written in the cast. It's nice in that case when it doesn't wrap to two lines.

Might as well put this in the `blender::io::usd` namespace too. I'd suggest a few tweaks too, just for general cleanup. ```cpp static bNode *add_input_named_attrib_node(bNodeTree *ntree, const StringRef name, int8_t prop_type) { bNode *node = nodeAddStaticNode(NULL, ntree, GEO_NODE_INPUT_NAMED_ATTRIBUTE); auto *storage = reinterpret_cast<NodeGeometryInputNamedAttribute *>(node->storage); storage->data_type = prop_type; bNodeSocket *socket = nodeFindSocket(node, SOCK_IN, "Name"); bNodeSocketValueString *str_value = static_cast<bNodeSocketValueString *>(socket->default_value); BLI_strncpy(str_value->value, name.data(), name.size()); return node; } ``` I thought the asserts weren't really necessary since the pointers are de-referenced on the following line, which will generally give the same effect and show the same thing to the reader. Also, `auto` can be used here since the type name is already written in the cast. It's nice in that case when it doesn't wrap to two lines.
makowalski marked this conversation as resolved
@ -0,0 +107,4 @@
PointCloud *point_cloud = BKE_pointcloud_new_nomain(positions.size());
auto positions_span = point_cloud->positions_for_write();
Member

auto -> MutableSpan<float3>. Same with auto elsewhere here. The type names are generally helpful to the reader, might as well include them.

`auto` -> `MutableSpan<float3>`. Same with `auto` elsewhere here. The type names are generally helpful to the reader, might as well include them.
makowalski marked this conversation as resolved
@ -0,0 +114,4 @@
}
auto scales_attribute =
point_cloud->attributes_for_write().lookup_or_add_for_write_only_span<float3>(
Member

A temporary variable would make this look a bit nicer.

bke::MutableAttributeAccessor attributes = point_cloud->attributes_for_write();
A temporary variable would make this look a bit nicer. ``` bke::MutableAttributeAccessor attributes = point_cloud->attributes_for_write(); ```
makowalski marked this conversation as resolved
@ -0,0 +117,4 @@
point_cloud->attributes_for_write().lookup_or_add_for_write_only_span<float3>(
"scales", bke::AttrDomain::Point);
for (int i = 0; i < positions.size(); i++) {
Member

The check can be removed from inside the loop

for (const int i : IndexRange(std::min(scales.size(), positions.size())) {

The check can be removed from inside the loop `for (const int i : IndexRange(std::min(scales.size(), positions.size())) {`
makowalski marked this conversation as resolved
@ -0,0 +140,4 @@
orientations[i].GetImaginary()[2]);
}
else {
orientations_attribute.span[i] = math::Quaternion();
Member

math::Quaternion::identity() I think

`math::Quaternion::identity()` I think
makowalski marked this conversation as resolved
@ -0,0 +161,4 @@
ModifierData *md = BKE_modifier_new(eModifierType_Nodes);
BLI_addtail(&object_->modifiers, md);
NodesModifierData &nmd = *reinterpret_cast<NodesModifierData *>(md);
nmd.node_group = ntreeAddTree(NULL, "Instances", "GeometryNodeTree");
Member

NULL -> nullptr

`NULL` -> `nullptr`
makowalski marked this conversation as resolved
@ -0,0 +191,4 @@
indices_attrib_node->locx = 100.0f;
indices_attrib_node->locy = -300.0f;
bNode *rotation_to_euler_node = nodeAddStaticNode(NULL, ntree, FN_NODE_ROTATION_TO_EULER);
Member

The rotation to Euler node should be unnecessary here.

The rotation to Euler node should be unnecessary here.
makowalski marked this conversation as resolved
@ -0,0 +268,4 @@
void USDPointInstancerReader::set_collection(Main *bmain, Collection *coll)
{
if (!object_) {
Member

Collection *coll can become a reference, then the assert can be removed. Also not sure here if !object_ is an overly defensive check

`Collection *coll` can become a reference, then the assert can be removed. Also not sure here if `!object_` is an overly defensive check
makowalski marked this conversation as resolved
@ -0,0 +274,4 @@
BLI_assert(coll);
ModifierData *mod_data = BKE_modifiers_findby_type(this->object_, eModifierType_Nodes);
Member

Typical name for mod_data is just md

Typical name for `mod_data` is just `md`
makowalski marked this conversation as resolved
@ -0,0 +281,4 @@
}
NodesModifierData *nmd = reinterpret_cast<NodesModifierData *>(mod_data);
if (!nmd) {
Member

NodesModifierData will not be null if ModifierData isn't null, and that was already checked.

`NodesModifierData` will not be null if `ModifierData` isn't null, and that was already checked.
makowalski marked this conversation as resolved
@ -0,0 +304,4 @@
return;
}
bNodeSocketValueCollection *socket_data = (bNodeSocketValueCollection *)sock->default_value;
Member

static_cast here

`static_cast` here
makowalski marked this conversation as resolved
@ -0,0 +1,57 @@
/*
Member

Looks like this can use the new copyright and license format

Looks like this can use the new copyright and license format
makowalski marked this conversation as resolved
@ -54,10 +56,25 @@
#include "WM_api.hh"
#include <iostream>
Member

Not clear that this include is used (sorry if I'm just missing it)

Not clear that this include is used (sorry if I'm just missing it)
makowalski marked this conversation as resolved
@ -618,0 +706,4 @@
continue;
}
blender::Vector<USDPrimReader *> proto_readers;
Member

blender::Vector -> Vector

`blender::Vector` -> `Vector`
makowalski marked this conversation as resolved

@blender-bot build

@blender-bot build

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113107) when ready.

@wave If you'd like to test this functionality, builds for this pull request are available here:

https://builder.blender.org/download/patch/PR113107/

@wave If you'd like to test this functionality, builds for this pull request are available here: https://builder.blender.org/download/patch/PR113107/
Michael Kowalski added 1 commit 2024-01-22 17:39:45 +01:00
Hans Goudey approved these changes 2024-01-22 17:53:36 +01:00
@ -0,0 +163,4 @@
bNode *instance_on_points_node = nodeAddStaticNode(nullptr, ntree, GEO_NODE_INSTANCE_ON_POINTS);
instance_on_points_node->locx = 300.0f;
bNodeSocket *socket = nodeFindSocket(instance_on_points_node, SOCK_IN, "Pick Instance");
((bNodeSocketValueBoolean *)socket->default_value)->value = true;
Member

This can use socket->default_value_typed<bNodeSocketValueBoolean>()->value = true; to avoid the C style cast

This can use `socket->default_value_typed<bNodeSocketValueBoolean>()->value = true;` to avoid the C style cast
makowalski marked this conversation as resolved

I addressed the review comments, I believe. However, @wave pointed out that the logic ignores invisibleIds in the USD, and I'm currently working to address this. I hope to push these additional changes in the next day or two.

I addressed the review comments, I believe. However, @wave pointed out that the logic ignores `invisibleIds` in the USD, and I'm currently working to address this. I hope to push these additional changes in the next day or two.

Here's a simple example that demonstrates a point instancer that has invisibleIds. Unzip the file and load AHH.usda to see it. If you have macOS, you should be able to double click on it to see it. I've included a screenshot of the expected look:

Here's a simple example that demonstrates a point instancer that has invisibleIds. Unzip the file and load AHH.usda to see it. If you have macOS, you should be able to double click on it to see it. I've included a screenshot of the expected look:
Michael Kowalski added 2 commits 2024-01-23 19:32:39 +01:00
Now reading the point instancer boolean mask values into
a "mask" point attribute which is set as the Selection
input of the Instance on Points node.
Michael Kowalski added 2 commits 2024-01-24 00:20:55 +01:00
USD: bug fixes
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
95b015984e
Now calling SpanAttributeWriter::finish().

Fixed issue with extra user count on proto collection.

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113107) when ready.

It would be great to finally get that geometry sets branch merged, so we can properly import hair and point clouds. Any help with that would be appreciated.

I think it would be better to use a point cloud than a mesh if we can, it's just a better fit without unnecessary data and better visualization. But the same node graph probably works with both meshes and point clouds, so it seems reasonable to go that way first if the geometry sets are not done soon.

Just to follow up on this thread from some time ago: Work on geometry sets import is in progress in this pull request: #115623

Given that the geometry sets work is already far along, I believe it makes sense to stick with point clouds for point instancing geometry, rather than meshes. However, this means that point animation won't be supported initially but this can be added once the geometry sets work is merged.

> It would be great to finally get that geometry sets branch merged, so we can properly import hair and point clouds. Any help with that would be appreciated. > > I think it would be better to use a point cloud than a mesh if we can, it's just a better fit without unnecessary data and better visualization. But the same node graph probably works with both meshes and point clouds, so it seems reasonable to go that way first if the geometry sets are not done soon. Just to follow up on this thread from some time ago: Work on geometry sets import is in progress in this pull request: https://projects.blender.org/blender/blender/pulls/115623 Given that the geometry sets work is already far along, I believe it makes sense to stick with point clouds for point instancing geometry, rather than meshes. However, this means that point animation won't be supported initially but this can be added once the geometry sets work is merged.

I'm taking a look now too. First impressions are great! The "4004 Moore Lane" DPEL asset loads with its vegetation now. However, there is a rather large set of memory leaks stemming from the node tree building:

From the OpenUSD City Set

Error: Not freed memory blocks: 140, total unfreed memory 0.044193 MB
strdup len: 56 0000011AC58B5C38
NodeTree len: 520 0000011AC63C0E38
init_data len: 264 0000011AC566E3C0
ntree_init_data len: 1552 0000011AC61AA440
make_socket len: 72 0000011AC61AD658
BLI_sprintfN len: 12 0000011AC62D5748
...
I'm taking a look now too. First impressions are great! The "4004 Moore Lane" DPEL asset loads with its vegetation now. However, there is a rather large set of memory leaks stemming from the node tree building: ``` From the OpenUSD City Set Error: Not freed memory blocks: 140, total unfreed memory 0.044193 MB strdup len: 56 0000011AC58B5C38 NodeTree len: 520 0000011AC63C0E38 init_data len: 264 0000011AC566E3C0 ntree_init_data len: 1552 0000011AC61AA440 make_socket len: 72 0000011AC61AD658 BLI_sprintfN len: 12 0000011AC62D5748 ... ```

The latest package build for this PR is available here: https://builder.blender.org/download/patch/PR113107/

@wave Thanks so much for the invisibleIds USD example! I've added support for handling point instancer masking. The mask values are now read into a "mask" point attribute that is used as the Selection input of the Instance on Points node:

image

Here is the example loaded in Blender (colors are missing because our Shape reader implementation doesn't support attributes yet):

image

The latest package build for this PR is available here: https://builder.blender.org/download/patch/PR113107/ @wave Thanks so much for the `invisibleIds` USD example! I've added support for handling point instancer masking. The mask values are now read into a "mask" point attribute that is used as the `Selection` input of the `Instance on Points` node: ![image](/attachments/3820ee05-bdbd-44d2-837e-d92e36761003) Here is the example loaded in Blender (colors are missing because our Shape reader implementation doesn't support attributes yet): ![image](/attachments/cb9107cb-f6af-434a-9114-f5d6cd7dd42e)
101 KiB
998 KiB
Michael Kowalski added 1 commit 2024-01-24 03:15:33 +01:00
USD: fix point instancer node treee mem leak.
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
3dd14a43a3

I'm taking a look now too. First impressions are great! The "4004 Moore Lane" DPEL asset loads with its vegetation now. However, there is a rather large set of memory leaks stemming from the node tree building:

Thanks for catching this! I just pushed a fix for the memory leak, but please let me know if the issue persists.

> I'm taking a look now too. First impressions are great! The "4004 Moore Lane" DPEL asset loads with its vegetation now. However, there is a rather large set of memory leaks stemming from the node tree building: Thanks for catching this! I just pushed a fix for the memory leak, but please let me know if the issue persists.

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113107) when ready.
First-time contributor

Works fine with PlantFactory instances

Works fine with PlantFactory instances

Works fine with PlantFactory instances

Thanks for testing!

> Works fine with PlantFactory instances Thanks for testing!
Jesse Yurkovich approved these changes 2024-01-29 06:12:24 +01:00
Jesse Yurkovich left a comment
Member

This seems to function quite well and I left just 2 minor comments. The most difficult part here is the stage reading which is quite confusing but I don't have a good suggestion on how to make it more clear.

I've also tested mixing in with Scene graph instances and didn't notice any problems.

This seems to function quite well and I left just 2 minor comments. The most difficult part here is the stage reading which is quite confusing but I don't have a good suggestion on how to make it more clear. I've also tested mixing in with Scene graph instances and didn't notice any problems.
@ -0,0 +245,4 @@
group_output,
static_cast<bNodeSocket *>(group_output->inputs.first));
BKE_ntree_update_main_tree(bmain, ntree, nullptr);

A call to BKE_object_modifier_set_active(object_, md); can be done after the ntree_update to ensure the modifier is selected/active. It's fine without it but it means the user has to select the modifier themselves before they can view it in the node editor.

A call to `BKE_object_modifier_set_active(object_, md);` can be done after the ntree_update to ensure the modifier is selected/active. It's fine without it but it means the user has to select the modifier themselves before they can view it in the node editor.
makowalski marked this conversation as resolved
@ -618,0 +718,4 @@
}
}
std::optional<UsdPathSet> USDStageReader::collect_point_instancer_proto_paths() const

The use of an optional here isn't really buying much since it basically means the same thing as an empty Set. It's also causing additional copy ctor calls for the Set since optional can only hold values and not references. I'd say just return the UsdPathSet.

The use of an optional here isn't really buying much since it basically means the same thing as an empty Set. It's also causing additional copy ctor calls for the Set since optional can only hold values and not references. I'd say just return the UsdPathSet.
makowalski marked this conversation as resolved
Michael Kowalski added 2 commits 2024-01-29 21:25:49 +01:00
Michael Kowalski added 2 commits 2024-01-31 00:22:25 +01:00
Changed USDStageReader::collect_readers pruned_prims argument
from a reference to a pointer.
USD: improve protoype collection names
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
36dd5ce5b3
Now adding the prototype index to the point instancer
prototype collection names.  The index will help identify
the collections and will also ensure that the collection
names maintain the correct alphabetical order, so that
the original prototype index remains valid.  (The Collection
Info geometry node orders collections alphabetically for
instancing, so we can't use an arbitrary collection name.)
Michael Kowalski changed title from WIP: USD PointInstancer import support to USD PointInstancer import support 2024-01-31 00:23:29 +01:00

@deadpin Thanks for the review! I made the final round of changes.

I also made one additional improvement to the prototype collection naming:

Now adding the prototype index to the point instancer prototype collection names. The index will help identify the collections and will also ensure that the collection names maintain the correct alphabetical order, so that the original prototype index remains valid. (The Collection Info geometry node orders collections alphabetically for instancing, so we can't use an arbitrary collection name.)

@deadpin Thanks for the review! I made the final round of changes. I also made one additional improvement to the prototype collection naming: Now adding the prototype index to the point instancer prototype collection names. The index will help identify the collections and will also ensure that the collection names maintain the correct alphabetical order, so that the original prototype index remains valid. (The Collection Info geometry node orders collections alphabetically for instancing, so we can't use an arbitrary collection name.)

@blender-bot build

@blender-bot build
Jesse Yurkovich reviewed 2024-01-31 06:03:52 +01:00
@ -618,0 +700,4 @@
/* Format the collection name to follow the proto_<index> pattern. */
std::ostringstream ss;
ss << std::setw(max_index_digits) << std::setfill('0') << proto_index;
std::string coll_name = "proto_" + ss.str();

After this is checked in I'll probably move this to using fmtlib to remove the dependency on iostreams. It requires a bit of cmake changes so I wouldn't bother to do it in this checkin.

After this is checked in I'll probably move this to using `fmtlib` to remove the dependency on iostreams. It requires a bit of cmake changes so I wouldn't bother to do it in this checkin.
@ -117,0 +139,4 @@
* the prim cannot be converted.
*/
USDPrimReader *collect_readers(const pxr::UsdPrim &prim,
const UsdPathSet *pruned_prims,

Is there really a difference between null and an empty set to have this be a pointer? Or are you worried about performance of checking the empty set perhaps?

Is there really a difference between null and an empty set to have this be a pointer? Or are you worried about performance of checking the empty set perhaps?

My thought was that there might be use cases in the future where we call collect_readers() without needing the option to prune anything, so it might be nice to avoid constructing an empty set just to call the function. But I don't feel strongly about this at all. I can change this to a reference, if you feel it's cleaner to do so. Let me know.

My thought was that there might be use cases in the future where we call collect_readers() without needing the option to prune anything, so it might be nice to avoid constructing an empty set just to call the function. But I don't feel strongly about this at all. I can change this to a reference, if you feel it's cleaner to do so. Let me know.

I'd personally only change the API as-needed when necessary. Though I think creating an empty set in those cases would be fine really.

I'd personally only change the API as-needed when necessary. Though I think creating an empty set in those cases would be fine really.

Thanks for the feedback! I updated the code to pass the set as a reference.

Thanks for the feedback! I updated the code to pass the set as a reference.
makowalski marked this conversation as resolved
Michael Kowalski added 1 commit 2024-01-31 16:59:53 +01:00
Merge branch 'main' into usd-read-pointinstancer-2
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
49e2a19376

@blender-bot build

@blender-bot build
Michael Kowalski added 1 commit 2024-01-31 22:56:08 +01:00
USD: pruned_pims argument now a reference
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
6b8a77e7a1

@blender-bot build

@blender-bot build
Jesse Yurkovich approved these changes 2024-02-01 03:49:20 +01:00
Michael Kowalski merged commit fcd10ee33a into main 2024-02-01 15:37:54 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
9 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#113107
No description provided.