GPv3: Transform operators (translate, rotate, scale, opacity, radius) #111836

Merged
Falk David merged 23 commits from casey-bianco-davis/blender:GPv3-transform into main 2023-10-21 15:12:57 +02:00

This adds the keybindings, menu, and functionally for the edit mode transform operators to grease pencil v3

This adds the keybindings, menu, and functionally for the edit mode transform operators to grease pencil v3
casey-bianco-davis added 1 commit 2023-09-02 22:30:25 +02:00

Have you sure it's already not a wip?

Have you sure it's already not a wip?
Iliya Katushenock added this to the Grease Pencil project 2023-09-02 22:33:14 +02:00
Iliya Katushenock added the
Module
Modeling
label 2023-09-02 22:33:22 +02:00
casey-bianco-davis changed title from GPv3: Add edit mode transform operator (i.e translate, rotation, scale, opacity, radius) to WIP: GPv3: Add edit mode transform operator (i.e translate, rotation, scale, opacity, radius) 2023-09-02 22:50:21 +02:00
casey-bianco-davis added 1 commit 2023-09-02 23:57:41 +02:00
casey-bianco-davis changed title from WIP: GPv3: Add edit mode transform operator (i.e translate, rotation, scale, opacity, radius) to GPv3: Add edit mode transform operator (i.e translate, rotation, scale, opacity, radius) 2023-09-03 00:12:13 +02:00
casey-bianco-davis added 1 commit 2023-09-03 01:25:25 +02:00
Falk David requested review from Falk David 2023-09-11 11:51:55 +02:00
casey-bianco-davis added 1 commit 2023-09-13 02:53:23 +02:00
Falk David requested changes 2023-09-27 10:35:56 +02:00
Falk David left a comment
Member

Thank you for working on this! Very much appreciated.
Overall the code looks very good already. I'd just like to see some more shared code between curves and grease pencil drawings.

Thank you for working on this! Very much appreciated. Overall the code looks very good already. I'd just like to see some more shared code between curves and grease pencil drawings.
@ -32,6 +32,7 @@ set(SRC
transform_convert_curves.cc
transform_convert_gpencil_legacy.cc
transform_convert_graph.cc
transform_convert_greasepencil.cc
Member

greasepencil -> grease_pencil

`greasepencil` -> `grease_pencil`
casey-bianco-davis marked this conversation as resolved
@ -0,0 +120,4 @@
scene->r.cfra, [&](int /*drawing_index*/, blender::bke::greasepencil::Drawing &drawing) {
bke::CurvesGeometry &curves = drawing.strokes_for_write();
MutableSpan<float3> positions = curves.positions_for_write();
Member

This code shouldn't be duplicated for Curves and GreasePencil. The implementation could be in transform_convert_curves.cc but it could be declared in transform_convert.hh so that we can just call it here.

This code shouldn't be duplicated for `Curves` and `GreasePencil`. The implementation could be in `transform_convert_curves.cc` but it could be declared in `transform_convert.hh` so that we can just call it here.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 1 commit 2023-09-28 06:47:38 +02:00
casey-bianco-davis added 1 commit 2023-09-30 23:28:02 +02:00
casey-bianco-davis requested review from Falk David 2023-09-30 23:28:39 +02:00
casey-bianco-davis added 1 commit 2023-10-01 00:32:01 +02:00
Falk David requested changes 2023-10-13 11:29:57 +02:00
Falk David left a comment
Member

Generally, this looks very close already. There are some changes in main (like the change to foreach_editable_drawing) that will have to be merged. Just some minor comments.

Generally, this looks very close already. There are some changes in `main` (like the change to `foreach_editable_drawing`) that will have to be merged. Just some minor comments.
@ -109,0 +115,4 @@
/**
* Used for both curves and grease pencil objects.
*/
void curvePopulateTransDataStructs(TransDataContainer &tc,
Member

curves_populate_trans_data_structs

`curves_populate_trans_data_structs`
casey-bianco-davis marked this conversation as resolved
@ -109,0 +119,4 @@
blender::bke::CurvesGeometry &curves,
blender::MutableSpan<float> *value_attribute,
const blender::IndexMask &selected_indices,
const bool use_proportional_edit,
Member

Remove const in declaration.

Remove `const` in declaration.
casey-bianco-davis marked this conversation as resolved
@ -109,0 +121,4 @@
const blender::IndexMask &selected_indices,
const bool use_proportional_edit,
const bool use_connected_only,
const int data_offset);
Member

data_offset -> trans_data_offset

`data_offset` -> `trans_data_offset`
casey-bianco-davis marked this conversation as resolved
@ -100,1 +98,3 @@
pseudoinverse_m3_m3(smtx, mtx, PSEUDOINVERSE_EPSILON);
curvePopulateTransDataStructs(tc,
curves,
nullptr /* Currently no attributes. */,
Member

/* Currently no transform for attributes other than position. */

`/* Currently no transform for attributes other than position. */`
casey-bianco-davis marked this conversation as resolved
@ -810,0 +819,4 @@
}
grease_pencil.foreach_editable_drawing(
scene->r.cfra, [&](int drawing_index, blender::bke::greasepencil::Drawing &drawing) {
Member

layer_index (same for other places as well)

`layer_index` (same for other places as well)
casey-bianco-davis marked this conversation as resolved
@ -810,0 +824,4 @@
const bke::crazyspace::GeometryDeformation deformation =
bke::crazyspace::get_evaluated_grease_pencil_drawing_deformation(
*depsgraph, *ob, drawing_index);
Member

layer_index

`layer_index`
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 6 commits 2023-10-14 02:52:12 +02:00
casey-bianco-davis requested review from Falk David 2023-10-14 03:21:55 +02:00
Falk David requested changes 2023-10-17 10:48:51 +02:00
Falk David left a comment
Member

Did a quick test of this code. Apart from the warning I mentioned, works great :)

Did a quick test of this code. Apart from the warning I mentioned, works great :)
@ -369,0 +373,4 @@
label="Bend",
icon="ops.gpencil.edit_bend",
widget=None,
keymap=(),
Member

This triggers a warning

WARN (wm.keymap): source/blender/windowmanager/intern/wm_keymap.cc:485 WM_keymap_poll: empty keymap '3D View Tool: Edit Grease Pencil, Bend'
This triggers a warning ``` WARN (wm.keymap): source/blender/windowmanager/intern/wm_keymap.cc:485 WM_keymap_poll: empty keymap '3D View Tool: Edit Grease Pencil, Bend' ```
casey-bianco-davis marked this conversation as resolved
@ -56,1 +56,4 @@
int drawing_index);
GeometryDeformation get_evaluated_grease_pencil_drawing_deformation(const Depsgraph &depsgraph,
const Object &ob_orig,
int drawing_index);
Member

This changed to the layer_index in main so I guess a merge is needed.

This changed to the `layer_index` in `main` so I guess a merge is needed.
casey-bianco-davis marked this conversation as resolved
Hans Goudey reviewed 2023-10-17 11:01:52 +02:00
Hans Goudey left a comment
Member

Overall this looks reasonable. I left some comments, mostly about style things.

Overall this looks reasonable. I left some comments, mostly about style things.
@ -109,0 +117,4 @@
*/
void curve_populate_trans_data_structs(TransDataContainer &tc,
blender::bke::CurvesGeometry &curves,
blender::MutableSpan<float> *value_attribute,
Member

Typically it's clearer to use std::optional than a pointer for such optional arguments

Typically it's clearer to use `std::optional` than a pointer for such optional arguments
casey-bianco-davis marked this conversation as resolved
@ -124,3 +124,1 @@
const int point_i = points[i];
TransData &td = tc.data[point_i];
float3 *elem = &positions[point_i];
void curve_populate_trans_data_structs(TransDataContainer &tc,
Member

Could this be in the blender::ed::transform::curves namespace as well?

Could this be in the `blender::ed::transform::curves` namespace as well?
Author
Member

I tried moving the code but kept getting error and I don't know c++ good enough to fix them. :|

I tried moving the code but kept getting error and I don't know c++ good enough to fix them. :|
Member

Okay, that's fine! It can be done later as a more general cleanup

Okay, that's fine! It can be done later as a more general cleanup
@ -148,3 +174,1 @@
TransData &td = tc.data[points[i]];
td.dist = closest_distances[i];
}
if (value_attribute != nullptr) {
Member

I'd have to dig into this code more to see how possible this is, but it would be nice to keep this value_attribute in a separate loop from the location. Even moving it to a separate function would be nice if possible.

For some background, I'm dreaming of a time where we can just process these arrays separately directly, rather than stuffing them in a TransData struct.

I'd have to dig into this code more to see how possible this is, but it would be nice to keep this `value_attribute` in a separate loop from the location. Even moving it to a separate function would be nice if possible. For some background, I'm dreaming of a time where we can just process these arrays separately directly, rather than stuffing them in a `TransData` struct.
@ -0,0 +110,4 @@
layer_points_offset);
if (use_proportional_edit) {
layer_points_offset += curves.point_num;
Member

curves.points_num(). Just nicer to user the accessor consistently.

`curves.points_num()`. Just nicer to user the accessor consistently.
casey-bianco-davis marked this conversation as resolved
@ -0,0 +134,4 @@
bke::CurvesGeometry &curves = drawing.strokes_for_write();
curves.calculate_bezier_auto_handles();
curves.tag_positions_changed();
Member

I think the drawing needs the "positions changed" tag too

I think the drawing needs the "positions changed" tag too
Member

Yep, otherwise the triangle caches are not tagged as dirty.

Yep, otherwise the triangle caches are not tagged as dirty.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 6 commits 2023-10-18 03:09:57 +02:00
Falk David approved these changes 2023-10-18 10:18:18 +02:00
Falk David left a comment
Member

This is looking good to me. More cleanup could be done afterwards in main

This is looking good to me. More cleanup could be done afterwards in main
Hans Goudey reviewed 2023-10-18 10:31:16 +02:00
@ -0,0 +7,4 @@
*/
#include "BLI_array.hh"
#include "BLI_inplace_priority_queue.hh"
Member

Unused include, maybe?

Unused include, maybe?
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 1 commit 2023-10-19 01:18:11 +02:00
Falk David requested review from Hans Goudey 2023-10-19 11:37:57 +02:00
Hans Goudey reviewed 2023-10-19 12:13:18 +02:00
Hans Goudey left a comment
Member

Would you be interested in adding the SHRINKFATTEN mode to curves edit mode too (the new "hair" curves type). It would be really nice to be able to take advantage of the shared code and also be able to adjust the radius there too. That change could be a separate PR if you preferred too.

Would you be interested in adding the `SHRINKFATTEN` mode to curves edit mode too (the new "hair" curves type). It would be really nice to be able to take advantage of the shared code and also be able to adjust the radius there too. That change could be a separate PR if you preferred too.
@ -0,0 +81,4 @@
const IndexMask selected_indices = selection_per_layer_per_object[i + layer_offset];
std::optional<blender::MutableSpan<float>> value_attribute = {};
Member
  • = {} is unnecessary, optional has a default constructor
  • blender:: is unnecessary here
- `= {}` is unnecessary, `optional` has a default constructor - `blender::` is unnecessary here
casey-bianco-davis marked this conversation as resolved
@ -0,0 +121,4 @@
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(tc.obedit->data);
grease_pencil.foreach_editable_drawing(
scene->r.cfra, [&](int /*layer_index*/, blender::bke::greasepencil::Drawing &drawing) {
Member

blender:: is unnecessary here

`blender::` is unnecessary here
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 2 commits 2023-10-20 01:55:59 +02:00
Author
Member

Would you be interested in adding the SHRINKFATTEN mode to curves edit mode too (the new "hair" curves type). It would be really nice to be able to take advantage of the shared code and also be able to adjust the radius there too. That change could be a separate PR if you preferred too.

I may be interested in trying SHRINKFATTEN but I think it be better as a separate pull request.

> Would you be interested in adding the `SHRINKFATTEN` mode to curves edit mode too (the new "hair" curves type). It would be really nice to be able to take advantage of the shared code and also be able to adjust the radius there too. That change could be a separate PR if you preferred too. I may be interested in trying `SHRINKFATTEN` but I think it be better as a separate pull request.
Hans Goudey changed title from GPv3: Add edit mode transform operator (i.e translate, rotation, scale, opacity, radius) to GPv3: Transform operators (translate, rotate, scale, opacity, radius) 2023-10-20 09:22:15 +02:00
Hans Goudey approved these changes 2023-10-20 10:13:14 +02:00
casey-bianco-davis added 1 commit 2023-10-21 01:37:57 +02:00
Falk David merged commit e266b142f6 into main 2023-10-21 15:12:57 +02:00
casey-bianco-davis deleted branch GPv3-transform 2023-10-21 21:21:47 +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
4 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#111836
No description provided.