GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool. #119039
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#119039
Loading…
Reference in New Issue
No description provided.
Delete Branch "casey-bianco-davis/blender:GPv3-Primitive-Tools"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
ands
keybinds.Also all control points are editable after extruding.
@ -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)]}),
The
wait_for_input
property is not defined for any of the new operators, i get a bunch of warnings:@ -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)
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.@ -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,
It's not clear to me what "extrinsic" means (not a native speaker), any more intuitive names we might use?
@ -0,0 +122,4 @@
InterpolationMode interpolate_mode;
float4x4 projection;
/* Helper class to project screen space coordinates to 3D. */
ed::greasepencil::DrawingPlacement placement_;
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
@ -0,0 +181,4 @@
ColorGeometry4f color_gizmo_primary;
ColorGeometry4f color_gizmo_secondary;
ColorGeometry4f color_gizmo_a;
UI_GetThemeColor4fv(TH_REDALERT, color_redalert);
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.@ -0,0 +703,4 @@
float4(0.0f);
srgb_to_linearrgb_v4(ptd.vertex_color_, ptd.vertex_color_);
/* TODO: Add UI. */
Is this going to be implemented?
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.@ -0,0 +719,4 @@
/* Updates indicator in header. */
grease_pencil_primitive_status_indicators(C, op, ptd);
/* add a modal handler for this operator */
Upper case, missing dot
@ -0,0 +1000,4 @@
const ControlPointType control_point_type = get_control_point_type(ptd, ui_id);
if (control_point_type == ControlPointType::JOIN_POINT) {
Looks like the same thing happens in all cases, no need to check the point type.
@ -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";
boxes
@ -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)
Why is this in a header now? Looks like the function only gets called locally and can remain static.
No it is used in
grease_pencil_primitive_invoke
line 617:While testing I noticed the box primitive seems to be broken atm.
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. */
The callbacks should be left in the operator definition. It's ok to have this duplicated.
@ -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)
All these functions should be
static
.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 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.
@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 Yes, that would be great, thanks.
GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool.to WIP: GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool.WIP: GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool.to GPv3: Primitive Tools: Line, Polyline, Arc, Curve, Box and Circle Tool.Tested this again and have some feedback:
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 usestroke 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)
@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.
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;
What does the value
1
mean here?Is it some value for an enum? I can't really tell
No reason to use upper register for constexpr variable names.
The number 1 is just because it is the second point (starting with zero)
First = 0
Center = 1
Last = 2
@ -0,0 +103,4 @@
/* The center point for `Box` and `Circle` type. */
static constexpr int CONTROL_POINT_CENTER = 1;
struct PrimitiveTool_OpData {
PrimitiveToolOperation
would be a bit more consistent with how we're naming things. E.g.StrokeOperation
.@ -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);
Can you explain why this is called here? I'm not sure it's worth it to add that dependency on the painting code.
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.
I see. I think that
grease_pencil_draw_operator_invoke
function can go intogrease_pencil_utils.cc
and inED_grease_pencil.hh
instead. So that we don't have to link the whole paint module against the editors code (change insource/blender/editors/grease_pencil/CMakeLists.txt
can be removed).The paint module is also being used for
radius_from_input_sample
andopacity_from_input_sample
functions should they be move as well?and if so should
paint_calc_object_space_radius
be moved or linked?@ -0,0 +1436,4 @@
WM_operatortype_append(GREASE_PENCIL_OT_primitive_circle);
}
wmKeyMap *ED_primitivetool_modal_keymap(wmKeyConfig *keyconf)
Why does this need to return the keymap?
Replied to previous comments
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
Copyright data should be
2024
@ -0,0 +120,4 @@
int subdivision;
float4x4 projection;
/* Helper class to project screen space coordinates to 3D. */
ed::greasepencil::DrawingPlacement placement;
ed::greasepencil::
shouldn't be needed.@ -0,0 +264,4 @@
static void grease_pencil_primitive_draw(const bContext * /*C*/, ARegion * /*region*/, void *arg)
{
PrimitiveToolOperation &ptd = *((PrimitiveToolOperation *)arg);
PrimitiveToolOperation &ptd = *reinterpret_cast<PrimitiveToolOperation *>(arg);
@ -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();
This is unused.
@ -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();
last_points_num
maybe? Since it's a size@ -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);
const
@ -0,0 +451,4 @@
new_vertex_colors.fill(ColorGeometry4f(ptd.vertex_color));
ToolSettings *ts = ptd.vc.scene->toolsettings;
const
@ -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;
const
@ -0,0 +507,4 @@
end_caps.finish();
}
cyclic.span.last() = ELEM(ptd.type, PrimitiveType::BOX, PrimitiveType::CIRCLE);
To make this a bit clearer, I would suggest to split this into a new line:
The compiler will optimize it out anyway.
@ -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(
ed::greasepencil::
shouldn't be needed.@ -0,0 +716,4 @@
float4(0.0f);
srgb_to_linearrgb_v4(ptd.vertex_color, ptd.vertex_color);
/* TODO: Add UI. */
Is this still a
TODO
? What's missing?Because the draw tool does not have UI for hardness so does these tools.
@ -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));
const
@ -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);
const
@ -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);
const
@ -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);
const
@ -0,0 +1216,4 @@
grease_pencil_primitive_rotate_all_update(ptd, event);
break;
}
case OperatorMode::IDLE: {
Just to make it clear, I'd add:
@ -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);
PrimitiveToolOperation &ptd = *reinterpret_cast<PrimitiveToolOperation *>(op->customdata);
@ -0,0 +1289,4 @@
const PrimitiveType default_type)
{
/* Flags. */
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO | OPTYPE_BLOCKING;
Better to only add the properties in this function. Even if setting the flag is repeated across all operators. That's fine.
@ -0,0 +1467,4 @@
WM_modalkeymap_assign(keymap, "GREASE_PENCIL_OT_primitive_box");
WM_modalkeymap_assign(keymap, "GREASE_PENCIL_OT_primitive_circle");
return;
Not needed.
@ -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);
Move this to
grease_pencil_utils.cc
and declare inED_grease_pencil.hh
.@ -25,9 +25,22 @@ class GreasePencilStrokeOperation {
namespace greasepencil {
float opacity_from_input_sample(const float pressure,
Move these three functions to
grease_pencil_utils.cc
and declare inED_grease_pencil.hh
.@ -0,0 +51,4 @@
namespace blender::ed::greasepencil {
enum class PrimitiveType : int8_t {
LINE = 0,
Small thing, but typical naming for
enum class
uses CamelCase for each value, likePrimitiveType::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.@ -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);
Use C++ casts here
@ -0,0 +282,4 @@
array_utils::copy(ptd.temp_control_points.as_span(), ptd.control_points.as_mutable_span());
}
template<typename T>
Does this need to be a template?
This was done because originally this PR did both 2D and 3D interpolations. But that going to be in a separate PR.
@ -0,0 +283,4 @@
}
template<typename T>
static void primitive_calulate_curve_positions_exec(PrimitiveToolOperation &ptd,
Usually "exec" refers to the operator callback. Might be clearer without it here
@ -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()) {
point_id
->point
That's the typical naming here. All the "point" is is an index into the arrays anyway.@ -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()) {
Looks like Falk's comment isn't resolved,
ed::greasepencil::
is still used hereIt 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.
Oh, sorry about that! That's how I do it too, I didn't realize Falk's review was so recent.
Code looks good to me now.
Will wait for @LukasTonne to accept.
@ -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):
props
seems to be unused@blender-bot build
Hm looks like this breaks the
script_validate_keymap
test.Can confirm, I will look in to it.
@filedescriptor I think the tests are fix now.
@blender-bot build
@casey-bianco-davis Tested this locally again and I'm getting some errors (in the UI code):
@blender-bot build
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.