GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool. #119039

Merged
Falk David merged 93 commits from casey-bianco-davis/blender:GPv3-Primitive-Tools into main 2024-04-22 10:48:16 +02:00

Adds the primitive tools to GPv3.

Unlike the legacy operator, this allow for the view port to be moved while in the operator.
This also adds rotation and scale sub-operators with r and s keybinds.
Also all control points are editable after extruding.

Adds the primitive tools to GPv3. Unlike the legacy operator, this allow for the view port to be moved while in the operator. This also adds rotation and scale sub-operators with `r` and `s` keybinds. Also all control points are editable after extruding.
casey-bianco-davis added the
Interest
Grease Pencil
Module
Grease Pencil
labels 2024-03-04 03:02:17 +01:00
casey-bianco-davis added 31 commits 2024-03-04 03:02:30 +01:00
casey-bianco-davis added this to the Grease Pencil project 2024-03-04 03:02:34 +01:00
casey-bianco-davis requested review from Falk David 2024-03-04 03:04:26 +01:00
Falk David requested review from Lukas Tönne 2024-03-04 10:39:14 +01:00
Lukas Tönne requested changes 2024-03-04 17:44:31 +01:00
@ -8077,0 +8090,4 @@
{"space_type": 'VIEW_3D', "region_type": 'WINDOW'},
{"items": [
("grease_pencil.primitive_line", params.tool_maybe_tweak_event,
{"properties": [("wait_for_input", False)]}),
Member

The wait_for_input property is not defined for any of the new operators, i get a bunch of warnings:

Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_line'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_line'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_line'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_polyline'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_polyline'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_box'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_box'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_box'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_circle'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_circle'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_circle'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_curve'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_curve'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_curve'
Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_curve'
The `wait_for_input` property is not defined for any of the new operators, i get a bunch of warnings: ``` Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_line' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_line' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_line' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_polyline' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_polyline' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_box' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_box' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_box' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_circle' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_circle' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_circle' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_curve' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_curve' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_curve' Warning: property 'wait_for_input' not found in item 'GREASE_PENCIL_OT_primitive_curve' ```
casey-bianco-davis marked this conversation as resolved
@ -1441,0 +1454,4 @@
if props is not None:
row = layout.row(align=True)
row.prop(props, "interpolate_mode", icon="VIEW_ORTHO", text="", invert_checkbox=True)
Member

This should be an actual enum property instead of a bool, and then use expand=True in the ui code to draw as toggle buttons rather than drawing two buttons for the same property.

This should be an actual enum property instead of a bool, and then use `expand=True` in the ui code to draw as toggle buttons rather than drawing two buttons for the same property.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +79,4 @@
/* The points that are at the end of segments. */
JOIN_POINT = 0,
/* The points inside of the segments. */
EXTRINSIC_POINT = 1,
Member

It's not clear to me what "extrinsic" means (not a native speaker), any more intuitive names we might use?

It's not clear to me what "extrinsic" means (not a native speaker), any more intuitive names we might use?
casey-bianco-davis marked this conversation as resolved
@ -0,0 +122,4 @@
InterpolationMode interpolate_mode;
float4x4 projection;
/* Helper class to project screen space coordinates to 3D. */
ed::greasepencil::DrawingPlacement placement_;
Member

For a simple public struct like this, don't use underscores at the end, they should only be used for private fields. Also other fields in this struct.
https://developer.blender.org/docs/handbook/guidelines/c_cpp/#class-data-member-names

For a simple public struct like this, don't use underscores at the end, they should only be used for private fields. Also other fields in this struct. https://developer.blender.org/docs/handbook/guidelines/c_cpp/#class-data-member-names
casey-bianco-davis marked this conversation as resolved
@ -0,0 +181,4 @@
ColorGeometry4f color_gizmo_primary;
ColorGeometry4f color_gizmo_secondary;
ColorGeometry4f color_gizmo_a;
UI_GetThemeColor4fv(TH_REDALERT, color_redalert);
Member

Using gizmo theme colors seems ok, but the REDALERT should probably not be used like a regular theme color. If necessary we can add more theme colors.

Using gizmo theme colors seems ok, but the `REDALERT` should probably not be used like a regular theme color. If necessary we can add more theme colors.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +703,4 @@
float4(0.0f);
srgb_to_linearrgb_v4(ptd.vertex_color_, ptd.vertex_color_);
/* TODO: Add UI. */
Member

Is this going to be implemented?

Is this going to be implemented?
Author
Member

Currently hardness has not been implemented for the draw tool and in the legacy version this option lives in the Advanced subpanel. so implementing this would require the subpanel, which would be better as a separate PR.

Currently hardness has not been implemented for the draw tool and in the legacy version this option lives in the `Advanced` subpanel. so implementing this would require the subpanel, which would be better as a separate PR.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +719,4 @@
/* Updates indicator in header. */
grease_pencil_primitive_status_indicators(C, op, ptd);
/* add a modal handler for this operator */
Member

Upper case, missing dot

Upper case, missing dot
casey-bianco-davis marked this conversation as resolved
@ -0,0 +1000,4 @@
const ControlPointType control_point_type = get_control_point_type(ptd, ui_id);
if (control_point_type == ControlPointType::JOIN_POINT) {
Member

Looks like the same thing happens in all cases, no need to check the point type.

Looks like the same thing happens in all cases, no need to check the point type.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +1353,4 @@
/* Identifiers. */
ot->name = "Grease Pencil Box Shape";
ot->idname = "GREASE_PENCIL_OT_primitive_box";
ot->description = "Create predefined grease pencil stroke boxs";
Member

boxes

boxes
casey-bianco-davis marked this conversation as resolved
@ -130,3 +130,3 @@
}
static int grease_pencil_brush_stroke_invoke(bContext *C, wmOperator *op, const wmEvent *event)
int grease_pencil_draw_operator_invoke(bContext *C, wmOperator *op)
Member

Why is this in a header now? Looks like the function only gets called locally and can remain static.

Why is this in a header now? Looks like the function only gets called locally and can remain static.
Author
Member

No it is used in grease_pencil_primitive_invoke line 617:

static int grease_pencil_primitive_invoke(bContext *C, wmOperator *op, const wmEvent *event)
{
  int return_value = ed::sculpt_paint::grease_pencil_draw_operator_invoke(C, op);
  if (return_value != OPERATOR_RUNNING_MODAL) {
    return return_value;
  }
No it is used in `grease_pencil_primitive_invoke` line 617: ``` static int grease_pencil_primitive_invoke(bContext *C, wmOperator *op, const wmEvent *event) { int return_value = ed::sculpt_paint::grease_pencil_draw_operator_invoke(C, op); if (return_value != OPERATOR_RUNNING_MODAL) { return return_value; } ```
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 11 commits 2024-03-04 20:11:21 +01:00
casey-bianco-davis requested review from Lukas Tönne 2024-03-04 20:17:50 +01:00
Falk David requested changes 2024-03-05 15:23:59 +01:00
Dismissed
Falk David left a comment
Member

While testing I noticed the box primitive seems to be broken atm.
image
The control points should be in the top-left and bottom-right corner (and move the appropriate vertex of the rectangle).

I also find the control points for the circle primitive less intuitive. I prefer if those stay the same to how they work in GPv2.

While testing I noticed the box primitive seems to be broken atm. ![image](/attachments/35d0b3ae-9671-462f-ab3d-6dee845d8739) The control points should be in the top-left and bottom-right corner (and move the appropriate vertex of the rectangle). I also find the control points for the circle primitive less intuitive. I prefer if those stay the same to how they work in GPv2.
@ -0,0 +1260,4 @@
const int default_subdiv,
const PrimitiveType default_type)
{
/* Callbacks. */
Member

The callbacks should be left in the operator definition. It's ok to have this duplicated.

The callbacks should be left in the operator definition. It's ok to have this duplicated.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +1308,4 @@
"Whether the interpolation happens in 2D view space or in 3D world space");
}
void GREASE_PENCIL_OT_primitive_line(wmOperatorType *ot)
Member

All these functions should be static.

All these functions should be `static`.
casey-bianco-davis marked this conversation as resolved
Author
Member

I originally tried to have the control points of the box operator be the Top left, Center and Bottom right, but this caused a problem, these three points do not fully determent the all of the other corners in 3D space and in only works in 2D space if the box is aligned with the view, but because of the rotating R that assumption is no longer valid.
The resonate that the top left, center and bottom right does not work is because those three points are collinear.

When the box operator is started (in the EXTRUDING mode) the control point are placed as if they were the top left and bottom right.

I could change the box and circle operation to behave like the legacy operator when Interpolation Mode is set to 2D but this would remove rotating the box (and this would mean that the control points would change between 2D and 3D Mode).

I originally tried to have the control points of the box operator be the Top left, Center and Bottom right, but this caused a problem, these three points do not fully determent the all of the other corners in 3D space and in only works in 2D space if the box is aligned with the view, but because of the rotating `R` that assumption is no longer valid. The resonate that the top left, center and bottom right does not work is because those three points are collinear. When the box operator is started (in the `EXTRUDING` mode) the control point are placed as if they were the top left and bottom right. I could change the box and circle operation to behave like the legacy operator when `Interpolation Mode` is set to 2D but this would remove rotating the box (and this would mean that the control points would change between 2D and 3D Mode).
casey-bianco-davis added 3 commits 2024-03-05 21:29:50 +01:00
casey-bianco-davis requested review from Falk David 2024-03-05 21:31:21 +01:00
Member

@casey-bianco-davis I didn't realize the mode changed the behavior this much.
These tools are primarily used for drawing shapes on the drawing plane. I think adding a mode that tries to make them work in 3D would need an approved design first.

@casey-bianco-davis I didn't realize the mode changed the behavior this much. These tools are primarily used for drawing shapes on the drawing plane. I think adding a mode that tries to make them work in 3D would need an approved design first.
Author
Member

I think adding a mode that tries to make them work in 3D would need an approved design first.

@filedescriptor Should I remove the Interpolation mode from this PR (and use the legacy points) and create a design task to discus 3D?

> I think adding a mode that tries to make them work in 3D would need an approved design first. @filedescriptor Should I remove the `Interpolation mode` from this PR (and use the legacy points) and create a design task to discus 3D?
casey-bianco-davis added 1 commit 2024-03-11 02:46:51 +01:00
Member

@casey-bianco-davis Yes, that would be great, thanks.

@casey-bianco-davis Yes, that would be great, thanks.
casey-bianco-davis added 3 commits 2024-03-12 03:40:52 +01:00
casey-bianco-davis changed title from GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool. to WIP: GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool. 2024-03-12 06:44:03 +01:00
casey-bianco-davis changed title from WIP: GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool. to GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool. 2024-03-13 03:31:14 +01:00
casey-bianco-davis added 3 commits 2024-03-13 03:31:21 +01:00
Member

Tested this again and have some feedback:

  • Right-click to cancel doesn't seem to work for most actions like Grab, Rotate, Scale, Extrude
  • I'd expect the polyline tool to add a dot when left clicking at the start. Atm, to start adding points you need to drag first and then clicking works.
  • When panning, the currently drawn shape will seemingly try to adapt to the new viewing angle. It would be best if the drawn shapes stick to the plane that they were first drawn on. Also zooming in and out will change the radii.
  • I'm getting some unexpected prints in the console like
rna_uiItemR: property not found: GREASE_PENCIL_OT_primitive_line.interpolate_mode
/.../blender-git/build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/properties_paint_common.py:1456
Unsupported mode: PAINT_GREASE_PENCIL
Tested this again and have some feedback: - Right-click to cancel doesn't seem to work for most actions like Grab, Rotate, Scale, Extrude - I'd expect the polyline tool to add a dot when left clicking at the start. Atm, to start adding points you need to drag first and then clicking works. - When panning, the currently drawn shape will seemingly try to adapt to the new viewing angle. It would be best if the drawn shapes stick to the plane that they were first drawn on. Also zooming in and out will change the radii. - I'm getting some unexpected prints in the console like ``` rna_uiItemR: property not found: GREASE_PENCIL_OT_primitive_line.interpolate_mode /.../blender-git/build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/properties_paint_common.py:1456 Unsupported mode: PAINT_GREASE_PENCIL ```
casey-bianco-davis added 8 commits 2024-03-16 05:53:25 +01:00
casey-bianco-davis added 1 commit 2024-03-18 04:45:56 +01:00
Author
Member
  • Fixed Right click cancel.

  • Fixed single click for polyline start.

  • Fixed rna_uiItemR: property not found console error (note: Unsupported mode: PAINT_GREASE_PENCIL error is not caused by this PR)

  • Currently the code uses ed::greasepencil::DrawingPlacement for placement which means the curve is calculated from the viewport's perspective, this could be changed to save the starting plane and then projecting on to it, but this would remove the ability to use stroke placement, I am not sure which would be better.

  • The radius changing with the viewport is because the code is shared between the draw tool and the primitive tool, and the draw tool currently uses viewport size (I believe @filedescriptor was working on a PR)

- Fixed Right click cancel. - Fixed single click for polyline start. - Fixed `rna_uiItemR: property not found` console error (note: `Unsupported mode: PAINT_GREASE_PENCIL` error is not caused by this PR) - Currently the code uses `ed::greasepencil::DrawingPlacement` for placement which means the curve is calculated from the viewport's perspective, this could be changed to save the starting plane and then projecting on to it, but this would remove the ability to use `stroke placement`, I am not sure which would be better. - The radius changing with the viewport is because the code is shared between the draw tool and the primitive tool, and the draw tool currently uses viewport size (I believe @filedescriptor was working on a PR)
Member

@casey-bianco-davis Ok, I would suggest to remove the ability to navigate the viewport while the tool is active then. I know that this is a nice feature (and we can make this work in future releases), but we should keep things simple for now and avoid too many design questions.
And yes, the radius is something I'm working on, so good to know that this will be fix then.

@casey-bianco-davis Ok, I would suggest to remove the ability to navigate the viewport while the tool is active then. I know that this is a nice feature (and we can make this work in future releases), but we should keep things simple for now and avoid too many design questions. And yes, the radius is something I'm working on, so good to know that this will be fix then.
Falk David requested changes 2024-03-19 12:40:38 +01:00
Dismissed
Falk David left a comment
Member

Added some cleanup comments.

Added some cleanup comments.
@ -0,0 +101,4 @@
static constexpr float UI_POINT_MAX_HIT_SIZE_PX = 600.0f;
/* The center point for `Box` and `Circle` type. */
static constexpr int CONTROL_POINT_CENTER = 1;
Member

What does the value 1 mean here?

What does the value `1` mean here?
Member

Is it some value for an enum? I can't really tell

Is it some value for an enum? I can't really tell

No reason to use upper register for constexpr variable names.

No reason to use upper register for constexpr variable names.
Author
Member

The number 1 is just because it is the second point (starting with zero)

First = 0
Center = 1
Last = 2

The number 1 is just because it is the second point (starting with zero) First = 0 Center = 1 Last = 2
casey-bianco-davis marked this conversation as resolved
@ -0,0 +103,4 @@
/* The center point for `Box` and `Circle` type. */
static constexpr int CONTROL_POINT_CENTER = 1;
struct PrimitiveTool_OpData {
Member

PrimitiveToolOperation would be a bit more consistent with how we're naming things. E.g. StrokeOperation.

`PrimitiveToolOperation` would be a bit more consistent with how we're naming things. E.g. `StrokeOperation`.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +593,4 @@
/* Invoke handler: Initialize the operator. */
static int grease_pencil_primitive_invoke(bContext *C, wmOperator *op, const wmEvent *event)
{
int return_value = ed::sculpt_paint::grease_pencil_draw_operator_invoke(C, op);
Member

Can you explain why this is called here? I'm not sure it's worth it to add that dependency on the painting code.

Can you explain why this is called here? I'm not sure it's worth it to add that dependency on the painting code.
Author
Member

This call is to ensure that the primitive tool will throw the same errors to the user as the draw tool.
(I did this before ensure_active_keyframe was made and duplicating the code would have been worse)
If you wanted the code can just be copied.

This call is to ensure that the primitive tool will throw the same errors to the user as the draw tool. (I did this before `ensure_active_keyframe` was made and duplicating the code would have been worse) If you wanted the code can just be copied.
Member

I see. I think that grease_pencil_draw_operator_invoke function can go into grease_pencil_utils.cc and in ED_grease_pencil.hh instead. So that we don't have to link the whole paint module against the editors code (change in source/blender/editors/grease_pencil/CMakeLists.txt can be removed).

I see. I think that `grease_pencil_draw_operator_invoke` function can go into `grease_pencil_utils.cc` and in `ED_grease_pencil.hh` instead. So that we don't have to link the whole paint module against the editors code (change in `source/blender/editors/grease_pencil/CMakeLists.txt` can be removed).
Author
Member

The paint module is also being used for radius_from_input_sample and opacity_from_input_sample functions should they be move as well?
and if so should paint_calc_object_space_radius be moved or linked?

The paint module is also being used for `radius_from_input_sample` and `opacity_from_input_sample` functions should they be move as well? and if so should `paint_calc_object_space_radius` be moved or linked?
casey-bianco-davis marked this conversation as resolved
@ -0,0 +1436,4 @@
WM_operatortype_append(GREASE_PENCIL_OT_primitive_circle);
}
wmKeyMap *ED_primitivetool_modal_keymap(wmKeyConfig *keyconf)
Member

Why does this need to return the keymap?

Why does this need to return the keymap?
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 4 commits 2024-03-19 23:32:19 +01:00
casey-bianco-davis requested review from Falk David 2024-03-19 23:39:57 +01:00
Falk David requested changes 2024-03-22 13:02:01 +01:00
Dismissed
Falk David left a comment
Member

Replied to previous comments

Replied to previous comments
casey-bianco-davis requested review from Falk David 2024-03-22 22:10:12 +01:00
casey-bianco-davis added 2 commits 2024-03-22 22:49:13 +01:00
casey-bianco-davis added 1 commit 2024-03-22 23:05:06 +01:00
Falk David requested changes 2024-04-02 14:21:37 +02:00
Dismissed
Falk David left a comment
Member

Did a full pass on this. Have a couple of cleanup comments. Code works now as I expect it to though 👍

Did a full pass on this. Have a couple of cleanup comments. Code works now as I expect it to though 👍
@ -0,0 +1,1471 @@
/* SPDX-FileCopyrightText: 2023 Blender Authors
Member

Copyright data should be 2024

Copyright data should be `2024`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +120,4 @@
int subdivision;
float4x4 projection;
/* Helper class to project screen space coordinates to 3D. */
ed::greasepencil::DrawingPlacement placement;
Member

ed::greasepencil:: shouldn't be needed.

`ed::greasepencil::` shouldn't be needed.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +264,4 @@
static void grease_pencil_primitive_draw(const bContext * /*C*/, ARegion * /*region*/, void *arg)
{
PrimitiveToolOperation &ptd = *((PrimitiveToolOperation *)arg);
Member

PrimitiveToolOperation &ptd = *reinterpret_cast<PrimitiveToolOperation *>(arg);

`PrimitiveToolOperation &ptd = *reinterpret_cast<PrimitiveToolOperation *>(arg);`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +428,4 @@
static void grease_pencil_primitive_update_curves(PrimitiveToolOperation &ptd)
{
bke::CurvesGeometry &curves = ptd.drawing->strokes_for_write();
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();
Member

This is unused.

This is unused.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +430,4 @@
bke::CurvesGeometry &curves = ptd.drawing->strokes_for_write();
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();
const int last_points = curves.points_by_curve()[curves.curves_range().last()].size();
Member

last_points_num maybe? Since it's a size

`last_points_num` maybe? Since it's a size
casey-bianco-davis marked this conversation as resolved
@ -0,0 +432,4 @@
const int last_points = curves.points_by_curve()[curves.curves_range().last()].size();
int new_points_num = grease_pencil_primitive_curve_points_number(ptd);
Member

const

`const`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +451,4 @@
new_vertex_colors.fill(ColorGeometry4f(ptd.vertex_color));
ToolSettings *ts = ptd.vc.scene->toolsettings;
Member

const

`const`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +452,4 @@
new_vertex_colors.fill(ColorGeometry4f(ptd.vertex_color));
ToolSettings *ts = ptd.vc.scene->toolsettings;
GP_Sculpt_Settings *gset = &ts->gp_sculpt;
Member

const

`const`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +507,4 @@
end_caps.finish();
}
cyclic.span.last() = ELEM(ptd.type, PrimitiveType::BOX, PrimitiveType::CIRCLE);
Member

To make this a bit clearer, I would suggest to split this into a new line:

const bool is_cyclic = ELEM(ptd.type, PrimitiveType::BOX, PrimitiveType::CIRCLE);
cyclic.span.last() = is_cyclic;

The compiler will optimize it out anyway.

To make this a bit clearer, I would suggest to split this into a new line: ```cpp const bool is_cyclic = ELEM(ptd.type, PrimitiveType::BOX, PrimitiveType::CIRCLE); cyclic.span.last() = is_cyclic; ``` The compiler will optimize it out anyway.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +650,4 @@
GreasePencil *grease_pencil = static_cast<GreasePencil *>(vc.obact->data);
/* Initialize helper class for projecting screen space coordinates. */
ed::greasepencil::DrawingPlacement placement = ed::greasepencil::DrawingPlacement(
Member

ed::greasepencil:: shouldn't be needed.

`ed::greasepencil::` shouldn't be needed.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +716,4 @@
float4(0.0f);
srgb_to_linearrgb_v4(ptd.vertex_color, ptd.vertex_color);
/* TODO: Add UI. */
Member

Is this still a TODO ? What's missing?

Is this still a `TODO` ? What's missing?
Author
Member

Because the draw tool does not have UI for hardness so does these tools.

Because the draw tool does not have UI for hardness so does these tools.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +841,4 @@
static void grease_pencil_primitive_grab_update(PrimitiveToolOperation &ptd, const wmEvent *event)
{
BLI_assert(ptd.active_control_point_index != -1);
float3 pos = ptd.placement.project(float2(event->mval));
Member

const

`const`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +877,4 @@
const float2 start_pos2 = ED_view3d_project_float_v2_m4(
ptd.vc.region, ptd.temp_control_points[ptd.active_control_point_index], ptd.projection);
float3 pos = ptd.placement.project(start_pos2 + dif);
Member

const

`const`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +918,4 @@
const float s = math::sin(rotation);
const float2x2 rot = float2x2(float2(c, s), float2(-s, c));
const float2 pos2 = rot * dif + center_of_mass;
float3 pos = ptd.placement.project(pos2);
Member

const

`const`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +938,4 @@
ptd.vc.region, ptd.temp_control_points[point_index], ptd.projection);
const float2 pos2 = (start_pos2 - center_of_mass) * scale + center_of_mass;
float3 pos = ptd.placement.project(pos2);
Member

const

`const`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +1216,4 @@
grease_pencil_primitive_rotate_all_update(ptd, event);
break;
}
case OperatorMode::IDLE: {
Member

Just to make it clear, I'd add:

/* Do nothing. */
break;
Just to make it clear, I'd add: ```cpp /* Do nothing. */ break; ```
casey-bianco-davis marked this conversation as resolved
@ -0,0 +1224,4 @@
/* Modal handler: Events handling during interactive part. */
static int grease_pencil_primitive_modal(bContext *C, wmOperator *op, const wmEvent *event)
{
PrimitiveToolOperation &ptd = *((PrimitiveToolOperation *)op->customdata);
Member

PrimitiveToolOperation &ptd = *reinterpret_cast<PrimitiveToolOperation *>(op->customdata);

`PrimitiveToolOperation &ptd = *reinterpret_cast<PrimitiveToolOperation *>(op->customdata);`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +1289,4 @@
const PrimitiveType default_type)
{
/* Flags. */
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO | OPTYPE_BLOCKING;
Member

Better to only add the properties in this function. Even if setting the flag is repeated across all operators. That's fine.

Better to only add the properties in this function. Even if setting the flag is repeated across all operators. That's fine.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +1467,4 @@
WM_modalkeymap_assign(keymap, "GREASE_PENCIL_OT_primitive_box");
WM_modalkeymap_assign(keymap, "GREASE_PENCIL_OT_primitive_circle");
return;
Member

Not needed.

Not needed.
casey-bianco-davis marked this conversation as resolved
@ -160,3 +133,1 @@
if (!ed::greasepencil::ensure_active_keyframe(*scene, grease_pencil)) {
BKE_report(op->reports, RPT_ERROR, "No Grease Pencil frame to draw on");
return OPERATOR_CANCELLED;
int return_value = ed::sculpt_paint::greasepencil::grease_pencil_draw_operator_invoke(C, op);
Member

Move this to grease_pencil_utils.cc and declare in ED_grease_pencil.hh.

Move this to `grease_pencil_utils.cc` and declare in `ED_grease_pencil.hh`.
casey-bianco-davis marked this conversation as resolved
@ -25,9 +25,22 @@ class GreasePencilStrokeOperation {
namespace greasepencil {
float opacity_from_input_sample(const float pressure,
Member

Move these three functions to grease_pencil_utils.cc and declare in ED_grease_pencil.hh.

Move these three functions to `grease_pencil_utils.cc` and declare in `ED_grease_pencil.hh`.
casey-bianco-davis marked this conversation as resolved
Hans Goudey reviewed 2024-04-03 03:28:03 +02:00
@ -0,0 +51,4 @@
namespace blender::ed::greasepencil {
enum class PrimitiveType : int8_t {
LINE = 0,
Member

Small thing, but typical naming for enum class uses CamelCase for each value, like PrimitiveType::Line. That's a fair amount easier to read IMO. The all-capital is left over from when we didn't write the enum prefix I think.

Small thing, but typical naming for `enum class` uses CamelCase for each value, like `PrimitiveType::Line`. That's a fair amount easier to read IMO. The all-capital is left over from when we didn't write the enum prefix I think.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +265,4 @@
static void grease_pencil_primitive_draw(const bContext * /*C*/, ARegion * /*region*/, void *arg)
{
PrimitiveToolOperation &ptd = *((PrimitiveToolOperation *)arg);
draw_control_points(ptd);
Member

Use C++ casts here

Use C++ casts here
casey-bianco-davis marked this conversation as resolved
@ -0,0 +282,4 @@
array_utils::copy(ptd.temp_control_points.as_span(), ptd.control_points.as_mutable_span());
}
template<typename T>
Member

Does this need to be a template?

Does this need to be a template?
Author
Member

This was done because originally this PR did both 2D and 3D interpolations. But that going to be in a separate PR.

This was done because originally this PR did both 2D and 3D interpolations. But that going to be in a separate PR.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +283,4 @@
}
template<typename T>
static void primitive_calulate_curve_positions_exec(PrimitiveToolOperation &ptd,
Member

Usually "exec" refers to the operator callback. Might be clearer without it here

Usually "exec" refers to the operator callback. Might be clearer without it here
casey-bianco-davis marked this conversation as resolved
@ -0,0 +454,4 @@
ToolSettings *ts = ptd.vc.scene->toolsettings;
GP_Sculpt_Settings *gset = &ts->gp_sculpt;
for (const int point_id : curve_points.index_range()) {
Member

point_id -> point That's the typical naming here. All the "point" is is an index into the arrays anyway.

`point_id` -> `point` That's the typical naming here. All the "point" is is an index into the arrays anyway.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +652,4 @@
/* Initialize helper class for projecting screen space coordinates. */
ed::greasepencil::DrawingPlacement placement = ed::greasepencil::DrawingPlacement(
*vc.scene, *vc.region, *view3d, *vc.obact, *grease_pencil->get_active_layer());
if (placement.use_project_to_surface()) {
Member

Looks like Falk's comment isn't resolved, ed::greasepencil:: is still used here

Looks like Falk's comment isn't resolved, `ed::greasepencil::` is still used here
Author
Member

It looks like you reviewed while I was working on Falk's comments, I marked as resolve because it's fix on my computer and I will push after I fix everything.

It looks like you reviewed while I was working on Falk's comments, I marked as resolve because it's fix on my computer and I will push after I fix everything.
Member

Oh, sorry about that! That's how I do it too, I didn't realize Falk's review was so recent.

Oh, sorry about that! That's how I do it too, I didn't realize Falk's review was so recent.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 18 commits 2024-04-03 04:54:16 +02:00
casey-bianco-davis requested review from Falk David 2024-04-03 04:55:28 +02:00
Falk David approved these changes 2024-04-03 15:33:11 +02:00
Dismissed
Falk David left a comment
Member

Code looks good to me now.

Code looks good to me now.
Member

Will wait for @LukasTonne to accept.

Will wait for @LukasTonne to accept.
Lukas Tönne approved these changes 2024-04-04 15:34:44 +02:00
@ -1392,3 +1392,3 @@
def brush_basic_grease_pencil_paint_settings(layout, context, brush, *, compact=False):
def brush_basic_grease_pencil_paint_settings(layout, context, brush, *, compact=False, props=None):
Member

props seems to be unused

`props` seems to be unused
casey-bianco-davis marked this conversation as resolved
Member

@blender-bot build

@blender-bot build
Member

Hm looks like this breaks the script_validate_keymap test.

Hm looks like this breaks the `script_validate_keymap` test.
casey-bianco-davis added 1 commit 2024-04-04 19:51:52 +02:00
Author
Member

Hm looks like this breaks the script_validate_keymap test.

Can confirm, I will look in to it.

> Hm looks like this breaks the `script_validate_keymap` test. Can confirm, I will look in to it.
casey-bianco-davis added 3 commits 2024-04-09 00:02:38 +02:00
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
2fd15ae8a1
Cleanup: Fix build test failure.
Author
Member

@filedescriptor I think the tests are fix now.

@filedescriptor I think the tests are fix now.
Member

@blender-bot build

@blender-bot build
Falk David requested changes 2024-04-09 12:06:04 +02:00
Dismissed
Falk David left a comment
Member

@casey-bianco-davis Tested this locally again and I'm getting some errors (in the UI code):

TypeError: brush_basic_grease_pencil_paint_settings() got an unexpected keyword argument 'props'
Traceback (most recent call last):
  File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_view3d.py", line 39, in draw
    self.draw_tool_settings(context)
  File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_view3d.py", line 52, in draw_tool_settings
    tool = ToolSelectPanelHelper.draw_active_tool_header(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_toolsystem_common.py", line 821, in draw_active_tool_header
    draw_settings(context, layout, tool)
  File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_toolsystem_toolbar.py", line 1935, in draw_settings
    _defs_paint_grease_pencil.grease_pencil_primitive_toolbar(context, layout, tool, props)
  File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_toolsystem_toolbar.py", line 1864, in grease_pencil_primitive_toolbar
    brush_basic_grease_pencil_paint_settings(layout, context, brush, compact=True, props=props)

@casey-bianco-davis Tested this locally again and I'm getting some errors (in the UI code): ```bash TypeError: brush_basic_grease_pencil_paint_settings() got an unexpected keyword argument 'props' Traceback (most recent call last): File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_view3d.py", line 39, in draw self.draw_tool_settings(context) File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_view3d.py", line 52, in draw_tool_settings tool = ToolSelectPanelHelper.draw_active_tool_header( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_toolsystem_common.py", line 821, in draw_active_tool_header draw_settings(context, layout, tool) File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_toolsystem_toolbar.py", line 1935, in draw_settings _defs_paint_grease_pencil.grease_pencil_primitive_toolbar(context, layout, tool, props) File "/.../build_linux_debug_lite/bin/4.2/scripts/startup/bl_ui/space_toolsystem_toolbar.py", line 1864, in grease_pencil_primitive_toolbar brush_basic_grease_pencil_paint_settings(layout, context, brush, compact=True, props=props) ```
casey-bianco-davis requested review from Falk David 2024-04-10 02:49:25 +02:00
casey-bianco-davis added 1 commit 2024-04-10 02:49:31 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
dbe3113063
Cleanup: Remove outdated code that caused errors.
Member

@blender-bot build

@blender-bot build
casey-bianco-davis added 2 commits 2024-04-14 01:44:05 +02:00
Falk David approved these changes 2024-04-22 10:47:03 +02:00
Falk David left a comment
Member

Thanks! I think this is ready to be merged now.
One thing that I found is that rotating the box primitive does not rotate the box (because of how it is generated by the two corners I think). But this can be looked at in a subsequent PR.

Thanks! I think this is ready to be merged now. One thing that I found is that rotating the box primitive does not rotate the box (because of how it is generated by the two corners I think). But this can be looked at in a subsequent PR.
Falk David merged commit 2df13ce4a3 into main 2024-04-22 10:48:16 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#119039
No description provided.