From 5497bdff10222472b60702884b210b9b4dfb72ce Mon Sep 17 00:00:00 2001 From: Douglas Paul Date: Tue, 17 Oct 2023 23:50:13 -0400 Subject: [PATCH 1/2] GPv3: Fix bugs in find_layer_insertion_index() and some of its usages --- .../blenkernel/intern/grease_pencil.cc | 114 +++++++++++++++--- 1 file changed, 96 insertions(+), 18 deletions(-) diff --git a/source/blender/blenkernel/intern/grease_pencil.cc b/source/blender/blenkernel/intern/grease_pencil.cc index 478b3c706eb..6ae44fed134 100644 --- a/source/blender/blenkernel/intern/grease_pencil.cc +++ b/source/blender/blenkernel/intern/grease_pencil.cc @@ -2022,6 +2022,10 @@ static void grow_customdata(CustomData &data, const int insertion_index, const i data = new_data; } +/** + * \returns the index of the layer to insert above when above == true, otherwise the index of the + * layer to insert below + */ static int find_layer_insertion_index( const blender::Span layers, const blender::bke::greasepencil::LayerGroup &group, @@ -2037,26 +2041,89 @@ static int find_layer_insertion_index( if (!group.as_node().parent_group()) { return 0; } + + /* The target group is empty (and not the root group), so we need to search either upward or + * downward through the nodes to find the nearest layer. */ bke::greasepencil::LayerGroup &parent_group = *group.as_node().parent_group(); const Span nodes = parent_group.nodes(); - int index = nodes.first_index(&group.as_node()); - while (index > 0 && index < nodes.size() - 1) { - if (nodes[index]->is_layer()) { - break; - } - if (above) { - index++; - } - else { - index--; + int node_index = nodes.first_index(&group.as_node()); + + if (above) { + /* Search upward through the nodes for a layer or non-empty group to insert below. + * Since above == true, the insertion will happen above the layer whose index we return. + * NOTE: This means we need to return a value of -1 in the case where the insertion point + * should be below the 0th layer (i.e., the caller should insert above the -1st layer). */ + + /* We know that node_index currently refers to an empty group and that we need to search + * upward, so we increment node_index here to start the search at the first node above our + * starting point (if any). */ + node_index++; + while (node_index < nodes.size()) { + if (nodes[node_index]->is_layer()) { + /* We've been searching upward and found a layer above the group we're inserting into, so + * we want to insert just below this layer we found. Since the insertion will happen above + * the index we return, we need to subtract 1 from the index of the layer we found. */ + int found_layer_index = layers.first_index(&nodes[node_index]->as_layer()); + return found_layer_index - 1; + } + + BLI_assert(nodes[node_index]->is_group()); + const bke::greasepencil::LayerGroup &found_group = nodes[node_index]->as_group(); + if (!found_group.layers().is_empty()) { + /* We've been searching upward and found a non-empty group above the group we're inserting + * into, so we want to insert just below the lowest layer in that group we found. Since the + * insertion will happen above the index we return, we need to subtract 1 from the index of + * that layer. */ + return layers.first_index(found_group.layers().first()) - 1; + } + + /* The node we're currently visiting must be an empty group, so we increment node_index to + * continue searching upward (if there are any more nodes above this one to visit). */ + node_index++; } + + /* We finished the upward search without finding a layer or non-empty group, so the insertion + * point should be above the highest layer. So we return the index of the highest layer. */ + return layers.size() - 1; } - index = math::clamp(index, 0, int(layers.size() - 1)); - return index; + + /* Search downward through the nodes for a layer or non-empty group to insert above. + * Since above == false, the insertion will happen below the layer whose index we return. + * NOTE: This means we need to return a value equal to layers.size() in the case where the + * insertion point should be above the highest layer. */ + + /* We know that node_index currently refers to an empty group and that we need to search + * downward, so we decrement node_index here to start the search at the first node below our + * starting point (if any). */ + node_index--; + while (node_index >= 0) { + if (nodes[node_index]->is_layer()) { + /* We've been searching downward and found a layer below the group we're inserting into, so + * we want to insert just above this layer we found. Since the insertion will happen below + * the index we return, we need to add 1 to the index of the layer we found. */ + int found_layer_index = layers.first_index(&nodes[node_index]->as_layer()); + return found_layer_index + 1; + } + + BLI_assert(nodes[node_index]->is_group()); + const bke::greasepencil::LayerGroup &found_group = nodes[node_index]->as_group(); + if (!found_group.layers().is_empty()) { + /* We've been searching downward and found a non-empty group below the group we're inserting + * into, so we want to insert just above the highest layer in that group we found. Since the + * insertion will happen below the index we return, we need to add 1 to the index of that + * layer. */ + return layers.first_index(found_group.layers().last()) + 1; + } + + node_index--; + } + + /* We finished the downward search without finding a layer or non-empty group, so the insertion + * point should be below the 0th layer. */ + return 0; } -static void grow_or_init_customdata(GreasePencil *grease_pencil, - const blender::bke::greasepencil::LayerGroup &parent_group) +static void grow_or_init_customdata(GreasePencil *grease_pencil) { using namespace blender; const Span layers = grease_pencil->layers(); @@ -2064,7 +2131,8 @@ static void grow_or_init_customdata(GreasePencil *grease_pencil, CustomData_realloc(&grease_pencil->layers_data, 0, 1); } else { - int insertion_index = find_layer_insertion_index(layers, parent_group, false); + /* New layers are always added at the highest position and subsequently moved into place. */ + int insertion_index = layers.size() - 1; grow_customdata(grease_pencil->layers_data, insertion_index, layers.size()); } } @@ -2074,7 +2142,7 @@ blender::bke::greasepencil::Layer &GreasePencil::add_layer( { using namespace blender; std::string unique_name = unique_layer_name(*this, name); - grow_or_init_customdata(this, parent_group); + grow_or_init_customdata(this); return parent_group.add_layer(unique_name); } @@ -2084,7 +2152,7 @@ blender::bke::greasepencil::Layer &GreasePencil::add_layer( { using namespace blender; std::string unique_name = unique_layer_name(*this, duplicate_layer.name()); - grow_or_init_customdata(this, parent_group); + grow_or_init_customdata(this); bke::greasepencil::Layer &new_layer = parent_group.add_layer(duplicate_layer); this->update_drawing_users_for_layer(new_layer); new_layer.set_name(unique_name); @@ -2171,7 +2239,10 @@ void GreasePencil::move_node_up(blender::bke::greasepencil::TreeNode &node, cons } else if (target_node.is_group()) { const bke::greasepencil::LayerGroup &group = target_node.as_group(); - to_index = layers.first_index(group.layers().last()); + to_index = find_layer_insertion_index(layers, group, true); + } + if (from_index > to_index) { + to_index++; } reorder_layer_data(*this, from_index, to_index); @@ -2204,6 +2275,9 @@ void GreasePencil::move_node_down(blender::bke::greasepencil::TreeNode &node, co const bke::greasepencil::LayerGroup &group = target_node.as_group(); to_index = find_layer_insertion_index(layers, group, false); } + if (to_index > from_index) { + to_index--; + } reorder_layer_data(*this, from_index, to_index); } @@ -2329,6 +2403,10 @@ void GreasePencil::move_node_into(blender::bke::greasepencil::TreeNode &node, const Span layers = this->layers(); const int from_index = layers.first_index(&node.as_layer()); int to_index = find_layer_insertion_index(layers, parent_group, true); + if (from_index > to_index) { + to_index++; + } + reorder_layer_data(*this, from_index, to_index); } if (node.is_group()) { -- 2.30.2 From 135b848cd9514344f659139efa858a63c4b2ef1c Mon Sep 17 00:00:00 2001 From: Douglas Paul Date: Wed, 18 Oct 2023 01:12:25 -0400 Subject: [PATCH 2/2] GPv3: Refactor to reduce redundancy & risk --- .../blenkernel/intern/grease_pencil.cc | 86 ++++++++----------- 1 file changed, 37 insertions(+), 49 deletions(-) diff --git a/source/blender/blenkernel/intern/grease_pencil.cc b/source/blender/blenkernel/intern/grease_pencil.cc index 6ae44fed134..c9fc64e6cb4 100644 --- a/source/blender/blenkernel/intern/grease_pencil.cc +++ b/source/blender/blenkernel/intern/grease_pencil.cc @@ -2026,7 +2026,7 @@ static void grow_customdata(CustomData &data, const int insertion_index, const i * \returns the index of the layer to insert above when above == true, otherwise the index of the * layer to insert below */ -static int find_layer_insertion_index( +static int find_layer_insertion_index_for_target_group( const blender::Span layers, const blender::bke::greasepencil::LayerGroup &group, const bool above = true) @@ -2123,6 +2123,36 @@ static int find_layer_insertion_index( return 0; } +/** + * \returns the index of the layer to insert above when above == true, otherwise the index of the + * layer to insert below + */ +static int find_layer_insertion_index( + const blender::Span layers, + const blender::bke::greasepencil::TreeNode &target_node, + const int from_index, + const bool above = true) +{ + using namespace blender; + int to_index = -1; + if (target_node.is_layer()) { + to_index = layers.first_index(&target_node.as_layer()); + } + else if (target_node.is_group()) { + const bke::greasepencil::LayerGroup &group = target_node.as_group(); + to_index = find_layer_insertion_index_for_target_group(layers, group, above); + } + + if (above && from_index > to_index) { + to_index++; + } + else if (!above && to_index > from_index) { + to_index--; + } + + return to_index; +} + static void grow_or_init_customdata(GreasePencil *grease_pencil) { using namespace blender; @@ -2233,17 +2263,7 @@ void GreasePencil::move_node_up(blender::bke::greasepencil::TreeNode &node, cons BLI_findlinkfrom(reinterpret_cast(&node), step)) ->wrap(); const int from_index = layers.first_index(&node.as_layer()); - int to_index = -1; - if (target_node.is_layer()) { - to_index = layers.first_index(&target_node.as_layer()); - } - else if (target_node.is_group()) { - const bke::greasepencil::LayerGroup &group = target_node.as_group(); - to_index = find_layer_insertion_index(layers, group, true); - } - if (from_index > to_index) { - to_index++; - } + const int to_index = find_layer_insertion_index(layers, target_node, from_index, true); reorder_layer_data(*this, from_index, to_index); } @@ -2267,17 +2287,7 @@ void GreasePencil::move_node_down(blender::bke::greasepencil::TreeNode &node, co BLI_findlinkfrom(reinterpret_cast(&node), -step)) ->wrap(); const int from_index = layers.first_index(&node.as_layer()); - int to_index = -1; - if (target_node.is_layer()) { - to_index = layers.first_index(&target_node.as_layer()); - } - else if (target_node.is_group()) { - const bke::greasepencil::LayerGroup &group = target_node.as_group(); - to_index = find_layer_insertion_index(layers, group, false); - } - if (to_index > from_index) { - to_index--; - } + const int to_index = find_layer_insertion_index(layers, target_node, from_index, false); reorder_layer_data(*this, from_index, to_index); } @@ -2339,17 +2349,7 @@ void GreasePencil::move_node_after(blender::bke::greasepencil::TreeNode &node, const Span layers = this->layers(); const int from_index = layers.first_index(&node.as_layer()); - int to_index = -1; - if (target_node.is_layer()) { - to_index = layers.first_index(&target_node.as_layer()); - } - else if (target_node.is_group()) { - const bke::greasepencil::LayerGroup &group = target_node.as_group(); - to_index = find_layer_insertion_index(layers, group, true); - } - if (from_index > to_index) { - to_index++; - } + const int to_index = find_layer_insertion_index(layers, target_node, from_index, true); reorder_layer_data(*this, from_index, to_index); } @@ -2371,17 +2371,7 @@ void GreasePencil::move_node_before(blender::bke::greasepencil::TreeNode &node, const Span layers = this->layers(); const int from_index = layers.first_index(&node.as_layer()); - int to_index = -1; - if (target_node.is_layer()) { - to_index = layers.first_index(&target_node.as_layer()); - } - else if (target_node.is_group()) { - const bke::greasepencil::LayerGroup &group = target_node.as_group(); - to_index = find_layer_insertion_index(layers, group, false); - } - if (to_index > from_index) { - to_index--; - } + const int to_index = find_layer_insertion_index(layers, target_node, from_index, false); reorder_layer_data(*this, from_index, to_index); } @@ -2402,10 +2392,8 @@ void GreasePencil::move_node_into(blender::bke::greasepencil::TreeNode &node, if (node.is_layer()) { const Span layers = this->layers(); const int from_index = layers.first_index(&node.as_layer()); - int to_index = find_layer_insertion_index(layers, parent_group, true); - if (from_index > to_index) { - to_index++; - } + const int to_index = find_layer_insertion_index( + layers, parent_group.as_node(), from_index, true); reorder_layer_data(*this, from_index, to_index); } -- 2.30.2