GPv3: Lock unselect materials operator #115278

Antonio Vazquez · 2023-11-22T17:57:28+01:00

Antonio Vazquez commented

2023-11-22 17:57:28 +01:00

Conversion of GPv2 operator

Antonio Vazquez added 2 commits 2023-11-22 17:57:33 +01:00

c6c25f0308 GPv3: Lock Unselected Materials Operator

This is the same functionality of GPv2 version

9c247b9721 Cleanup include lines

Antonio Vazquez requested review from Hans Goudey 2023-11-22 17:57:52 +01:00

Antonio Vazquez requested review from Falk David 2023-11-22 17:58:00 +01:00

Antonio Vazquez requested review from Matias Mendiola 2023-11-22 17:58:08 +01:00

Antonio Vazquez added this to the Grease Pencil project 2023-11-22 17:58:16 +01:00

Antonio Vazquez referenced this pull request

2023-11-22 17:58:53 +01:00

GPv3: Material panel operators #114997

Antonio Vazquez commented

2023-11-22 17:59:49 +01:00

@HooglyBoogly Please review the loop of the Strokes IndexMask because I don't know if there is any way in C++ to exit of the loop.

Hans Goudey commented

2023-11-22 19:10:12 +01:00

Sure, I'll make sure there's a good way to do that check.

Hans Goudey commented

2023-11-22 20:18:53 +01:00

Ah, this is simple, (was on my phone before) you can just return from the loop. Though, this shouldn't create the VArraySpan without first handling the case where the attribute doesn't exist (you can use VArray::get_if_single() for that). VArraySpan always creates an array, this is wasteful if there is only a single value.

However, the time complexity isn't great here if there are many materials, since we iterate through all curves for every material. Would be better to just iterate once. I tried that here (code hasn't been tested though!):

static VectorSet<int> find_materials_in_mask(const VArray<int> &material_indices,
                                             const IndexMask &mask)
{
  VectorSet<int> materials;
  if (const std::optional<int> single = material_indices.get_if_single()) {
    materials.add(*single);
  }
  else {
    mask.foreach_index([&](const int i) { materials.add(material_indices[i]); });
  }
}

    const VArray<int> material_indices = *attributes.lookup_or_default<int>(
        "material_index", ATTR_DOMAIN_CURVE, 0);
    const VectorSet<int> selected_materials = find_materials_in_mask(material_indices, strokes);

    for (const int material_index : IndexRange(object->totcol)) {
      if (selected_materials.contains(material_index)) {
        continue;
      }

      if (Material *ma = BKE_object_material_get(object, material_index + 1)) {
        MaterialGPencilStyle &gp_style = *ma->gp_style;
        gp_style.flag |= GP_MATERIAL_LOCKED;
        DEG_id_tag_update(&ma->id, ID_RECALC_COPY_ON_WRITE);
        changed = true;
      }

Ah, this is simple, (was on my phone before) you can just `return` from the loop. Though, this shouldn't create the `VArraySpan` without first handling the case where the attribute doesn't exist (you can use `VArray::get_if_single()` for that). `VArraySpan` always creates an array, this is wasteful if there is only a single value. However, the time complexity isn't great here if there are many materials, since we iterate through all curves for every material. Would be better to just iterate once. I tried that here (code hasn't been tested though!): ```Cpp static VectorSet<int> find_materials_in_mask(const VArray<int> &material_indices, const IndexMask &mask) { VectorSet<int> materials; if (const std::optional<int> single = material_indices.get_if_single()) { materials.add(*single); } else { mask.foreach_index([&](const int i) { materials.add(material_indices[i]); }); } } ``` ```Cpp const VArray<int> material_indices = *attributes.lookup_or_default<int>( "material_index", ATTR_DOMAIN_CURVE, 0); const VectorSet<int> selected_materials = find_materials_in_mask(material_indices, strokes); for (const int material_index : IndexRange(object->totcol)) { if (selected_materials.contains(material_index)) { continue; } if (Material *ma = BKE_object_material_get(object, material_index + 1)) { MaterialGPencilStyle &gp_style = *ma->gp_style; gp_style.flag |= GP_MATERIAL_LOCKED; DEG_id_tag_update(&ma->id, ID_RECALC_COPY_ON_WRITE); changed = true; } ```

Antonio Vazquez commented

2023-11-22 20:28:59 +01:00

@HooglyBoogly Thanks for the code. I see now how you avoid the loop creating a temp array. I'm going to test the code and fix a return missing.

I still need to get used to the way of thinking in C++.

@HooglyBoogly Thanks for the code. I see now how you avoid the loop creating a temp array. I'm going to test the code and fix a return missing. I still need to get used to the way of thinking in C++.

Antonio Vazquez added 1 commit 2023-11-22 23:01:07 +01:00

65edcec2db GPv3: Cleanup loop

Antonio Vazquez added 1 commit 2023-11-23 16:06:28 +01:00

316cc8a47d Merge branch 'main' into GPv3_mat_lock_unselect

Conflicts:
	scripts/startup/bl_ui/properties_material_gpencil.py
	source/blender/editors/grease_pencil/intern/grease_pencil_material.cc

Antonio Vazquez added 1 commit 2023-11-23 19:11:25 +01:00

ac448e5e2d GPv3: Fix error locking materials

The lock cannot be done inside the loop because the
selected strokes would be wrong.

Now, the list of used materials is saved in a Vector
and when the loop ends, the list is used to lock the
unused materials.

Antonio Vazquez added 1 commit 2023-11-23 19:20:31 +01:00

be8bd0e7a3 GPv3: Cleanup comments

Antonio Vazquez added 1 commit 2023-11-23 19:22:43 +01:00

441043aaa7 GPv3: Refine comments

Antonio Vazquez added 1 commit 2023-11-23 19:29:21 +01:00

60c0d54c22 GPv3: Refine comments II

Matias Mendiola approved these changes 2023-11-23 19:43:57 +01:00

Antonio Vazquez added 1 commit 2023-11-24 10:54:20 +01:00

e14986782d GPv3: Use blender::Vector instead of standard Vector

Hans Goudey requested changes 2023-11-27 02:03:59 +01:00

source/blender/editors/grease_pencil/intern/grease_pencil_material.cc Outdated

						
				@ -256,0 +287,4 @@

				  bool changed = false;

				  const Array<MutableDrawingInfo> drawings = retrieve_editable_drawings(*scene, grease_pencil);

				  blender::Vector<int> materials_used;

Hans Goudey commented

2023-11-27 02:02:03 +01:00

blender:: is unnecessary here, this code is already using the Blender namespace. std::vector is what you were thinking of. You can check by using the ctrl-click functionality in your IDE.

`blender::` is unnecessary here, this code is already using the Blender namespace. `std::vector` is what you were thinking of. You can check by using the ctrl-click functionality in your IDE.

antoniov marked this conversation as resolved

source/blender/editors/grease_pencil/intern/grease_pencil_material.cc Outdated

						
				@ -256,0 +304,4 @@

				    for (const int material_index : IndexRange(object->totcol)) {

				      /* Add the material as used to not be blocked. */

				      if (selected_materials.contains(material_index)) {

Hans Goudey commented

2023-11-27 02:03:56 +01:00

Vector::contains has linear performance, that's not good if there's many drawings and many materials. Instead, change the VectorSet to a Set, and pass it as an argument to find_materials_in_mask instead of creating a new one every time.

`Vector::contains` has linear performance, that's not good if there's many drawings and many materials. Instead, change the `VectorSet` to a `Set`, and pass it as an argument to `find_materials_in_mask` instead of creating a new one every time.

antoniov marked this conversation as resolved

Antonio Vazquez added 2 commits 2023-11-27 10:34:17 +01:00

429e769fbe Merge branch 'main' into GPv3_mat_lock_unselect

b292e037af GPv3: Cleanup style and fix comment

Antonio Vazquez added 1 commit 2023-11-28 11:54:56 +01:00

bf1c08a537 GPv3: Simplify loops and use `Set`instead of `VectorSet`

Falk David approved these changes 2023-11-28 11:59:13 +01:00

Falk David left a comment

Had some comments, like removing the static function alltogether. Antonio already made the changes.
This looks good to me now.

Had some comments, like removing the static function alltogether. Antonio already made the changes. This looks good to me now.

Hans Goudey reviewed 2023-11-28 16:28:32 +01:00

source/blender/editors/grease_pencil/intern/grease_pencil_material.cc

						
				@ -256,0 +320,4 @@

				  /* Identifiers. */

				  ot->name = "Lock Unselected Materials";

				  ot->idname = "GREASE_PENCIL_OT_material_lock_unselected";

				  ot->description = "Lock any material not used in any selected stroke";

Hans Goudey commented

2023-11-28 16:28:26 +01:00

Seems like this is now doing the opposite, locking materials used by selected strokes

Antonio Vazquez added 1 commit 2023-11-28 16:35:57 +01:00

c8b14f404c GPv3: Fix Locked used instead of unused

This was introduced by error in previous commit

Hans Goudey approved these changes 2023-11-28 16:37:02 +01:00

Antonio Vazquez merged commit b9fba005bd into main

2023-11-28 16:39:08 +01:00

Antonio Vazquez referenced this issue from a commit

2023-11-28 16:39:09 +01:00

GPv3: Lock unselect materials operator

Antonio Vazquez deleted branch GPv3_mat_lock_unselect

2023-11-28 16:39:11 +01:00

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: Lock unselect materials operator #115278