Cleanup: Geometry: Some unification in working with attributes in realize instances #108821

Iliya Katushenock · 2023-06-09T20:12:43+02:00

Iliya Katushenock commented

2023-06-09 20:12:43 +02:00

Split loops to separate them by their conceptual meaning.
Separate the copying of attributes into a new function.

Split loops to separate them by their conceptual meaning. Separate the copying of attributes into a new function.

Iliya Katushenock added the

Interest

Geometry Nodes

label 2023-06-09 20:12:43 +02:00

Iliya Katushenock added 1 commit 2023-06-09 20:12:55 +02:00

67b8dbe1ad init

Iliya Katushenock added this to the Nodes & Physics project 2023-06-09 20:13:00 +02:00

Iliya Katushenock added 1 commit 2023-06-09 20:14:52 +02:00

f7c2012567 progress

Iliya Katushenock requested review from Hans Goudey 2023-06-09 20:15:58 +02:00

Hans Goudey commented

2023-06-09 20:27:28 +02:00

Could you explain how this is better in the PR description? Right now it just looks like a bunch of code getting shuffled around.

Iliya Katushenock commented

2023-06-09 20:38:27 +02:00

Basically, it's just a shuffled around code.
I was doing support for a new type of component and noticed that the code has a lot of duplication, so I decided to at least make it simpler. As an example: separate a component-specific loop from a nested loop.
One could say that this allows you to take the check out of the loop, but I don’t think that this would be a good improvement as a main advantage.
I also introduced a new function, which gave unification.

Basically, it's just a shuffled around code. I was doing support for a new type of component and noticed that the code has a lot of duplication, so I decided to at least make it simpler. As an example: separate a component-specific loop from a nested loop. One could say that this allows you to take the check out of the loop, but I don’t think that this would be a good improvement as a main advantage. I also introduced a new function, which gave unification.

Hans Goudey requested changes 2023-06-09 20:43:10 +02:00

source/blender/geometry/intern/realize_instances.cc

						
				@ -716,0 +730,4 @@

				  }

				  if (info.create_radius_attribute) {

				    for (const int pointcloud_index : info.realize_info.index_range()) {

Hans Goudey commented

2023-06-09 20:42:44 +02:00

Don't add three separate loops over all the point clouds, that adds code duplication for no real purpose.

Same for the other geometry components.

Don't add three separate loops over all the point clouds, that adds code duplication for no real purpose. Same for the other geometry components.

Iliya Katushenock commented

2023-06-09 20:46:12 +02:00

When I said nesting, I meant...

  ...
  if (info.create_radius_attribute) {
    pointcloud_info.radii = *attributes.lookup_or_default("radius", ATTR_DOMAIN_POINT, 0.01f);
  }
}

And

if (info.create_radius_attribute) {
  for (const int pointcloud_index : info.realize_info.index_range()) {
    pointcloud_info.radii = *attributes.lookup_or_default("radius", ATTR_DOMAIN_POINT, 0.01f);
  }
}

Although I see some code duplication, does it still seem easier to separate this into a separate processing step?

When I said nesting, I meant... ```Cpp ... if (info.create_radius_attribute) { pointcloud_info.radii = *attributes.lookup_or_default("radius", ATTR_DOMAIN_POINT, 0.01f); } } ``` And ```Cpp if (info.create_radius_attribute) { for (const int pointcloud_index : info.realize_info.index_range()) { pointcloud_info.radii = *attributes.lookup_or_default("radius", ATTR_DOMAIN_POINT, 0.01f); } } ``` Although I see some code duplication, does it still seem easier to separate this into a separate processing step?

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Cleanup: Geometry: Some unification in working with attributes in realize instances #108821

Pull request closed