GPv3: Move to layer #117244
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
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#117244
Loading…
Reference in New Issue
No description provided.
Delete Branch "mendio/blender:GPv3_OP_Move_to_layer"
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?
This PR adds the Move to Layer operator from GPv2.
@ -1789,0 +1867,4 @@
curves_num += c_src + c_dst;
/* got an Assert with this line. */
curves_dst.resize(points_num, curves_num);
@HooglyBoogly do you mind to look at this? I've got an assert with this Resize line. Points and curves numbers seems to be right in my tests
When adding curves, you have to set the new curve offsets too, otherwise the new values will be uninitialized.
Side note, this code would be easier to read using full words instead of "p" "c" etc. And if the new sizes were declared as
const int ... =
in one line.If you end up using
geometry::join_geometries
though, it shouldn't be necessary to resize curves manually at all though.Thanks! do you have any code sample using
geometry::join_geometries
maybe is the right way to do thisNo great example yet. The general structure would look like this though:
Thanks Hans! after some tweaks of your code I've got this working, I'll remove the WIP to the title and wait for a more indeep review
WIP: GPv3 Move to layerto GPv3: Move to layer@ -1605,0 +1645,4 @@
/* Iterate through all the drawings at current scene frame. */
const Array<MutableDrawingInfo> drawings_src = retrieve_editable_drawings(*scene, grease_pencil);
for (const MutableDrawingInfo &info : drawings_src) {
use
threading::parellel_for
? :)It seems not possible, in Separate operator was told me that frames can't be inserted into a grease pencil data-block in parallel
ok, wasn't aware of that. I'll note this, thanks :)
@ -1605,0 +1653,4 @@
continue;
}
if (grease_pencil.get_drawing_at(*layer_dst, info.frame_number) == nullptr) {
!layer_dst.has_drawing_at(frame_num)
looks more suitableinfo.frame_number
orscene->r.cfra
. Not sure which is correct here (haven't checked this locally yet 😅)@ -1605,0 +1658,4 @@
grease_pencil.insert_blank_frame(*layer_dst, info.frame_number, 0, BEZT_KEYTYPE_KEYFRAME);
/* Copy strokes to new CurvesGeometry. */
Drawing &drawing_dst = *grease_pencil.get_editable_drawing_at(*layer_dst, info.frame_number);
drawing_dst.strokes_for_write() = bke::curves_copy_point_selection(
It seems this will create new drawing of selected points.
But in GPv2, the entire stroke is moved the dst_layer.
So I think we should use
retrieve_selected_points
andcurves_copy_curve_selection
Also use
remove_curves
instead ofremove_points
@ -1605,0 +1612,4 @@
static int grease_pencil_move_to_layer_exec(bContext *C, wmOperator *op)
{
using namespace bke::greasepencil;
Scene *scene = CTX_data_scene(C);
const
@ -1605,0 +1618,4 @@
Object *object = CTX_data_active_object(C);
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(object->data);
Layer *layer_dst = nullptr;
int layer_index = RNA_int_get(op->ptr, "layer");
const
I'm seeing that there is a special handling to add a new layer in
move_to_layer
. We shouldn't be doing this. There is already alayer_add
operator, so we should be using that.@ -277,0 +282,4 @@
layout.operator_context = 'INVOKE_REGION_WIN'
obd = context.active_object.data
layout.operator("grease_pencil.move_to_layer", text="New Layer", icon='ADD').layer = -1
Use
layer_add
here. This can also be invoked as a popup if that's what you want to do.in GPv2 we follow the Move to Collection behavior, when the user wants to move an object to a new collection, the name of the new collection can be typed in a popup window. Maybe I'm wrong but is this not possible with
layer_add
? At this point always use the name "Layer" for new layers. This seems like a regression from GPv2 because the user has to change the name later in the layer list, which slows down the workflow. This also applies to set Active Layer operator.Right, the
layer_add
is just missing aninvoke
:)Try this:
And then just use
layer_add
in the menu :)perfect, that works, also the set Active Layer operator is working as intended
Another pass :)
@ -277,0 +280,4 @@
def draw(self, context):
layout = self.layout
layout.operator_context = 'INVOKE_REGION_WIN'
obd = context.active_object.data
obd
->grease_pencil
@ -277,0 +282,4 @@
layout.operator_context = 'INVOKE_REGION_WIN'
obd = context.active_object.data
nlop = layout.operator("grease_pencil.layer_add", text="New Layer", icon='ADD')
This is also fine:
layout.operator("grease_pencil.layer_add", text="New Layer", icon='ADD').new_layer_name = "Layer"
When doing this way the Layer add popup shows empty
sorry, the problem was a typo
@ -13,6 +13,7 @@
#include "BLI_math_vector_types.hh"
#include "BLI_span.hh"
#include "BLI_stack.hh"
#include "BLI_string.h"
I think this include is not needed.
@ -41,3 +43,4 @@
#include "WM_api.hh"
#include "UI_resources.hh"
using namespace blender::bke;
This should be removed. If a function needs this, it can be added in that function.
@ -1605,0 +1624,4 @@
/* get layer by index */
layer_dst = grease_pencil.layers_for_write()[layer_index];
}
else {
This
else
case can be removed.You should be able to do
Layer *layer_dst = grease_pencil.layers_for_write()[layer_index];
directly.@ -16,6 +16,8 @@
#include "ED_keyframes_edit.hh"
#include "WM_api.hh"
I think this include is no longer needed
@filedescriptor right now because we are no longer use the invoke in the operator when adding a new layer the code never reach
grease_pencil_move_to_layer_exec
, it only works when we select a layer that already exist.Do we need to use a macro to solve this or what do you propose?
Hi, found some "cleanup" changes.
@ -277,0 +281,4 @@
layout = self.layout
layout.operator_context = 'INVOKE_REGION_WIN'
grease_pencil = context.active_object.data
trailing whitespace
@ -277,0 +283,4 @@
grease_pencil = context.active_object.data
layout.operator("grease_pencil.layer_add", text="New Layer", icon='ADD').new_layer_name = "Layer"
trailing whitespace
@ -1605,0 +1650,4 @@
drawing_dst.tag_topology_changed();
}
else {
/* For existing Layers append the strokes to new CurvesGeometry. */
I think: For existing "drawing"
I rephrased the comments for clarity.
@ -1605,0 +1669,4 @@
info.drawing.tag_topology_changed();
changed = true;
};
;
is not required 🙂Some more cleanup comments.
@ -1605,0 +1638,4 @@
}
if (!layer_dst->has_drawing_at(info.frame_number)) {
/* For new layers Insert Keyframe at current frame/layer. */
/* If the target layer does not have a keyframe at the current frame, insert a new keyframe. */
I rephrased the comments in the
if...else
to clarify that one move/copy geometry and the other append to existing geometry.Let me know if you think more detail is still needed in this comments.
@ -1605,0 +1691,4 @@
/* callbacks. */
ot->exec = grease_pencil_move_to_layer_exec;
ot->poll = ot->poll = editable_grease_pencil_poll;
Double
ot->poll =
Just tested the PR. Looks correct apart from this one change 🙂
@ -1605,0 +1641,4 @@
/* For new layers Insert Keyframe at current frame/layer. */
grease_pencil.insert_blank_frame(*layer_dst, info.frame_number, 0, BEZT_KEYTYPE_KEYFRAME);
/* Copy strokes to new CurvesGeometry. */
Drawing &drawing_dst = *grease_pencil.get_editable_drawing_at(*layer_dst, info.frame_number);
drawing_dst
isnullptr
when dst layer is locked.@ -1605,0 +1651,4 @@
}
else {
/* For existing Layers append the strokes to new CurvesGeometry. */
Drawing &drawing_dst = *grease_pencil.get_editable_drawing_at(*layer_dst, info.frame_number);
drawing_dst
isnullptr
when dst layer is locked.@mendio Yes, I think an operator macro makes sense. Maybe
GREASE_PENCIL_OT_move_to_new_layer
.@ -1605,0 +1699,4 @@
/* Grease Pencil layer to use. */
prop = RNA_def_int(ot->srna, "layer", 0, -1, INT_MAX, "Grease Pencil Layer", "", -1, INT_MAX);
RNA_def_property_flag(prop, PROP_HIDDEN | PROP_SKIP_SAVE);
RNA_def_string(
This prop should be removed now.
@ -1605,0 +1697,4 @@
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
/* Grease Pencil layer to use. */
prop = RNA_def_int(ot->srna, "layer", 0, -1, INT_MAX, "Grease Pencil Layer", "", -1, INT_MAX);
It might make more sense to make this a
RNA_def_string
with thelayer_name
. Because the layer that we move to always has to exist.maybe I don't get what is your idea. Right now
layer
property stores the layer index and that is what we use ingrease_pencil_move_to_layer_exec
to determine the target layer. The property that there is no longer needed isnew_layer_name
because now we uselayer_add
operator to handle adding layers.Yes I'm not talking about the
new_layer_name
. I mean that thelayer
(or maybe a better name would betarget_layer
) that we move the selection into could be astring
property with the name rather than an index. Maybe that makes the setup for the macro a bit easier (since we'd need to find the index of the newly added layer, but we know the name already).I see, just updated the
grease_pencil_move_to_layer_exec
to use an string property.I guess we only have to tackle the macro. Do you have a sample for me to see?
Thanks. Looks correct to me :)
Falk/Hans may have some suggestions
It looks like the option to move the strokes to a new layer has not be implemented yet. I would add this operator macro in this PR as well. It should only require a few lines.
By default selected strokes are moved to new/chosen layer here (like legacy operator). Maybe I'm misunderstanding your request? 🙂
Edit: Ah got it, "new" layer 😅
I started to implement the macro to add a new layer and move the strokes.
The code still have problems, the macro adds the new layer but
GREASE_PENCIL_OT_move_to_layer
is not executed@mendio I updated the code since I couldn't make the macro work nicely. It was breaking when the new layer name was already taken by a layer. I replaced the logic with a boolean property on the
move_to_layer
operator that adds a layer (similar to what was there before). Sorry for the trouble.