GPv3: Reimplement how customdata is updated when layers are re-ordered #113962

Douglas Paul · 2023-10-20T08:45:38+02:00

Douglas Paul commented

2023-10-20 08:45:38 +02:00

Instead of calculating the expected insertion index for a customdata entry being moved, this implementation just records the initial layer indices so that it can compare them with the final indices to determine how the customdata entries need to be re-arranged.

This PR supersedes #113874. If this one is merged, that one should be closed.

The first commit fixes a bug where grow_or_init_customdata() was calculating on an insertion index when it should not have been. Since GreasePencil::add_layer() always adds the new layer at the top, grow_or_init_custom_data() should always add the new customdata entry at the end.

The second commit is the focus of this PR. With the above bug fixed, the only thing that the insertion index was used for was to prepare a mapping from the old layer indices to the new ones (to be passed to reorder_customdata()). This implementation instead builds that map based on a "snapshot" of the layer indices taken before the layers are re-ordered. That eliminates the need to calculate an insertion index at all, which allows a lot of logic to be deleted.

Another benefit of this approach is that it works just as well if multiple layers are moved at the same time—it should be able to support the movement of entire layer groups.

Instead of calculating the expected insertion index for a customdata entry being moved, this implementation just records the initial layer indices so that it can compare them with the final indices to determine how the customdata entries need to be re-arranged. --- This PR supersedes #113874. If this one is merged, that one should be closed. The first commit fixes a bug where `grow_or_init_customdata()` was calculating on an insertion index when it should not have been. Since `GreasePencil::add_layer()` always adds the new layer at the top, `grow_or_init_custom_data()` should always add the new customdata entry at the end. The second commit is the focus of this PR. With the above bug fixed, the only thing that the insertion index was used for was to prepare a mapping from the old layer indices to the new ones (to be passed to `reorder_customdata()`). This implementation instead builds that map based on a "snapshot" of the layer indices taken before the layers are re-ordered. That eliminates the need to calculate an insertion index at all, which allows a lot of logic to be deleted. Another benefit of this approach is that it works just as well if multiple layers are moved at the same time—it should be able to support the movement of entire layer groups.

❤️ 1

Douglas Paul added 2 commits 2023-10-20 08:45:54 +02:00

20292b016d Fix grow_customdata() to always insert at the end

Previously, grow_customdata() accepted a parameter for the location
at which the new entry should be inserted, but that was not
appropriate because GreasePencil::add_layer() always adds the new
layer at the top. (The layer is separately moved to the correct
position in the tree via GreasePencil::move_node_after() when
appropriate.)

1c8d11283a Reimplement how customdata is updated when layers are re-ordered

Instead of calculating the expected insertion index for a
customdata entry being moved, this implementation just records the
initial layer indices so that it can compare them with the final
indices to determine how the customdata entries need to be re-
arranged.

Douglas Paul reviewed 2023-10-20 08:50:58 +02:00

Douglas Paul reviewed 2023-10-20 09:04:47 +02:00

Douglas Paul referenced this pull request

2023-10-20 09:15:31 +02:00

GPv3: Fix bugs in find_layer_insertion_index() and some of its usages #113874

Douglas Paul requested review from Falk David 2023-10-20 09:15:53 +02:00

Douglas Paul commented

2023-10-20 09:32:25 +02:00

Bah, I can't figure out how to do comments on multiple lines of code in Gitea, so I'll just ask my questions here:

Could the implementation of grow_customdata() be simplified even further now that it only ever inserts at the end?
In my new implementation of reorder_layer_data(), is it too risky to use pointers as map keys? I don't know how likely or unlikely it would be for an an operation that changes the order of the layers to move them around in memory. If that's a concern, all that's really needed is a unique identifier each layer before & after the layer order is changed.

Bah, I can't figure out how to do comments on multiple lines of code in Gitea, so I'll just ask my questions here: 1. Could the implementation of `grow_customdata()` be simplified even further now that it only ever inserts at the end? 2. In my new implementation of `reorder_layer_data()`, is it too risky to use pointers as map keys? I don't know how likely or unlikely it would be for an an operation that changes the order of the layers to move them around in memory. If that's a concern, all that's really needed is a unique identifier each layer before & after the layer order is changed.

Falk David requested changes 2023-10-20 10:54:00 +02:00

Falk David left a comment

Thanks for working on this, makes the code so much cleaner :)

I think there is one bug in your code right now that needs fixing. And I left some smaller cleanup comments.

Thanks for working on this, makes the code so much cleaner :) I think there is one bug in your code right now that needs fixing. And I left some smaller cleanup comments.

source/blender/blenkernel/intern/grease_pencil.cc Outdated

						
				@ -2113,3 +2067,4 @@

				static void grow_or_init_customdata(GreasePencil *grease_pencil)

				{

				  using namespace blender;

				  const Span<const bke::greasepencil::Layer *> layers = grease_pencil->layers();

Falk David commented

2023-10-20 10:43:06 +02:00

I think with what you're going for, this if/else can just be replaced by CustomData_realloc(&grease_pencil->layers_data, layers.size(), layers.size() + 1); and the static function grow_customdata is no longer needed.

I think with what you're going for, this `if`/`else` can just be replaced by `CustomData_realloc(&grease_pencil->layers_data, layers.size(), layers.size() + 1);` and the static function `grow_customdata` is no longer needed.

Douglas Paul commented

2023-10-21 15:13:29 +02:00

Great, thanks. With this change grow_or_init_customdata() is now just 2 simple lines, so maybe not worth its weight as a function? Not sure where the Blender team lands on the tradeoffs on that sort of thing. I've left it for now.

Great, thanks. With this change `grow_or_init_customdata()` is now just 2 simple lines, so maybe not worth its weight as a function? Not sure where the Blender team lands on the tradeoffs on that sort of thing. I've left it for now.

Falk David commented

2023-10-21 15:41:12 +02:00

Yea and grow_or_init_customdata isn't used in many places either, so we can remove that one now too :)
I'm happy to see that we can remove a lot of code complexity here 👍

Yea and `grow_or_init_customdata` isn't used in many places either, so we can remove that one now too :) I'm happy to see that we can remove a lot of code complexity here 👍

👍 1

source/blender/blenkernel/intern/grease_pencil.cc Outdated

						
				@ -2128,3 +2082,3 @@

				  using namespace blender;

				  std::string unique_name = unique_layer_name(*this, name);

				  grow_or_init_customdata(this, parent_group);

				  grow_or_init_customdata(this);

Falk David commented

2023-10-20 10:41:22 +02:00

This now grows the custom data by one, but the layer might not be at the end. It's added to the end of the parent_group which doesn't have to be the root. So I assume that there needs to be some reorder_custom_data call here as well that shifts things in the right place.

This now grows the custom data by one, but the layer might not be at the end. It's added to the end of the `parent_group` which doesn't have to be the root. So I assume that there needs to be some `reorder_custom_data` call here as well that shifts things in the right place.

Douglas Paul commented

2023-10-21 15:33:46 +02:00

Ack, yes, very glad you caught this because I had completely forgotten to deal with it.

Most places that add a layer were adding the layer to the root group anyway, so I added some signatures of add_layer() that implicitly add the layer to the root group (and append the custom data at the end). And I made these existing signatures of add_layer() (that accept a group) into basically convenience methods: They just add the layer to the root group and then move it into the requested group via move_node_into().

Ack, yes, very glad you caught this because I had completely forgotten to deal with it. Most places that add a layer were adding the layer to the root group anyway, so I added some signatures of `add_layer()` that implicitly add the layer to the root group (and append the custom data at the end). And I made these existing signatures of `add_layer()` (that accept a group) into basically convenience methods: They just add the layer to the root group and then move it into the requested group via `move_node_into()`.

Falk David commented

2023-10-21 15:42:06 +02:00

Right, I just read through the changes and I like the way you resolved this. Just calling move_node_into for the convinience function makes a lot of sense.

Right, I just read through the changes and I like the way you resolved this. Just calling `move_node_into` for the convinience function makes a lot of sense.

source/blender/blenkernel/intern/grease_pencil.cc

						
				@ -2184,2 +2128,2 @@

				    array_utils::fill_index_range(reorder_indices.slice(IndexRange(reorder_from + 1, dist)),

				                                  reorder_from);

				  /* Stash the initial layer order that we can refer back to later */

				  auto old_layer_index_by_layer = Map<const bke::greasepencil::Layer *, int>();

Falk David commented

2023-10-20 10:45:52 +02:00

We like to avoid auto when possible. This can just be Map<const bke::greasepencil::Layer *, int> old_layer_index_by_layer;

We like to avoid `auto` when possible. This can just be `Map<const bke::greasepencil::Layer *, int> old_layer_index_by_layer;`

Douglas Paul commented

2023-10-21 15:36:20 +02:00

Good to know, thanks. You might consider adding that to the style guide, especially since the default linter settings in some IDEs (or at least mine) nudge towards using auto wherever possible.

Good to know, thanks. You might consider adding that to the [style guide](https://wiki.blender.org/wiki/Style_Guide/C_Cpp), especially since the default linter settings in some IDEs (or at least mine) nudge towards using auto wherever possible.

filedescriptor marked this conversation as resolved

source/blender/blenkernel/intern/grease_pencil.cc

						
				@ -2204,0 +2140,4 @@

				  /* Compose the mapping from old layer indices to new layer indices */

				  Array<int> new_by_old_map(layers.size());

				  int i_new = 0;

				  for (const bke::greasepencil::Layer *layer : layers) {

Falk David commented

2023-10-20 10:50:44 +02:00

for (const int layer_i_new : layers.index_range()) {
    const bke::greasepencil::Layer *layer = layers[layer_i_new];
    BLI_assert(old_layer_index_by_layer.contains(layer));
    const int layer_i_old = old_layer_index_by_layer.pop(layer);
    new_by_old_map[layer_i_old] = layer_i_new;
}

``` for (const int layer_i_new : layers.index_range()) { const bke::greasepencil::Layer *layer = layers[layer_i_new]; BLI_assert(old_layer_index_by_layer.contains(layer)); const int layer_i_old = old_layer_index_by_layer.pop(layer); new_by_old_map[layer_i_old] = layer_i_new; } ```

Douglas Paul commented

2023-10-21 15:37:49 +02:00

Indeed! An embarrassing oversight

filedescriptor marked this conversation as resolved

Falk David commented

2023-10-20 10:59:53 +02:00

Bah, I can't figure out how to do comments on multiple lines of code in Gitea, so I'll just ask my questions here:

It's a bit weird but you can click the plus on a line, then "Start Review", add all your comments and then in the upper right where it says "Review", just press the "Comment" button.

Could the implementation of grow_customdata() be simplified even further now that it only ever inserts at the end?

Yes, I left a comment in the code about it.

In my new implementation of reorder_layer_data(), is it too risky to use pointers as map keys? I don't know how likely or unlikely it would be for an an operation that changes the order of the layers to move them around in memory. If that's a concern, all that's really needed is a unique identifier each layer before & after the layer order is changed.

Because the layer tree is implemented with links and linked-lists, I don't think this is a problem. Reordering should only ever change the link pointers and shouldn't reallocate anything. Using the pointers as keys is ok.

> Bah, I can't figure out how to do comments on multiple lines of code in Gitea, so I'll just ask my questions here: It's a bit weird but you can click the plus on a line, then "Start Review", add all your comments and then in the upper right where it says "Review", just press the "Comment" button. > 1. Could the implementation of `grow_customdata()` be simplified even further now that it only ever inserts at the end? Yes, I left a comment in the code about it. > 2. In my new implementation of reorder_layer_data(), is it too risky to use pointers as map keys? I don't know how likely or unlikely it would be for an an operation that changes the order of the layers to move them around in memory. If that's a concern, all that's really needed is a unique identifier each layer before & after the layer order is changed. Because the layer tree is implemented with links and linked-lists, I don't think this is a problem. Reordering should only ever change the link pointers and shouldn't reallocate anything. Using the pointers as keys is ok.

Douglas Paul added 4 commits 2023-10-21 14:57:29 +02:00

17060f6c6e Simplify implementation of grow_or_init_customdata()

e10f8b5651 Apply code style fixes from the code review

81e0201f22 Avoid unnecessarily adding a layer to the non-root group

There was no need to add the new layer to the active layer's group
instead of the root group, because the new layer will be moved to the
correct place via the subsequent call to move_node_after() anyway.

buildbot/vexp-code-patch-coordinator Build done. Details

3bdb22dfea Refactor so adding a layer to the root group is the typical case

and adding to a specific group is a convenience method

Falk David commented

2023-10-21 15:06:17 +02:00

@blender-bot build

Douglas Paul reviewed 2023-10-21 16:04:49 +02:00

source/blender/blenkernel/intern/grease_pencil.cc Outdated

						
				@ -2121,2 +2073,2 @@

				    grow_customdata(grease_pencil->layers_data, insertion_index, layers.size());

				  }

				  blender::bke::greasepencil::Layer &new_layer = add_layer(name);

				  move_node_into(new_layer.as_node(), parent_group);

Douglas Paul commented

2023-10-21 16:03:11 +02:00

If someone happened to call this method and pass in the root group as parent_group, then this call to move_node_into() is just wasted effort. I could add a check to avoid that, but all we'd really be doing is optimizing a case where the calling code was using the wrong method signature (because they should instead use the signature that implicitly adds to the root group).

If someone happened to call this method and pass in the root group as `parent_group`, then this call to `move_node_into()` is just wasted effort. I could add a check to avoid that, but all we'd really be doing is optimizing a case where the calling code was using the wrong method signature (because they should instead use the signature that implicitly adds to the root group).

Falk David commented

2023-10-21 16:22:23 +02:00

Yea optimizing a case that the function shouldn't be used for in the first place seems like a waste. Better keep it simple.

source/blender/blenkernel/intern/grease_pencil.cc Outdated

						
				@ -2205,0 +2147,4 @@

				  BLI_assert(old_layer_index_by_layer.is_empty());

				  /* Use the mapping to re-order the custom data */

				  reorder_customdata(grease_pencil.layers_data, new_by_old_map);

Douglas Paul commented

2023-10-21 15:58:14 +02:00

It's possible that the new layer order is exactly the same as the old layer order, in which case we could skip this call to reorder_customdata(). It's tempting to detect that case and skip this call as an optimization, but I'm not sure there's any practical benefit to doing that? It would arguably arguably just "complexity creep" to optimize things that don't really need to be optimized.

It's possible that the new layer order is exactly the same as the old layer order, in which case we could skip this call to `reorder_customdata()`. It's tempting to detect that case and skip this call as an optimization, but I'm not sure there's any practical benefit to doing that? It would arguably arguably just "complexity creep" to optimize things that don't really need to be optimized.

Falk David commented

2023-10-21 16:23:51 +02:00

Agreed. We might come back to this if it is an issue for some reason, but I can't see why it would be a bottleneck right now.

Douglas Paul added 1 commit 2023-10-21 16:23:56 +02:00

292a1893ca Inline grow_or_init_customdata()

Falk David approved these changes 2023-10-21 16:27:47 +02:00

Falk David left a comment

Code looks good now :) I'll test this locally once more, but I think we can merge it after that goes well.

Falk David merged commit f5c9acc154 into main

2023-10-21 16:47:25 +02:00

Falk David referenced this issue from a commit

2023-10-21 16:47:26 +02:00

GPv3: Reimplement how customdata is updated when layers are re-ordered

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GPv3: Reimplement how customdata is updated when layers are re-ordered #113962