blender
/
blender
136
412
Fork 596
Code Issues 6.3k Pull Requests 776 Projects 19 Wiki Activity

GPv3: Set selected curve material as active material #114188

Merged
Falk David merged 12 commits from antoniov/blender:GPv3_Set_selected_as_actmat into main 2023-11-08 15:00:59 +01:00
Conversation 3 Commits 12 Files Changed 2 +53
2 changed files with 53 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
scripts/startup/bl_ui/space_view3d.py
View File

@ -5833,6 +5833,10 @@ class VIEW3D_MT_edit_greasepencil_stroke(Menu):
layout.separator()
layout.operator("grease_pencil.set_active_material")
layout.separator()
layout.operator("grease_pencil.cyclical_set", text="Toggle Cyclic").type = 'TOGGLE'

49
source/blender/editors/grease_pencil/intern/grease_pencil_edit.cc
View File

@ -957,6 +957,54 @@ static void GREASE_PENCIL_OT_cyclical_set(wmOperatorType *ot)
/** \} */
/* -------------------------------------------------------------------- */
/** \name Set selected material as active material
* \{ */
static int grease_pencil_set_active_material_exec(bContext *C, wmOperator * /*op*/)
antoniov marked this conversation as resolved Outdated
Pratik Borhade commented 2023-10-29 22:44:18 +01:00
Outdated
Review
Copy Link

I guess /*layer_index*/ ? :)

I guess `/*layer_index*/` ? :)
{
using namespace blender;
const Scene *scene = CTX_data_scene(C);
Object *object = CTX_data_active_object(C);
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(object->data);
if (object->totcol == 0) {
return OPERATOR_CANCELLED;
}
const Array<MutableDrawingInfo> drawings = retrieve_editable_drawings(*scene, grease_pencil);
antoniov marked this conversation as resolved Outdated
Hans Goudey commented 2023-10-30 21:10:43 +01:00
Outdated
Review
Copy Link

material_index should be retrieved with the default of 0 rather than -1. That's the standard default value used for all integer attributes, and it means we refer to the first material when there is no attribute.

`material_index` should be retrieved with the default of `0` rather than `-1`. That's the standard default value used for all integer attributes, and it means we refer to the first material when there is no attribute.
for (const MutableDrawingInfo &info : drawings) {
bke::CurvesGeometry &curves = info.drawing.strokes_for_write();
Falk David commented 2023-11-01 10:19:48 +01:00
Outdated
Review
Copy Link

It's not relevant for this PR but I find it a bit strange that the operator just looks at the first selected curve here. I wonder if a better approach would be something like pick_material where we find the closest curve and set its material as the active one. See ed_grease_pencil_select_pick. Or we finally introduce an active element for the point and curve domain. Then this operator would set the active material based on the material of the active curve.

It's not relevant for this PR but I find it a bit strange that the operator just looks at the first selected curve here. I wonder if a better approach would be something like `pick_material` where we find the closest curve and set its material as the active one. See `ed_grease_pencil_select_pick`. Or we finally introduce an active element for the point and curve domain. Then this operator would set the active material based on the material of the active curve.
Matias Mendiola commented 2023-11-01 12:30:56 +01:00
Outdated
Review
Copy Link

This operator was introduced before Grease Pencil had the eyedropper tool.
Now that we have an eyedropper tool, we can use it with CTRL or SHIFT to pick over a stroke and change the active material instead of creating a new one. That way this operator will no longer be necessary.

This operator was introduced before Grease Pencil had the eyedropper tool. Now that we have an eyedropper tool, we can use it with CTRL or SHIFT to pick over a stroke and change the active material instead of creating a new one. That way this operator will no longer be necessary.
Antonio Vazquez commented 2023-11-01 12:36:04 +01:00
Outdated
Review
Copy Link

I'm not sure the eyedropper is the replace for this operator. First, this is not easy to discover and maybe you want use the actual selection.

IMHO the best solution would be to implement the concept of "active stroke"...in this way, you can select something and assign the material base on active item, not first one as is now.

I'm not sure the eyedropper is the replace for this operator. First, this is not easy to discover and maybe you want use the actual selection. IMHO the best solution would be to implement the concept of "active stroke"...in this way, you can select something and assign the material base on active item, not first one as is now.
Matias Mendiola commented 2023-11-01 12:43:54 +01:00
Outdated
Review
Copy Link

Agree, an "active stroke" is very necessary and not only for this, but in my experience teaching Grease Pencil, artists always try to use the eyedropper to get the active material, only to realize later that what it does is create new materials/colors. Maybe we should have both: tool and operator for that functionality, but it's something to discuss outside of this PR :)

Agree, an "active stroke" is very necessary and not only for this, but in my experience teaching Grease Pencil, artists always try to use the eyedropper to get the active material, only to realize later that what it does is create new materials/colors. Maybe we should have both: tool and operator for that functionality, but it's something to discuss outside of this PR :)
Antonio Vazquez commented 2023-11-01 13:21:27 +01:00
Outdated
Review
Copy Link

Yes, I'm talking about having both operators.

Yes, I'm talking about having both operators.
Pratik Borhade commented 2023-11-07 12:46:19 +01:00
Outdated
Review
Copy Link

yes, concept of "active stroke" will resolve this situation.

just looks at the first selected curve here

I also found this bit confusing while creating the task.

yes, concept of "active stroke" will resolve this situation. > just looks at the first selected curve here I also found this bit confusing while creating the task.
Hans Goudey commented 2023-11-08 09:20:13 +01:00
Review
Copy Link

Use continue instead of indenting the rest of the loop

Use `continue` instead of indenting the rest of the loop
antoniov marked this conversation as resolved Outdated
Hans Goudey commented 2023-10-30 21:12:14 +01:00
Outdated
Review
Copy Link

Use selected_curves.first() rather than looping and stopping at the first iteration.

Use `selected_curves.first()` rather than looping and stopping at the first iteration.
IndexMaskMemory memory;
antoniov marked this conversation as resolved Outdated
Hans Goudey commented 2023-10-30 21:11:22 +01:00
Outdated
Review
Copy Link

We should never store -1 as a material index on geometry-- the values are always means to be greater than or equal to 0. So you shouldn't need to check that here.

We should never store `-1` as a material index on geometry-- the values are always means to be greater than or equal to 0. So you shouldn't need to check that here.
IndexMask selected_curves = ed::curves::retrieve_selected_curves(curves, memory);
antoniov marked this conversation as resolved Outdated
Falk David commented 2023-11-01 10:21:02 +01:00
Outdated
Review
Copy Link

This return doesn't do anything. The lambda function returns here anyways. We would have to introduce a way of returning a value that would exit from the loop in foreach_editable_drawing. Then we don't need the

if (changed) {
   return;
}

at the top.

This `return` doesn't do anything. The lambda function returns here anyways. We would have to introduce a way of returning a value that would exit from the loop in `foreach_editable_drawing`. Then we don't need the ``` if (changed) { return; } ``` at the top.
Hans Goudey commented 2023-11-08 09:20:44 +01:00
Review
Copy Link

This check can be moved to the beginning of the exec callback

This check can be moved to the beginning of the exec callback
if (selected_curves.is_empty()) {
continue;
}
const blender::VArray<int> materials = *curves.attributes().lookup_or_default<int>(
"material_index", ATTR_DOMAIN_CURVE, 0);
object->actcol = materials[selected_curves.first()] + 1;
Falk David commented 2023-11-07 14:02:53 +01:00
Outdated
Review
Copy Link

I see that there is not depsgraph tag here. Have you tested it? Is it updating correctly?

I see that there is not depsgraph tag here. Have you tested it? Is it updating correctly?
Antonio Vazquez commented 2023-11-07 15:39:30 +01:00
Outdated
Review
Copy Link

Yes, I tested. The tag is not needed because what it is changed here is the active material in the object and the notifier is enough here.

Yes, I tested. The tag is not needed because what it is changed here is the active material in the object and the notifier is enough here.
break;
};
WM_event_add_notifier(C, NC_GEOM | ND_DATA | NA_EDITED, &grease_pencil);
return OPERATOR_FINISHED;
}
static void GREASE_PENCIL_OT_set_active_material(wmOperatorType *ot)
{
ot->name = "Set Active Material";
ot->idname = "GREASE_PENCIL_OT_set_active_material";
ot->description = "Set the selected stroke material as the active material";
ot->exec = grease_pencil_set_active_material_exec;
ot->poll = editable_grease_pencil_poll;
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
}
/** \} */
} // namespace blender::ed::greasepencil
void ED_operatortypes_grease_pencil_edit()
@ -968,6 +1016,7 @@ void ED_operatortypes_grease_pencil_edit()
WM_operatortype_append(GREASE_PENCIL_OT_delete_frame);
WM_operatortype_append(GREASE_PENCIL_OT_stroke_material_set);
WM_operatortype_append(GREASE_PENCIL_OT_cyclical_set);
WM_operatortype_append(GREASE_PENCIL_OT_set_active_material);
}
void ED_keymap_grease_pencil(wmKeyConfig *keyconf)