Geometry Nodes: Add selection and depth options to realize instances #116582
No reviewers
Labels
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
7 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#116582
Loading…
Reference in New Issue
No description provided.
Delete Branch "aryeramaty/blender:WIP-realize-depth"
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?
This is a rebased version of the pull request made by @ab_stanton, which you can find here: #104832. I attempted to follow the original design, as the developers seemed to agree on the general direction. Nonetheless, I made multiple changes as needed to make it functional.
There are a couple of bugs I need to fix to prepare it for review:
Also, I wasn't sure if we still need the 'selection input' since we can use the depth. What do you think?
Other mentions:
be aware that I am only a newbie to C++, I hope this code is at least good enough to build upon something better.
I am taking the liberty to comment here as a user, since you're asking a few design questions. Specifically regarding depth versus selection. I have been generating trees and instancing leaves, finally realizing them because of an unrelated performance issue in Cycles (nested instances have a memory penalty right now).
I think selection is important to keep, because you might have different instances serving largely different purposes in a single nodetree. In which case, a single depth input isn't enough to filter out what you want to realize vs what you want to keep as instances.
Now, what if instance depth was a builtin attribute on the instance domain? enabling the user to realize instances on the basis of this attribute, using a compare node to select specific depth levels? you'd be able to further refine the selection with regular instance attributes (=leaves, twigs, or entire branches, keeping the tree example). In that case, we could do with a single selection input and nothing else.
This is just food for thought, considering only a very specific use case. In any case thanks for working on this
Thanks for the suggestion! BTW I too consider myself a user, so, I feel you :)
As far as I understand, a single depth value is enough if a zero-depth means 'do not realize this instance', but I think it's better to keep the selection input either way for a couple of other reasons. For example, it maintains the same convention as other nodes (e.g., 'duplicate elements'). Also, we may need it if we separate the function for
realizing all
andrealizing to a certain depth
(The problem: the depth value should be overridable by the 'realize all' input, which then would lack a selection).This is indeed an interesting idea, and pretty useful, but I don't think it should be within the scope of the PR. First of all, while I strongly believe in the concept of 'chewing more than you can,' it still sounds much more complex. I don't think there is a way to manipulate specific instances that are inside others in the current GN. For example, will we be able to use the proposed
depth
built-in attribute to control the position of instances inside other instances? Will they keep the underlying referenced geometry intact? In your tree example, imagine moving a specific leaf using this node—would that reflect on the other instanced branches?Anyway, in the meantime, I am working on propagating the parent instances' attributes to the children instance components. This is, from a design POV, a much less debatable feature. I am trying to wrap my head around it; we'll see...
A little update:
Thanks for looking into this! We talked about this briefly at the module meeting and thought that the "Realize All" input should be a node input (socket) so it can be a field.
Update:
Now attributes are propagated between the parent instances to its children instances. However, there is still a 'bug' where it will use all the attributes in the current elements (even from instances that were not realized). This is also true for the other component. We just need to modify the gather_attributes_for_propagation() function to consider the depth value.
On another note, I tested it on my own use case (with 500,000 multiple-level nested instances and 40,000,000 verts), and it worked as intended—apart from the mentioned bug, which doesn't really impact usability.
The next steps are:
743b2b4fcb
todd1822dcc0
Hmm... I mistakenly updated from main in a rather stupid way. Sorry for the clutter.
Update on the current state of the patch:
In terms of usability, at least in my tests, it's working fine.
There are some performance issues; namely, it's slower than the original by approximately 30%.
Currently, the 0 depth is intentionally buggy—it does realize everything but does not propagate any attributes (intended, as I don't think the end implementation would realize instances at all at 0 depth).
In terms of performance, I used 10,000,000 instances of instanced cubes.
The numbers are from the geometry node UI timing option
Realizing it completely in main took around 4000ms on my machine.
Realizing it only one level in the branch took 3000ms.
Realizing it completely in the branch took around 6500ms.
In my own 'real-life scenario,' the overall node tree was 20% faster using this branch, primarily because this feature enables many optimization opportunities.
Update:
It's working mostly as intended, but there are still two issues that I'm afraid I won't be able to finish in time for 4.1
execute_instances_tasks
function.Update:
I've resolved the issue from yesterday, and as far as I can see, functionality-wise, it's working as intended.
I'm still not entirely sure if it's ready for review, as I can think of some cleanup I would like to do, on the other hand, I would really appreciate another set of more experienced eyes to ensure I am heading in the right direction. I guess I will remove the WIP tag for now.
WIP:Add selection and depth options to realize instancesto Add selection and depth options to realize instances@ -237,2 +237,4 @@
const GeometryComponent &component)>;
bool attribute_foreach(Span<GeometryComponent::Type> component_types,
bool include_instances,
Redundant parameter?
Removed it.
@ -238,1 +238,4 @@
bool attribute_foreach(Span<GeometryComponent::Type> component_types,
bool include_instances,
const int current_depth,
Cleanup:
const
is not necessary for trivial types (int
,bool
,float
,IndexRange
,Span<>
, ...) in function declaration.@ -239,0 +239,4 @@
bool attribute_foreach(Span<GeometryComponent::Type> component_types,
bool include_instances,
const int current_depth,
const int depth_target,
int current_depth, int depth_target
->IndexRange depth_to_check
?Hmm... But what happens when the
depth_target
is negative? I use it to create an endless loop. Can this be achieved withIndexRange
?std::numeric_limits<int>::max()
?Not sure if this is good idea to use negative end value for positive loops in any way tbh/
@ -251,6 +259,14 @@ struct GeometrySet {
const AnonymousAttributePropagationInfo &propagation_info,
Map<AttributeIDRef, AttributeKind> &r_attributes) const;
void gather_attributes_for_propagation(Span<GeometryComponent::Type> component_types,
Im not really strong opinion in this, but not sure if such specific method (and above one) is necessary here. Might be better to keep it as static function in geometry module.
I am not sure either, and probably not experienced enough to clearly outline the pros & cons. I probably just wanted to keep my changes in places where it is easier to see their 'parent' variation.
@ -576,0 +594,4 @@
* - callback: Callback function for attribute processing.
*/
// Initialize flag to track if child instances have the specified components.
Cleanup: Comment style.
https://developer.blender.org/docs/handbook/guidelines/c_cpp/#comments
@ -576,0 +604,4 @@
// Iterate over instances based on the selection index mask.
const Instances &instances = *this->get_instances();
// ensure objects and collection are included.
Instances ensure_instances = instances;
What for this copy is done?
Because the orignal component is a const, but probably there is a better way to do that.
What should I do?
Well, i think it is better tell the caller to do it
@ -576,0 +606,4 @@
// ensure objects and collection are included.
Instances ensure_instances = instances;
ensure_instances.ensure_geometry_instances();
const IndexMask indices = (current_depth == 0) ? selection : IndexMask(IndexRange(ensure_instances.instances_num()));
Cleanup: Clang-format was missed here. You can run
make format
in root folder of blender to fix that.Plus: Why index mask used there at all?
This looks like it would be much easier to use something like
Pseudocode:
@ -576,0 +625,4 @@
// Flag to track if any relevant attributes were found.
bool is_relevant = false;
// Iterate over specified component types.
Comments should not paraphrase next line.
@ -576,0 +627,4 @@
// Iterate over specified component types.
for (const GeometryComponent::Type component_type : component_types) {
// Skip if the component type is not present.
Not really useful comment.
@ -576,0 +634,4 @@
// Check for a special instance condition.
const bool is_special_instance = (component_type == GeometryComponent::Type::Instance) && (component_types.size() > 1);
if (is_special_instance && !is_child_has_component) {
Better do describe why here is
skip
instead of// Check for a special instance condition.
.@ -14,1 +14,4 @@
void join_attributes(const Span<const bke::GeometryComponent *> src_components,
bke::GeometryComponent &result,
const Span<StringRef> ignored_attributes);
https://developer.blender.org/docs/handbook/guidelines/c_cpp/#const
https://developer.blender.org/docs/handbook/guidelines/c_cpp/#function-arguments
ignored_attributes need to be last becouse of the default value
It is necessary to have default value (see
bke::gather_attributes
as instance)?@ -185,6 +185,9 @@ static void join_component_type(const bke::GeometryComponent::Type component_typ
options.keep_original_ids = true;
options.realize_instance_attributes = false;
options.propagation_info = propagation_info;
options.depths = VArray<int>::ForSingle( 99, instances.get()->instances_num());
99
->std::numeric_limits<int>::max()
@ -286,3 +314,4 @@
}
};
static void realize_collections(Collection *collection, bke::Instances* instances){
bke::Instances collection_to_instance(const Collection &collection, const bool recursive)
?@ -477,3 +529,1 @@
index++;
}
FOREACH_COLLECTION_OBJECT_RECURSIVE_END;
Collection *collection_ptr = &reference.collection();
What for this change?
Otherwise, nested collections would be flattened instead of realized. This is similar to the 'collection info node' implementation. I thought it's probably better to move it into
geometry_set_instances.cc
. It is similar to theensure_geometry_instances
but keeps the collection hierarchyShould I move it there?
@ -480,0 +531,4 @@
realize_collections(collection_ptr, instances.get());
const bke::GeometrySet colleciton_geometry = bke::GeometrySet::from_instances(instances.release());
/* important as otherwise the Instances pointer would be deleted with the GeomtrySet*/
colleciton_geometry.get_component(bke::GeometryComponent::Type::Instance)->add_user();
I tried something like that, but it didn't work for me. It seems to be related to the geometrySet getting out of scope or the component not owning the data. I'm not entirely sure what's happening there.
@SimonThommes @HooglyBoogly @JacquesLucke Hi, while checking this node, i noticed this:
In the past we thought about analog of Separate Component node for instances (so this will output instances only with required components, like only mesh instances or only curves and so).
While checking this node i notices that we can perform the same operation by separating instances by mask/depth parameters.
Do you think just splitting this node function in separate node with more clear inputs (instead of
Selection
,Realize All
,Depth
, ...) might be better?On technical side, can say this might be much more easier node to create and maintain (just recursive copy of instances with deleting part of components).
I have been deep a little more in this node for now, but there is still just code clean up comments.
I'd prefer to wait if other design for this feature (Separate Instances in Depth) might be better solution.
@ -41,0 +71,4 @@
* output will contain an `id` attribute as well. The output id is generated by mixing/hashing ids
* of instances and of the instanced geometry data.
*
* Will realize only the instances chosen by varied_depth_option to there chosen depth.
@ -1581,0 +1973,4 @@
all_instances.depths = VArray<int>::ForSingle(VariedDepthOption::MAX_DEPTH,
geometry_set.get_instances()->instances_num());
IndexMaskMemory memory;
all_instances.selection = IndexMask::from_bools(
IndexMask(geometry_set.get_instances()->instances_num())
@ -16,0 +18,4 @@
b.add_input<decl::Bool>("Selection")
.default_value(true)
.hide_value()
.supports_field()
supports_field
->field_on_all
.I think this is the reason why you have issues with attribute propagation, not sure. But this is still mistake. The same for all other new inputs.
from what I understands (I am not sure I am correct I am new to blender) field_on_all will enable supports_field in all the inputs including b.add_inputdecl::Geometry("Geometry"); is that a problem?
No, here is a long story about how attribute lifetime is defined..
If simple,
supports_field
just enable to input field in field node.field_on_all
mean what input field need to be stored in input geometry (otherwise, geometry nodes will delete unused attribute from inputs).@ -23,0 +49,4 @@
static auto selection_override = mf::build::SI2_SO<int, bool, bool>(
"selection_override",
[](int value, bool selection) { return value == 0 ? false : selection; },
min(0)
(ormax(...)
) functions for socket declaration is just soft limits. Sovalue == 0
->value <= 0
.its value == 0 by design the value input is the output of depth_override there the output will be VariedDepthOption::MAX_DEPTH(-1) for realize_all
I will make it clearer in my next PR to Arye
@ -23,0 +53,4 @@
mf::build::exec_presets::AllSpanOrSingle());
Field<int> depth_field_overrided(
FieldOperation::Create(depth_override, {depth_field, realize_all_filed}));
depth_field
->std::move(depth_field)
(the same for other fields).Be careful, you can move variable only once, after that variable will get default value.
@ -23,0 +66,4 @@
evaluator.evaluate();
geometry::VariedDepthOption varied_depth_option;
varied_depth_option.depths = evaluator.get_evaluated<int>(evaluated_depth_index);
evaluator.get_evaluated<int>(evaluated_depth_index);
->evaluator.get_evaluated<int>(0);
`/**
*/`
according to this add return value should be used as get_evaluated input
It's make sense in case if there is a lot of such values of them is defined in runtime by arbitrary number of fields. But for now this is just one zero.
yes but I think its make it a bit less readable and may cause bugs in the future if the file will be changed and more value added.
but if this is the convention I will change it back to 0
@mod_moder I'm not sure what exactly you are proposing. The user has no granular control for a mask of elements anywhere in the hierarchy, but just the top level. So what are the inputs for that node that you are proposing, what exactly would it do and how is it different to the current design of this node.
@SimonThommes Well, i'm okay with
Top
andBottom
level inputs (just int fields) for top level instances to separate selection into separate geometry and another one for others (like selection by depth range and unselected).So,
Top
level can be 0 by default,Bottom
can be 100 or something like this. And if someone need to select only 0-5 levels of some top level instances, result will be such. Next, this instances can be realized.All non-selected instances will be on other output.
@mod_moder
It's getting more clear to me, but would still be useful if you explicitly described the node you are proposing.
What I'm reading out of this is you're proposing a node that would look almost the same as the one of this PR, but it just doesn't do the realize step (?)
@SimonThommes Probably yes,
Separate Instances in Depth
might be the same (i'm actually started this thread from idea to split this new functionality forRealize Instances
in just separate one node) asRealize Instances
withDepth
field input. Maybe is there a case to useSeparate Instances in Depth
in other way, like actually just access only to nested instances as part of lod system and so.Main propose for me there is to do not make already complicated implementation of instances realization more complicated. And in this context, i think is okay to just perform this depth selection as separate node for user.
@mod_moder
Yes, I think you have a point. I can see just separating certain instances without realizing being useful. To be honest, imo this functionality should maybe just be merged into the
Separate Geometry
node though.But I'm not convinced that this functionality would actually replace a
Realize Depth
setup. How would you tackle the 'from leaves' use-case mentioned in #116044 by splitting instances and realizing them in separate steps?@mod_moder
hmm... I am not sure I am following the idea here. If I understand correctly, there are three things I am unsure about:
@SimonThommes
From Leaves
case looks like just separating all leaves instances and group them in top by original instances. So, if number of levels of instances to group leaves is just attribute value for top level ones, then this can be another one node (or part ofSeparate Instances in Depth
).@aryeramaty
Separate Instances in Depth
, because this will mean that we have to separate part of instances as it is and part of instances need to be actually separated in depth.Sorry for the delay here. Thanks for your persistence here! There's a lot of new code that I didn't get the chance to read fully, but I left some preliminary comments.
@ -77,2 +76,2 @@
GeometryComponent &result,
const Span<StringRef> ignored_attributes = {})
void join_attributes(const Span<const GeometryComponent *> src_components,
const Span<StringRef> ignored_attributes,
Could you keep the argument order and naming the same here? it's potentially a valid cleanup, but not really related to the goal of the PR. And things are easier to review if we keep the changes as small as posisble.
Sure.
@mod_moder requested that change in a prior review.
Can be reverted here and be done in a later PR.
Ah, if this was original order of args, i might made mistake.
@ -474,40 +543,14 @@ static void foreach_geometry_in_reference(
FunctionRef<void(const bke::GeometrySet &geometry_set, const float4x4 &transform, uint32_t id)>
fn)
{
switch (reference.type()) {
What's the reason to change this to creating and returning a
GeometrySet
?The change was done because this switch case was needed in two places. Here and in attribute_foreach.
It was put into a separate function and instead of creating a local GeometrySet and calling fn using it we use GeometrySet as an output_param to be used in the calling function as needed.
@ -226,2 +220,4 @@
};
struct AllInstancesInfo {
/** store an array of void pointer to attributes for each component. */
store
->Stores
@ -463,6 +500,38 @@ static Vector<std::pair<int, GSpan>> prepare_attribute_fallbacks(
return attributes_to_override;
}
static bke::GeometrySet &geometry_set_from_reference(const InstanceReference &reference,
Returning a reference to the output argument seems unnecessary and potentially confusing. I'd just return a
bke::GeometrySet
by value hereIsn't it more costly to create a local GeometrySet and then copy it twice in the return than to use an output param?.
Not really, creating a geometry set should be very cheap, and in C++ they should usually amount to the same thing. Some terms to look up are return value optimization and copy elision.
Ok didn't see that it is basically a wrapped pointer.
@ -466,0 +524,4 @@
r_geometry_set = bke::GeometrySet(); // Return an empty GeometrySet for None type
break;
}
default: {
The default case can be removed. Instead
return {};
can be added after the switch. That would give compiler warnings when new enum values are added, which we want.@ -679,0 +794,4 @@
}
}
/*Flag to track if any relevant attributes were found.*/
Missing space after
/*
here and in other places@ -679,0 +828,4 @@
* Specialized for Specialized attribute_foreach to get:
* current_depth, depth_target, instance_depth and selection.
*/
void static gather_attributes_for_propagation(
void static
->static void
@ -679,0 +951,4 @@
bke::AttributeIDRef id = all_instances_attributes.ids[attribute_index];
eCustomDataType type = all_instances_attributes.kinds[attribute_index].data_type;
blender::bke::MutableAttributeAccessor attr = dst_instances->attributes_for_write();
attr.lookup_or_add_for_write_only_span(id, domain, type).finish();
What is going on here? Looks like the attribute writer doesn't actually do anything
I am hoping its clearer now. this section makes sure generic output attributes exists so later code can be better optimaized.
@ -16,0 +23,4 @@
b.add_input<decl::Bool>("Realize All")
.default_value(true)
.field_on_all()
.description("Determine wether to realize nested instances completly");
Realize all levels of nested instances for a top-level instances
What about:
Realize all levels of nested instances for a top-level instances.\nNote: Override the value of the Depth input
Nice! I'd change "Override" to "Overrides" and probably remove the newline and "Note:" to simplify things. But otherwise, seems perfect.
Functionality is working great!
I'd suggest mentioning in the tooltip that the
Depth
input is not used when theRealize All
input is true, since that might be confusingThe whole topic of realizing from the other direction is still unclear to me on a design level, since there are many aspects of it that complicate it more compared to this approach. So to me that is a separate topic now.
So I'll preliminarily approve this from the user side now only with the note about the tooltip and what Hans already mentioned.
Add selection and depth options to realize instancesto Geometry Nodes: Add selection and depth options to realize instancesLooks like the patch hasn't been updated. Maybe you forgot to push your changes?
@HooglyBoogly if I look in files changed then the changes are there.
I don't know why it will not show them to you, maybe there is some bug becouse arye merged a pull request from me into his branch an didn't push the changes himself?.
Indeed, if you look at the commit history you can see the last commits by David.
Overall looks good and reasonable. I've still got many comments, most of them should be quite simple to address.
The main annoying thing is the introduction of the new
attribute_foreach
andgather_attributes_for_propagation
functions. However, I don't have an idea for how to do it better right now and the code looks fine. Maybe we can extract some more general utilities to reduce duplication as a separate step.@ -28,0 +31,4 @@
/**
* Allow the user to choice which instances to realize and to what depth.
*/
struct VariedDepthOption {
...OptionS
Variable names for this type should generally also have the
s
at the end.@ -304,6 +320,25 @@ static int64_t get_final_points_num(const GatherTasks &tasks)
return points_num;
}
static void realize_collections(Collection *collection, bke::Instances *instances)
Use references instead of pointers.
@ -307,0 +326,4 @@
float4x4 transform = float4x4::identity();
transform.location() += float3((collection_child->collection)->instance_offset);
transform.location() -= float3(collection->instance_offset);
const int handle = instances->add_reference(*(collection_child->collection));
Unnecessary parenthesis, same above and below.
@ -378,3 +413,3 @@
}
static void create_result_ids(const RealizeInstancesOptions &options,
static void create_result_ids(bool keep_original_ids,
add
const
@ -466,0 +507,4 @@
const Object &object = reference.object();
const bke::GeometrySet geometry_set = bke::object_get_evaluated_geometry_set(object);
return geometry_set;
break;
Remove
break
afterreturn
.@ -535,2 +578,3 @@
for (const int i : transforms.index_range()) {
/* If at top level, get instance indices from selection field, else use all instances. */
const IndexMask indices = current_depth == 0 ? gather_info.selection :
const bool is_top_level = ...
The variable can also be used below.
@ -537,0 +579,4 @@
/* If at top level, get instance indices from selection field, else use all instances. */
const IndexMask indices = current_depth == 0 ? gather_info.selection :
IndexMask(IndexRange(instances.instances_num()));
for (const int mask_index : indices.index_range()) {
Use
indices.foreach_*
to iterate over index masks, it's more efficient than usingoperator[]
many times.@ -537,0 +583,4 @@
const int i = indices[mask_index];
/* If at top level, retrieve depth from gather_info, else continue with target_depth. */
const int depth_target = current_depth == 0 ? gather_info.depths[i] : target_depth;
depth_target
->child_target_depth
. That's better than havingtarget_depth
anddepth_target
as variable names in the same scope.@ -679,0 +758,4 @@
const Span<bke::GeometryComponent::Type> component_types,
const int current_depth,
const int depth_target,
const VArray<int> instance_depth,
Use reference.
@ -679,0 +764,4 @@
{
/* Initialize flag to track if child instances have the specified components.*/
bool child_has_component = true;
Why is this set to
true
?@ -679,0 +773,4 @@
const IndexMask indices = (0 == current_depth) ?
selection :
IndexMask(IndexRange(instances.instances_num()));
for (const int index : indices.index_range()) {
Use
indices.foreach_*
.@ -679,0 +775,4 @@
IndexMask(IndexRange(instances.instances_num()));
for (const int index : indices.index_range()) {
const int i = indices[index];
const int depth_target_tmp = (0 == current_depth) ? instance_depth[i] : depth_target;
child_depth_target
maybe?@ -679,0 +780,4 @@
instances.references()[instances.reference_handles()[i]]);
/* Process child instances with a recursive call.*/
if (current_depth != depth_target_tmp) {
child_has_component = child_has_component | attribute_foreach(instance_geometry_set,
Seems like we should be able to break out of the loop early when
child_has_component
is true.That was the case in earlier revisions, but we need to execute the attribute_foreach either way. For example, in cases where there are multiple children with relevant attributes, terminating the loop too quickly would omit some possible attributes.
I see, maybe just add a comment mentioning that breaking out of this loop early is not correct.
@ -679,0 +792,4 @@
}
/* Flag to track if any relevant attributes were found.*/
bool is_relevant = false;
any_attribute_found
@ -679,0 +796,4 @@
for (const bke::GeometryComponent::Type component_type : component_types) {
if (geometry_set.has(component_type)) {
/* Check if the current instance components is the main one*/
Missing dot and whitespace at end of comment.
@ -679,0 +797,4 @@
for (const bke::GeometryComponent::Type component_type : component_types) {
if (geometry_set.has(component_type)) {
/* Check if the current instance components is the main one*/
const bool is_special_instance = (bke::GeometryComponent::Type::Instance ==
What does "main one" mean?
Sounds like the variable should be called something like
is_main_instance_component
.@ -679,0 +811,4 @@
return true;
});
is_relevant = true;
Seems like this should be moved into the lambda above.
@ -679,0 +826,4 @@
* current_depth, depth_target, instance_depth and selection.
*/
static void gather_attributes_for_propagation(
bke::GeometrySet re_geometry_set,
What does
re
stand for?@ -679,0 +907,4 @@
options.propagation_info,
attributes_to_propagate);
attributes_to_propagate.remove("position");
attributes_to_propagate.remove("radius");
Does removing
radius
andposition
here have a purpose (still)? The position attribute was removed a while ago, and the radius attribute never had a special meaning on instances.@ -679,0 +930,4 @@
return;
}
VArray<blender::float4x4>::ForSpan(src_base_transforms);
This line does nothing.
@ -679,0 +959,4 @@
const bke::InstancesComponent &src_component = static_cast<const bke::InstancesComponent &>(
*src_components[component_index]);
const bke::Instances &src_instances = *src_component.get();
const blender::float4x4 src_base_transform = src_base_transforms[component_index];
Use reference.
@ -679,0 +960,4 @@
*src_components[component_index]);
const bke::Instances &src_instances = *src_component.get();
const blender::float4x4 src_base_transform = src_base_transforms[component_index];
const Array<const void *> attribute_fallback_array = attribute_fallback[component_index].array;
Use
Span
.@ -679,0 +969,4 @@
}
const IndexRange dst_range = offsets[component_index];
for (const int attribute_index : all_instances_attributes.index_range()) {
bke::AttributeIDRef id = all_instances_attributes.ids[attribute_index];
Use
const
.@ -679,0 +977,4 @@
dst_instances->attributes_for_write().lookup_for_write_span(id);
GMutableSpan dst_span = write_attribute.span;
if (!write_attribute) { // do not override existing attributes
Comment style. Also, do you have a specific example where this condition might be true?
hm... I tried to disable it and see if I could locate any wrong behavior, I remember there was a reason for it but I'm not sure anymore.
On another note, I have found a bug that if one of the children's instances has the same attribute as the father it overwrites it. which wouldn't be a problem as long it doesn't overwrite the attribute on its siblings, which means the sibling inherited attribute is replaced with the default value.
@ -679,0 +990,4 @@
}
GVArray src_span = GVArray::ForSingle(*cpp_type, dst_range.size(), attribute_ptr);
array_utils::copy(src_span, dst_span.slice(dst_range));
Can use
cpp_type->fill_assign_n
instead ofarray_utils::copy
which is a bit better here I think.@ -679,0 +1007,4 @@
auto &dst_component = r_realized_geometry.get_component_for_write<bke::InstancesComponent>();
Vector<const bke::GeometryComponent *> for_join_attributes;
for (bke::GeometryComponentPtr compent : src_components) {
typo
compent
@ -679,0 +1011,4 @@
for_join_attributes.append(compent.get());
}
join_attributes(
Add a comment here about what kinds of attributes are joined here, compared to the attributes created above.
position
might not be a special case here anymore.@ -771,3 +1118,3 @@
static void execute_realize_pointcloud_task(
const RealizeInstancesOptions &options,
bool keep_original_ids,
Use
const
. Same below.Also, I'm not quite sure this is actually an improvement. This specific function does not really care about original ids and just forwards the parameter. Also, it makes it harder to add more parameters in the future. Would prefer to revert this change.
@ -1597,0 +1957,4 @@
*/
static void propagate_instances_to_keep(
const bke::GeometrySet &geometry_set,
IndexMask selection,
Use const reference.
@ -23,0 +49,4 @@
[](int depth, bool realize_all_field) {
return realize_all_field ? geometry::VariedDepthOption::MAX_DEPTH : std::max(depth, 0);
},
mf::build::exec_presets::AllSpanOrSingle());
No need to use
AllSpanOrSingle
here, because the benefit this should be negilible given how much work comes afterwards to actually realize instances. Same below.@ -23,0 +51,4 @@
},
mf::build::exec_presets::AllSpanOrSingle());
Field<int> depth_field_overrided(FieldOperation::Create(
overrided
->overridden
Please run
make format
if you haven't recently.I believe I've addressed all the comments (there are probably a couple of stuff I misunderstood, but let's hope for the better :). Apologies for the delay; David and I had some things last week that slowed us down. Thanks again for your time, Hans and Jacques.
Looks good to me now (except for small inline comments), thanks for spending all this time on this patch!
@ -679,0 +820,4 @@
* current_depth, depth_target, instance_depth and selection.
*/
static void gather_attributes_for_propagation(
bke::GeometrySet geometry_set,
This should probably take the geometry by const reference.
@ -23,0 +49,4 @@
[](int depth, bool realize_all_field) {
return realize_all_field ? geometry::VariedDepthOptions::MAX_DEPTH : std::max(depth, 0);
},
mf::build::exec_presets::Materialized());
Materialized
is the default preset and does not have to be passed in, can just be omitted. Same in the other file.@blender-bot build
This seems reasonable. Thanks for all your work here. Just noticed two tiny comment style things.
@ -466,0 +520,4 @@
return geometry_set;
}
case InstanceReference::Type::None: {
return {}; // Return an empty GeometrySet for None type
Comment style:
/* Return an empty GeometrySet for None type. */
@ -679,0 +789,4 @@
for (const bke::GeometryComponent::Type component_type : component_types) {
if (geometry_set.has(component_type)) {
/*Check if the current instance component is the selected one. Instances are handled
/*Check
->/* Check