Geometry Nodes: Add selection and depth options to realize instances #116582

Merged
Jacques Lucke merged 119 commits from aryeramaty/blender:WIP-realize-depth into main 2024-05-03 12:48:12 +02:00
Contributor

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.

image

There are a couple of bugs I need to fix to prepare it for review:

  1. It was agreed that at depth 0, the node should not change the geometry at all; currently, it 'realizes all recursively'
  2. There should be a new input for 'realize all' instead of using the depth
  3. I used an old version of the join_instances function as the base for the join_transform_instance_components, which is probably not "the right things to do"
  4. Anonymous attributes aren't propagated on child instances (but do propagate on other child components).

Also, I wasn't sure if we still need the 'selection input' since we can use the depth. What do you think?

Other mentions:

  1. #92744
  2. #116044
  3. #114280

be aware that I am only a newbie to C++, I hope this code is at least good enough to build upon something better.

This is a rebased version of the pull request made by @ab_stanton, which you can find here: https://projects.blender.org/blender/blender/pulls/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. ![image](/attachments/96e038fd-13a1-4b79-9afe-0ad5d00e2a88) There are a couple of bugs I need to fix to prepare it for review: 1. It was agreed that at depth 0, the node should not change the geometry at all; currently, it 'realizes all recursively' 2. There should be a new input for 'realize all' instead of using the depth 3. I used an old version of the join_instances function as the base for the join_transform_instance_components, which is probably not "the right things to do" 4. Anonymous attributes aren't propagated on child instances (but do propagate on other child components). Also, I wasn't sure if we still need the 'selection input' since we can use the depth. What do you think? Other mentions: 1. https://projects.blender.org/blender/blender/issues/92744 2. https://projects.blender.org/blender/blender/issues/116044 3. https://projects.blender.org/blender/blender/issues/114280 be aware that I am only a newbie to C++, I hope this code is at least good enough to build upon something better.
Arye Ramaty added 22 commits 2023-12-27 15:40:31 +01:00
Arye Ramaty added 1 commit 2023-12-27 15:43:05 +01:00
Iliya Katushenock added this to the Nodes & Physics project 2023-12-27 15:44:36 +01:00
Iliya Katushenock added the
Interest
Geometry Nodes
label 2023-12-27 15:44:40 +01:00
Arye Ramaty added 1 commit 2023-12-27 16:00:10 +01:00
First-time contributor

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

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
Author
Contributor

Thanks for the suggestion! BTW I too consider myself a user, so, I feel you :)

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.

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 and realizing to a certain depth (The problem: the depth value should be overridable by the 'realize all' input, which then would lack a selection).

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

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...

Thanks for the suggestion! BTW I too consider myself a user, so, I feel you :) > 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. 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` and `realizing to a certain depth` (The problem: the depth value should be overridable by the 'realize all' input, which then would lack a selection). > 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 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...
Author
Contributor

A little update:

  1. I'm still working on it.
  2. Regarding the input options, I think I'll go with the design proposed by Iliya (proposed in the Blender chat). I'll elaborate more when I get to it.
  3. As I've said before, I don't consider myself a real developer; I am an artist (with a small technical background) in need of a specific feature, and I am willing to try and implement it myself. So, even though I tagged this PR as WIP, feel free to add technical comments, point out my mistakes, or suggest possible solutions. And if you feel you can take this patch and do it better, by all means, do it :)
A little update: 1. I'm still working on it. 2. Regarding the input options, I think I'll go with the design proposed by Iliya (proposed in the Blender chat). I'll elaborate more when I get to it. 3. As I've said before, I don't consider myself a real developer; I am an artist (with a small technical background) in need of a specific feature, and I am willing to try and implement it myself. So, even though I tagged this PR as WIP, feel free to add technical comments, point out my mistakes, or suggest possible solutions. And if you feel you can take this patch and do it better, by all means, do it :)
Member

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.

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.
Arye Ramaty added 2 commits 2024-01-17 17:20:35 +01:00
ad8ff59c30 Propagate attributes to child instances components
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
Author
Contributor

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:

  1. Make sure only the relevant attributes are passed to the final output.
  2. Change the sockets as was requested by the Geo-Node team
  3. (A lot of) Cleanups
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: 1. Make sure only the relevant attributes are passed to the final output. 2. Change the sockets as was requested by the Geo-Node team 3. (A lot of) Cleanups
Arye Ramaty added 1 commit 2024-01-21 13:24:34 +01:00
dd1822dcc0 Utilize depth input in the preprocess stage.
This adds two new variations for the functions: 'attribute_foreach' and 'gather_attributes_for_propagation'. The functions are the same but also accept a VArray containing the depth value for each instance component.

There is still a 'known bug' that a fully realized component (such as a mesh, etc.) may get attributes from a realized instance that doesn't hold any mesh component itself. This behavior may be debatable in some scenarios.

After some thought, I decided that the current design is okay-ish.
Arye Ramaty force-pushed WIP-realize-depth from 743b2b4fcb to dd1822dcc0 2024-01-21 16:46:21 +01:00 Compare
Author
Contributor

Hmm... I mistakenly updated from main in a rather stupid way. Sorry for the clutter.

Hmm... I mistakenly updated from main in a rather stupid way. Sorry for the clutter.
Arye Ramaty added 1 commit 2024-01-22 10:26:18 +01:00
Arye Ramaty added 1 commit 2024-01-22 10:45:30 +01:00
3ad10b8386 Temporarily set the default value of the depth to 99
This is a tmp change so that old files will be opened correctly.
Arye Ramaty added 1 commit 2024-01-23 14:42:07 +01:00
bed27190d2 Clean-up
Cleaned many parts of the code, mainly making it more similar to the other component implementations and removing redundant code and comments.
This also makes the performance much closer to the original code.
Author
Contributor

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 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.
Arye Ramaty added 1 commit 2024-02-03 23:34:15 +01:00
efe2563c4e support realize depth input
Added support for using a new 'realize all' field. Also, the selection input is not working properly. The only missing part is supporting collections as instances; currently, they are getting flattened to 'keep everything' geometry set compatible. It shouldn't be too hard to support that.
Author
Contributor

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

  1. Currently, nested collections are still realized recursively. I couldn't figure out why attempting to mimic the implementation in the collection info node has resulted in a memory management issue.
  2. I think there is a memory leak somewhere, probably in the execute_instances_tasks function.
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 1. Currently, nested collections are still realized recursively. I couldn't figure out why attempting to mimic the implementation in the collection info node has resulted in a memory management issue. 2. I think there is a memory leak somewhere, probably in the `execute_instances_tasks` function.
Arye Ramaty added 2 commits 2024-02-06 12:59:36 +01:00
08bf47e5aa support collection as instances
The current state was that collections were realized recursively.
Now they mimic the behavior of the collection info node
Author
Contributor

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.

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.
Arye Ramaty changed title from WIP:Add selection and depth options to realize instances to Add selection and depth options to realize instances 2024-02-06 13:06:51 +01:00
Arye Ramaty added 1 commit 2024-02-06 13:30:11 +01:00
Iliya Katushenock reviewed 2024-02-06 13:46:39 +01:00
@ -237,2 +237,4 @@
const GeometryComponent &component)>;
bool attribute_foreach(Span<GeometryComponent::Type> component_types,
bool include_instances,

Redundant parameter?

Redundant parameter?
Author
Contributor

Removed it.

Removed it.
aryeramaty marked this conversation as resolved
@ -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.

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?

`int current_depth, int depth_target` -> `IndexRange depth_to_check`?
Author
Contributor

Hmm... But what happens when the depth_target is negative? I use it to create an endless loop. Can this be achieved with IndexRange?

Hmm... But what happens when the `depth_target` is negative? I use it to create an endless loop. Can this be achieved with `IndexRange`?

std::numeric_limits<int>::max()?
Not sure if this is good idea to use negative end value for positive loops in any way tbh/

`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.

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.
Author
Contributor

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.

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?

What for this copy is done?
Author
Contributor

Because the orignal component is a const, but probably there is a better way to do that.
What should I do?

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

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.

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:

/* Instance and it level. */
Vector<std::pair<int, const Instances *>> instances_to_check;

// instances_to_check.append({0, this->get_instances()});
or
// for (child : top_instance.childs()) {
//   if (depth_to_check.contains(level_of_child_instance))
//     instances_to_check.append({instance_depth[for_this_instance], child});
// }

while (!instances_to_check.is_empty()) {
  const auto &&[level_of_instance, instance] = instances_to_check.pop();
  for (child_instances....) {
    ...
    if (depth_to_check.contains(level_of_child_instance)) {
      instances_to_check.append({level_of_child_instance, child_instance});
    }
  }
}

Plus: Why index mask used there at all? This looks like it would be much easier to use something like Pseudocode: ```Cpp /* Instance and it level. */ Vector<std::pair<int, const Instances *>> instances_to_check; // instances_to_check.append({0, this->get_instances()}); or // for (child : top_instance.childs()) { // if (depth_to_check.contains(level_of_child_instance)) // instances_to_check.append({instance_depth[for_this_instance], child}); // } while (!instances_to_check.is_empty()) { const auto &&[level_of_instance, instance] = instances_to_check.pop(); for (child_instances....) { ... if (depth_to_check.contains(level_of_child_instance)) { instances_to_check.append({level_of_child_instance, child_instance}); } } } ```
@ -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.

Comments should not paraphrase next line.
aryeramaty marked this conversation as resolved
@ -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.

Not really useful comment.
aryeramaty marked this conversation as resolved
@ -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..

Better do describe why here is `skip` instead of `// Check for a special instance condition.`.
aryeramaty marked this conversation as resolved
@ -14,1 +14,4 @@
void join_attributes(const Span<const bke::GeometryComponent *> src_components,
bke::GeometryComponent &result,
const Span<StringRef> ignored_attributes);
1. Const for trivial argument in function declaration. https://developer.blender.org/docs/handbook/guidelines/c_cpp/#const 2. Order of arguments. https://developer.blender.org/docs/handbook/guidelines/c_cpp/#function-arguments
Contributor

ignored_attributes need to be last becouse of the default value

ignored_attributes need to be last becouse of the default value

It is necessary to have default value (see bke::gather_attributes as instance)?

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()

`99` -> `std::numeric_limits<int>::max()`
aryeramaty marked this conversation as resolved
@ -286,3 +314,4 @@
}
};
static void realize_collections(Collection *collection, bke::Instances* instances){

bke::Instances collection_to_instance(const Collection &collection, const bool recursive)?

~~`bke::Instances collection_to_instance(const Collection &collection, const bool recursive)`?~~
aryeramaty marked this conversation as resolved
@ -477,3 +529,1 @@
index++;
}
FOREACH_COLLECTION_OBJECT_RECURSIVE_END;
Collection *collection_ptr = &reference.collection();

What for this change?

What for this change?
Author
Contributor

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 the ensure_geometry_instances but keeps the collection hierarchy
Should I move it there?

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 the `ensure_geometry_instances` but keeps the collection hierarchy Should 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();
bke::Instances gathered_collections = ...;
const bke::GeometrySet colleciton_geometry = bke::GeometrySet::from_instances(&gathered_collections, bke::GeometryOwnershipType::Editable);
```Cpp bke::Instances gathered_collections = ...; const bke::GeometrySet colleciton_geometry = bke::GeometrySet::from_instances(&gathered_collections, bke::GeometryOwnershipType::Editable); ```
Author
Contributor

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.

Collection *collection_ptr = &reference.collection();
bke::Instances gathered_collections = realize_collections(collection_ptr);
const bke::GeometrySet colleciton_geometry = bke::GeometrySet::from_instances(
          &gathered_collections, bke::GeometryOwnershipType::Editable);
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. ``` Collection *collection_ptr = &reference.collection(); bke::Instances gathered_collections = realize_collections(collection_ptr); const bke::GeometrySet colleciton_geometry = bke::GeometrySet::from_instances( &gathered_collections, bke::GeometryOwnershipType::Editable); ```
Arye Ramaty added 4 commits 2024-02-06 14:46:56 +01:00
Arye Ramaty added 2 commits 2024-02-13 08:28:25 +01:00
Arye Ramaty added 2 commits 2024-02-18 16:41:22 +01:00
Arye Ramaty added 2 commits 2024-02-20 13:13:35 +01:00
73b64daff1 Fix some logic
If at least one from all the Instances has a child with a relevant attribute -> include the attributes from the current Instances domain
Arye Ramaty added 1 commit 2024-02-25 14:49:24 +01:00
Arye Ramaty added 2 commits 2024-02-28 15:32:43 +01:00
Arye Ramaty added 1 commit 2024-03-03 12:02:39 +01:00
ef43c9db4d Move specialised function to the geometry module
The PR adds two variations to the functions  'attribute_foreach' and 'gather_attributes_for_propagation'.
I decided it's better to move them into the realize_instances.cc file instead of the 'geometry_set.cc' due to how specific they are.
Arye Ramaty added 1 commit 2024-03-03 17:39:50 +01:00
f421076caa Introduce 'geometry_set_from_reference' for better abstraction.
Splits 'foreach_geometry_in_reference' logic into 'geometry_set_from_reference' for improved code clarity and modularity.
Minor adjustments to related functions for consistency.
Arye Ramaty added 1 commit 2024-03-10 12:25:18 +01:00
Arye Ramaty added 1 commit 2024-03-14 14:13:09 +01:00
e62ed2622f davidhaver-WIP-realize-depth
memory leak fix

Co-authored-by: david.haver <d.n.h.s.b.j@gmail.com>
Pull Request: #1
Arye Ramaty added 12 commits 2024-03-21 14:55:28 +01:00

@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).

@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).
Iliya Katushenock reviewed 2024-03-24 12:13:10 +01:00
Iliya Katushenock left a comment
Member

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.

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.
/**
 * Same #realize_instances but ...
 */
```Cpp /** * Same #realize_instances but ... */ ```
aryeramaty marked this conversation as resolved
@ -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())

`IndexMask(geometry_set.get_instances()->instances_num())`
aryeramaty marked this conversation as resolved
@ -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.

`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.
Contributor

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?

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_input<decl::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).

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).
aryeramaty marked this conversation as resolved
@ -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) (or max(...)) functions for socket declaration is just soft limits. So value == 0 -> value <= 0.

`min(0)` (or `max(...)`) functions for socket declaration is just soft limits. So `value == 0` -> `value <= 0`.
Contributor

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

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
aryeramaty marked this conversation as resolved
@ -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.

`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.
aryeramaty marked this conversation as resolved
@ -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);

`evaluator.get_evaluated<int>(evaluated_depth_index);` -> `evaluator.get_evaluated<int>(0);`
Contributor

`/**

  • \param field: Field to add to the evaluator.
  • \param varray_ptr: Once #evaluate is called, the resulting virtual array will be will be
  • assigned to the given position.
  • \return Index of the field in the evaluator which can be used in the #get_evaluated methods.
    */`
    according to this add return value should be used as get_evaluated input
`/** * \param field: Field to add to the evaluator. * \param varray_ptr: Once #evaluate is called, the resulting virtual array will be will be * assigned to the given position. * \return Index of the field in the evaluator which can be used in the #get_evaluated methods. */` 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.

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.
Contributor

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

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
Member

Do you think just splitting this node function in separate node with more clear inputs (instead of Selection, Realize All, Depth, ...) might be better?

@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.

> Do you think just splitting this node function in separate node with more clear inputs (instead of Selection, Realize All, Depth, ...) might be better? @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.
Arye Ramaty added 1 commit 2024-03-25 10:52:16 +01:00

@SimonThommes Well, i'm okay with Top and Bottom 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).

1, 2, 3 nesting levels 1 and 2 level selections 2 level selection (all upper levels can be collapsed or no..)
image image image

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.

@SimonThommes Well, i'm okay with `Top` and `Bottom` 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). | 1, 2, 3 nesting levels | 1 and 2 level selections | 2 level selection (all upper levels can be collapsed or no..) | | -- | -- | -- | | ![image](/attachments/fb5df523-cd32-46e6-96f7-0d0810ac0393) | ![image](/attachments/0ac6360d-1d04-40ce-bafa-103bbe539eb4) | ![image](/attachments/4ff6beea-be2c-4fb6-b7e4-3abcdcba1aa7) | 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.
5.9 KiB
8.0 KiB
5.7 KiB
Member

@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 (?)

@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 for Realize Instances in just separate one node) as Realize Instances with Depth field input. Maybe is there a case to use Separate 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.

@SimonThommes Probably yes, `Separate Instances in Depth` might be the same (i'm actually started this thread from idea to split this new functionality for `Realize Instances` in just separate one node) as `Realize Instances` with `Depth` field input. Maybe is there a case to use `Separate 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.
Member

@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 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?
Author
Contributor

@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:

  1. What is the meaning of 'top selected instance' if you separate them from their 'reference'? Would that result in an empty point cloud?
  2. The concept of selecting from the bottom (analogous to leaves) instead of the top (trunk) seems a little complicated. The same instance may have multiple depth levels. For example, a single instance can hold a nested instance and a regular instance, which means its 'bottom depth' could be either 2 or 3 depending on the child instance we decide to start from.
  3. What is the difference between a separated child instance that is 'detached' from its parent and a child instance that enters the same state by realizing its parent?
@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: 1. What is the meaning of 'top selected instance' if you separate them from their 'reference'? Would that result in an empty point cloud? 2. The concept of selecting from the bottom (analogous to leaves) instead of the top (trunk) seems a little complicated. The same instance may have multiple depth levels. For example, a single instance can hold a nested instance and a regular instance, which means its 'bottom depth' could be either 2 or 3 depending on the child instance we decide to start from. 3. What is the difference between a separated child instance that is 'detached' from its parent and a child instance that enters the same state by realizing its parent?

@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 of Separate Instances in Depth).

@aryeramaty

  1. Not sure how to add selection to 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.
  2. See first part of this message.
  3. Not sure i get that.
@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 of `Separate Instances in Depth`). @aryeramaty 1. Not sure how to add selection to `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. 2. See first part of this message. 3. Not sure i get that.
Jacques Lucke requested review from Jacques Lucke 2024-04-01 12:41:07 +02:00
Jacques Lucke requested review from Hans Goudey 2024-04-01 12:41:07 +02:00
Arye Ramaty added 1 commit 2024-04-06 21:45:02 +02:00
Arye Ramaty added 1 commit 2024-04-10 12:18:58 +02:00
7990d27f12 Fix: Transformation Parent -> child
Apply the rotation and scale of the parent instance to the location of the child instance
Arye Ramaty added 1 commit 2024-04-15 16:31:56 +02:00
Arye Ramaty added 1 commit 2024-04-15 16:35:13 +02:00
Hans Goudey requested changes 2024-04-15 21:19:32 +02:00
Hans Goudey left a comment
Member

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.

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,
Member

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.

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.
Contributor

Sure.
@mod_moder requested that change in a prior review.
Can be reverted here and be done in a later PR.

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.

Ah, if this was original order of args, i might made mistake.
aryeramaty marked this conversation as resolved
@ -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()) {
Member

What's the reason to change this to creating and returning a GeometrySet?

What's the reason to change this to creating and returning a `GeometrySet`?
Contributor

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.

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.
aryeramaty marked this conversation as resolved
@ -226,2 +220,4 @@
};
struct AllInstancesInfo {
/** store an array of void pointer to attributes for each component. */
Member

store -> Stores

`store` -> `Stores`
aryeramaty marked this conversation as resolved
@ -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,
Member

Returning a reference to the output argument seems unnecessary and potentially confusing. I'd just return a bke::GeometrySet by value here

Returning a reference to the output argument seems unnecessary and potentially confusing. I'd just return a `bke::GeometrySet` by value here
Contributor

Isn't it more costly to create a local GeometrySet and then copy it twice in the return than to use an output param?.

Isn't it more costly to create a local GeometrySet and then copy it twice in the return than to use an output param?.
Member

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.

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.
Contributor

Ok didn't see that it is basically a wrapped pointer.

Ok didn't see that it is basically a wrapped pointer.
aryeramaty marked this conversation as resolved
@ -466,0 +524,4 @@
r_geometry_set = bke::GeometrySet(); // Return an empty GeometrySet for None type
break;
}
default: {
Member

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.

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.
aryeramaty marked this conversation as resolved
@ -679,0 +794,4 @@
}
}
/*Flag to track if any relevant attributes were found.*/
Member

Missing space after /* here and in other places

Missing space after `/*` here and in other places
aryeramaty marked this conversation as resolved
@ -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(
Member

void static -> static void

`void static` -> `static void`
aryeramaty marked this conversation as resolved
@ -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();
Member

What is going on here? Looks like the attribute writer doesn't actually do anything

What is going on here? Looks like the attribute writer doesn't actually do anything
Contributor

I am hoping its clearer now. this section makes sure generic output attributes exists so later code can be better optimaized.

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");
Member

Realize all levels of nested instances for a top-level instances

`Realize all levels of nested instances for a top-level instances`
Author
Contributor

What about:
Realize all levels of nested instances for a top-level instances.\nNote: Override the value of the Depth input

What about: `Realize all levels of nested instances for a top-level instances.\nNote: Override the value of the Depth input`
Member

Nice! I'd change "Override" to "Overrides" and probably remove the newline and "Note:" to simplify things. But otherwise, seems perfect.

Nice! I'd change "Override" to "Overrides" and probably remove the newline and "Note:" to simplify things. But otherwise, seems perfect.
aryeramaty marked this conversation as resolved
Simon Thommes approved these changes 2024-04-16 14:47:42 +02:00
Simon Thommes left a comment
Member

Functionality is working great!

I'd suggest mentioning in the tooltip that the Depth input is not used when the Realize All input is true, since that might be confusing

The 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.

Functionality is working great! I'd suggest mentioning in the tooltip that the `Depth` input is not used when the `Realize All` input is true, since that might be confusing The 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.
Hans Goudey changed title from Add selection and depth options to realize instances to Geometry Nodes: Add selection and depth options to realize instances 2024-04-16 14:57:02 +02:00
Arye Ramaty requested review from Hans Goudey 2024-04-22 11:58:03 +02:00
Member

Looks like the patch hasn't been updated. Maybe you forgot to push your changes?

Looks like the patch hasn't been updated. Maybe you forgot to push your changes?
Contributor

@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?.

@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?.
Author
Contributor

Indeed, if you look at the commit history you can see the last commits by David.

Indeed, if you look at the commit history you can see the last commits by David.
Jacques Lucke requested changes 2024-04-25 13:34:19 +02:00
Jacques Lucke left a comment
Member

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 and gather_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.

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` and `gather_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 {
Member

...OptionS

Variable names for this type should generally also have the s at the end.

`...OptionS` Variable names for this type should generally also have the `s` at the end.
aryeramaty marked this conversation as resolved
@ -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)
Member

Use references instead of pointers.

Use references instead of pointers.
aryeramaty marked this conversation as resolved
@ -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));
Member

Unnecessary parenthesis, same above and below.

Unnecessary parenthesis, same above and below.
aryeramaty marked this conversation as resolved
@ -378,3 +413,3 @@
}
static void create_result_ids(const RealizeInstancesOptions &options,
static void create_result_ids(bool keep_original_ids,
Member

add const

add `const`
aryeramaty marked this conversation as resolved
@ -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;
Member

Remove break after return.

Remove `break` after `return`.
aryeramaty marked this conversation as resolved
@ -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 :
Member

const bool is_top_level = ...
The variable can also be used below.

`const bool is_top_level = ...` The variable can also be used below.
aryeramaty marked this conversation as resolved
@ -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()) {
Member

Use indices.foreach_* to iterate over index masks, it's more efficient than using operator[] many times.

Use `indices.foreach_*` to iterate over index masks, it's more efficient than using `operator[]` many times.
aryeramaty marked this conversation as resolved
@ -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;
Member

depth_target -> child_target_depth. That's better than having target_depth and depth_target as variable names in the same scope.

`depth_target` -> `child_target_depth`. That's better than having `target_depth` and `depth_target` as variable names in the same scope.
aryeramaty marked this conversation as resolved
@ -679,0 +758,4 @@
const Span<bke::GeometryComponent::Type> component_types,
const int current_depth,
const int depth_target,
const VArray<int> instance_depth,
Member

Use reference.

Use reference.
aryeramaty marked this conversation as resolved
@ -679,0 +764,4 @@
{
/* Initialize flag to track if child instances have the specified components.*/
bool child_has_component = true;
Member

Why is this set to true?

Why is this set to `true`?
aryeramaty marked this conversation as resolved
@ -679,0 +773,4 @@
const IndexMask indices = (0 == current_depth) ?
selection :
IndexMask(IndexRange(instances.instances_num()));
for (const int index : indices.index_range()) {
Member

Use indices.foreach_*.

Use `indices.foreach_*`.
aryeramaty marked this conversation as resolved
@ -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;
Member

child_depth_target maybe?

`child_depth_target` maybe?
aryeramaty marked this conversation as resolved
@ -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,
Member

Seems like we should be able to break out of the loop early when child_has_component is true.

Seems like we should be able to break out of the loop early when `child_has_component` is true.
Author
Contributor

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.

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.
Member

I see, maybe just add a comment mentioning that breaking out of this loop early is not correct.

I see, maybe just add a comment mentioning that breaking out of this loop early is not correct.
JacquesLucke marked this conversation as resolved
@ -679,0 +792,4 @@
}
/* Flag to track if any relevant attributes were found.*/
bool is_relevant = false;
Member

any_attribute_found

`any_attribute_found`
aryeramaty marked this conversation as resolved
@ -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*/
Member

Missing dot and whitespace at end of comment.

Missing dot and whitespace at end of comment.
aryeramaty marked this conversation as resolved
@ -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 ==
Member

What does "main one" mean?
Sounds like the variable should be called something like is_main_instance_component.

What does "main one" mean? Sounds like the variable should be called something like `is_main_instance_component`.
aryeramaty marked this conversation as resolved
@ -679,0 +811,4 @@
return true;
});
is_relevant = true;
Member

Seems like this should be moved into the lambda above.

Seems like this should be moved into the lambda above.
aryeramaty marked this conversation as resolved
@ -679,0 +826,4 @@
* current_depth, depth_target, instance_depth and selection.
*/
static void gather_attributes_for_propagation(
bke::GeometrySet re_geometry_set,
Member

What does re stand for?

What does `re` stand for?
aryeramaty marked this conversation as resolved
@ -679,0 +907,4 @@
options.propagation_info,
attributes_to_propagate);
attributes_to_propagate.remove("position");
attributes_to_propagate.remove("radius");
Member

Does removing radius and position here have a purpose (still)? The position attribute was removed a while ago, and the radius attribute never had a special meaning on instances.

Does removing `radius` and `position` here have a purpose (still)? The position attribute was removed a while ago, and the radius attribute never had a special meaning on instances.
aryeramaty marked this conversation as resolved
@ -679,0 +930,4 @@
return;
}
VArray<blender::float4x4>::ForSpan(src_base_transforms);
Member

This line does nothing.

This line does nothing.
aryeramaty marked this conversation as resolved
@ -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];
Member

Use reference.

Use reference.
aryeramaty marked this conversation as resolved
@ -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;
Member

Use Span.

Use `Span`.
aryeramaty marked this conversation as resolved
@ -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];
Member

Use const.

Use `const`.
aryeramaty marked this conversation as resolved
@ -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
Member

Comment style. Also, do you have a specific example where this condition might be true?

Comment style. Also, do you have a specific example where this condition might be true?
Author
Contributor

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.

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));
Member

Can use cpp_type->fill_assign_n instead of array_utils::copy which is a bit better here I think.

Can use `cpp_type->fill_assign_n` instead of `array_utils::copy` which is a bit better here I think.
aryeramaty marked this conversation as resolved
@ -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) {
Member

typo compent

typo `compent`
aryeramaty marked this conversation as resolved
@ -679,0 +1011,4 @@
for_join_attributes.append(compent.get());
}
join_attributes(
Member

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.

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.
aryeramaty marked this conversation as resolved
@ -771,3 +1118,3 @@
static void execute_realize_pointcloud_task(
const RealizeInstancesOptions &options,
bool keep_original_ids,
Member

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.

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.
aryeramaty marked this conversation as resolved
@ -1597,0 +1957,4 @@
*/
static void propagate_instances_to_keep(
const bke::GeometrySet &geometry_set,
IndexMask selection,
Member

Use const reference.

Use const reference.
aryeramaty marked this conversation as resolved
@ -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());
Member

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.

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.
aryeramaty marked this conversation as resolved
@ -23,0 +51,4 @@
},
mf::build::exec_presets::AllSpanOrSingle());
Field<int> depth_field_overrided(FieldOperation::Create(
Member

overrided -> overridden

`overrided` -> `overridden`
aryeramaty marked this conversation as resolved
Arye Ramaty added 17 commits 2024-04-30 15:14:43 +02:00
f023d86bcc Why is child_has_component set to true?
Comment #10 (The original reason disappeared along the road :)
3698126688 is_special_instance -> is_main_instance_component
Comment #16
Also a better comment about the intention behind the code
f158008114 Does removing radius and position here have a purpose (still)?
Comment #19
I think there is no longer any need for that. I don't sure why the radius was here at all, probably I just copy pasted from the point domain and forgot to remove the radius...
Arye Ramaty added 9 commits 2024-04-30 16:53:52 +02:00
Arye Ramaty added 5 commits 2024-05-01 14:16:14 +02:00
8e79221c61 revert keep_original_ids -> options
Comment 28
This would be easier to add more parameters in the future, also, the specific function doesn't really care about the id
Member

Please run make format if you haven't recently.

Please run `make format` if you haven't recently.
Arye Ramaty added 2 commits 2024-05-01 14:55:56 +02:00
093dc1b4c0 Do you have a specific example where this condition might be true?
Comment 24
I pretty sure there was some thing that may have caused it to be true, but I couldn't think about any of them...
So I removed it for the time being.
Author
Contributor

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.

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.
Jacques Lucke approved these changes 2024-05-01 21:20:49 +02:00
Jacques Lucke left a comment
Member

Looks good to me now (except for small inline comments), thanks for spending all this time on this patch!

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,
Member

This should probably take the geometry by const reference.

This should probably take the geometry by const reference.
aryeramaty marked this conversation as resolved
@ -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());
Member

Materialized is the default preset and does not have to be passed in, can just be omitted. Same in the other file.

`Materialized` is the default preset and does not have to be passed in, can just be omitted. Same in the other file.
aryeramaty marked this conversation as resolved
Arye Ramaty added 2 commits 2024-05-02 07:52:25 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
e0b1bb8a45
Take the geometry by const reference.
Member

@blender-bot build

@blender-bot build
Hans Goudey approved these changes 2024-05-02 20:20:45 +02:00
Hans Goudey left a comment
Member

This seems reasonable. Thanks for all your work here. Just noticed two tiny comment style things.

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
Member

Comment style: /* Return an empty GeometrySet for None type. */

Comment style: `/* Return an empty GeometrySet for None type. */`
aryeramaty marked this conversation as resolved
@ -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
Member

/*Check -> /* Check

`/*Check` -> `/* Check`
aryeramaty marked this conversation as resolved
Arye Ramaty added 1 commit 2024-05-03 01:28:03 +02:00
Jacques Lucke merged commit 1db538683d into main 2024-05-03 12:48:12 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
7 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#116582
No description provided.