USD PointInstancer import support #113107
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#113107
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "expenses/blender:usd-read-pointinstancer-2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.)
@ -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.
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.
@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.
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.
@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.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 usingComputeInstanceTransformsAtTime
?I also commented out the geometry node setup as it prevents points from being rendered by-default.
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:
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:
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.
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.
@ -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 tobNodeSocketValueBoolean::value
: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.
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.
@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.
@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.
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.
@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:
This is what the node tree looks like:
Here is the OpenUSD City Set example:
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:
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)
Might as well put this in the
blender::io::usd
namespace too. I'd suggest a few tweaks too, just for general cleanup.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.@ -0,0 +107,4 @@
PointCloud *point_cloud = BKE_pointcloud_new_nomain(positions.size());
auto positions_span = point_cloud->positions_for_write();
auto
->MutableSpan<float3>
. Same withauto
elsewhere here. The type names are generally helpful to the reader, might as well include them.@ -0,0 +114,4 @@
}
auto scales_attribute =
point_cloud->attributes_for_write().lookup_or_add_for_write_only_span<float3>(
A temporary variable would make this look a bit nicer.
@ -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++) {
The check can be removed from inside the loop
for (const int i : IndexRange(std::min(scales.size(), positions.size())) {
@ -0,0 +140,4 @@
orientations[i].GetImaginary()[2]);
}
else {
orientations_attribute.span[i] = math::Quaternion();
math::Quaternion::identity()
I think@ -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");
NULL
->nullptr
@ -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);
The rotation to Euler node should be unnecessary here.
@ -0,0 +268,4 @@
void USDPointInstancerReader::set_collection(Main *bmain, Collection *coll)
{
if (!object_) {
Collection *coll
can become a reference, then the assert can be removed. Also not sure here if!object_
is an overly defensive check@ -0,0 +274,4 @@
BLI_assert(coll);
ModifierData *mod_data = BKE_modifiers_findby_type(this->object_, eModifierType_Nodes);
Typical name for
mod_data
is justmd
@ -0,0 +281,4 @@
}
NodesModifierData *nmd = reinterpret_cast<NodesModifierData *>(mod_data);
if (!nmd) {
NodesModifierData
will not be null ifModifierData
isn't null, and that was already checked.@ -0,0 +304,4 @@
return;
}
bNodeSocketValueCollection *socket_data = (bNodeSocketValueCollection *)sock->default_value;
static_cast
here@ -0,0 +1,57 @@
/*
Looks like this can use the new copyright and license format
@ -54,10 +56,25 @@
#include "WM_api.hh"
#include <iostream>
Not clear that this include is used (sorry if I'm just missing it)
@ -618,0 +706,4 @@
continue;
}
blender::Vector<USDPrimReader *> proto_readers;
blender::Vector
->Vector
@blender-bot build
@blender-bot package
Package build started. Download here 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/
@ -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;
This can use
socket->default_value_typed<bNodeSocketValueBoolean>()->value = true;
to avoid the C style castI 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:
@blender-bot package
Package build started. Download here when ready.
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.
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:
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 theSelection
input of theInstance on Points
node:Here is the example loaded in Blender (colors are missing because our Shape reader implementation doesn't support attributes yet):
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
Package build started. Download here when ready.
Works fine with PlantFactory instances
Thanks for testing!
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.@ -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.
WIP: USD PointInstancer import supportto USD PointInstancer import support@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
@ -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.@ -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?
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.
Thanks for the feedback! I updated the code to pass the set as a reference.
@blender-bot build
@blender-bot build