GPv3: Port frame_clean_duplicate
operator #116655
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
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#116655
Loading…
Reference in New Issue
No description provided.
Delete Branch "amsaid1989/blender:gpv3-delete-duplicate-frames"
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?
Port the
frame_clean_duplicate
operator to GPv3:GREASE_PENCIL_OT_frame_clean_duplicate
operatorHi, I checked the PR locally, found few points
@ -266,2 +279,3 @@
if (keyframe_region_circle_test(static_cast<const KeyframeEdit_CircleData *>(ked->data),
pt)) {
pt))
{
accidental formatting change. Can you revert this? :)
Reverted it
@ -359,0 +466,4 @@
bool attrs_equal = true;
attribute_math::convert_to_static_type(attrs_a.varray.type(), [&](auto dummy) {
not sure if this is needed. You could just call
std::equal()
in current lamda too, no?I might be misunderstanding something
This was implemented based on guidance I got in blender.chat from @mod_moder when I started working on this. This is what was recommended to me to compare all the attributes.
@ -359,0 +507,4 @@
continue;
}
std::optional<bke::CurvesGeometry *> current_geo = get_gp_curves_geometry_from_frame_key(
wouldn't
get_editable_drawing_at
work here?You're right..
get_editable_drawing_at
works. Updated it@ -359,0 +509,4 @@
std::optional<bke::CurvesGeometry *> current_geo = get_gp_curves_geometry_from_frame_key(
grease_pencil, layer, current);
std::optional<bke::CurvesGeometry *> next_geo = get_gp_curves_geometry_from_frame_key(
same as above.
Also names can be improved. eg:
curves
andcurves_next
Switched to
get_editable_drawing_at
and renamed the variables@ -359,0 +523,4 @@
frames_to_delete.append(next);
}
for (const FramesMapKey frame : frames_to_delete) {
grease_pencil.remove_frames()
When I try to use this
grease_pencil.remove_frames(*layer, frames_to_delete);
I get a crash. I ran Blender in a debugger to see if I can find what is causing the crash, but I haven't figured out the reason. I am sure I am missing something with this being my first time working with Blender's code. This is the output I get in the terminal after running the test file I am using. I can't see what might be causing the problem so I thought I would share it here in case someone can figure it out.BTW, PR looks ready enough. Can we remove WIP tag?
Thank you for the review @PratikPB2123.. Sorry, I've been busy at work and missed the notification with your comments. I will address the feedback and remove the
WIP
tag once done.WIP: GPv3: Port `frame_clean_duplicate` operatorto GPv3: Port `frame_clean_duplicate` operator@PratikPB2123 I addressed most of your feedback, but not sure what you want me to do regarding the 2 comments I left above.
Hi, sorry I missed the notification. Will check again :)
Since this is mostly about attributes, I think Falk and Hans are perfect for the further review.
Hi, can you merge latest main branch in your PR to update it? After
1b65faddd0
get_editable_drawing_at
now requires layer reference instead of pointer.To update PR branch (also see this docs):
make update
on main branch@ -359,0 +490,4 @@
continue;
}
Drawing *drawing = grease_pencil.get_editable_drawing_at(layer, current);
get_editable_drawing_at
accepts layer reference. So pass*layer
@ -359,0 +491,4 @@
}
Drawing *drawing = grease_pencil.get_editable_drawing_at(layer, current);
Drawing *drawing_next = grease_pencil.get_editable_drawing_at(layer, next);
same as above
Updated all uses of
get_editable_drawing_at
to pass aLayer &
instead ofLayer *
bit confused about these 2 checks. This will fail the operation in some cases.
For example: when keyframe is not duplicated but instead the strokes are copied between two adjacent drawings
@ -359,0 +388,4 @@
}
if (attrs_span_a.data() != attrs_span_b.data()) {
return false;
I'm not sure why this check is needed.
This should be
Make sense. Maybe same for other
if()
that I mentionedUpdated the check to what @mod_moder suggested
@ -359,0 +431,4 @@
const Set<AttributeIDRef> ids_a = attributes_a.all_ids();
const Set<AttributeIDRef> ids_b = attributes_b.all_ids();
if (ids_a != ids_b) {
Not sure why this check is needed.
In case lists of attributes are different, i think this is reasonable to treat such curves as different..
I will leave this check as is since @mod_moder agrees it's needed
I see, make sense.
@PratikPB2123 I have merged the
main
branch and addressed the feedback.Hi, thanks for updating the PR. Works correctly for me so far. AFAICS, only
"BKE_curves.hh"
header file is required. Code complies fine without rest of headers that you added :)@ -30,2 +42,4 @@
#include "WM_api.hh"
#include <algorithm>
#include <optional>
These headers are not needed.
Also add operator in UI, this menu:
VIEW3D_MT_edit_greasepencil_cleanup
@PratikPB2123 Done. Removed the unnecessary header and added the operator to the menu.
Thanks. Among the all headers you added, only
"BKE_curves.hh"
is needed. You can remove rest all.Sorry. I misunderstood and thought you only wanted me to remove the 2 headers you mentioned. All removed now.
Hi, found a crash with empty drawings. Attaching .blend file in case needed
@ -412,0 +462,4 @@
if (curves_a.curves_num() != curves_b.curves_num() ||
curves_a.points_num() != curves_b.points_num() || curves_a.offsets() != curves_b.offsets())
{
here offset() memory is garbage if curves has empty drawing. This would lead to crash. Just add one if condition before this to return true when both the curves has 0 points:
Fixed it. Thank you very much for catching it
Thanks. Looks correct to me 🙂
Let's wait for Falk/Hans/Iliya for further review of attribute comparison code.
@ -412,0 +423,4 @@
const GSpan attrs_span_a = attrs_a.varray.get_internal_span();
const GSpan attrs_span_b = attrs_b.varray.get_internal_span();
if (attrs_span_a.size() != attrs_span_b.size()) {
You have check size few lines above, right?
Removed this extra size check
@ -412,0 +487,4 @@
return false;
}
bool attrs_equal = true;
attrs_equal
->attributes_are_equal
Renamed the variable
@ -412,0 +520,4 @@
const Span<FramesMapKey> &keys = layer->sorted_keys();
Vector<FramesMapKey> frames_to_delete = {};
for (size_t i = 0; i < keys.size(); ++i) {
for (size_t i = 0; i < keys.size(); ++i) {
andif (i + 1 >= keys.size()) {
->
const int i : keys.index_range().drop_front(1)
.... ?
I needed
const int i : keys.index_range().drop_back(1)
instead ofconst int i : keys.index_range().drop_front(1)
@ -412,0 +525,4 @@
break;
}
FramesMapKey current = keys[i];
Can be const?
Changed to
const
Thank you for your help, @PratikPB2123 😊
@mod_moder Let me know if there are any changes you would like me to make
@ -413,0 +429,4 @@
}
}
return true;
Should be false.
Done
@ -413,0 +481,4 @@
GAttributeReader attrs_b = attributes_b.lookup(id);
if (!attributes_varrays_are_equal(attrs_a, attrs_b)) {
return false;
Done
@ -413,0 +515,4 @@
}
const Span<FramesMapKey> &keys = layer->sorted_keys();
Vector<FramesMapKey> frames_to_delete = {};
Vector<FramesMapKey> frames_to_delete;
Done
@mod_moder Addressed your latest feedback
@ -413,0 +489,4 @@
attribute_math::convert_to_static_type(attrs_a.varray.type(), [&](auto dummy) {
using T = decltype(dummy);
attributes_are_equal = attributes_elements_are_equal<T>(attrs_a, attrs_b);
Right now this is incorrect.
if (attributes_varrays_are_equal(attrs_a, attrs_b)) {
is used for early skip if this is possible (arrays is actually is the same data segment in memory).But right now you also have to check if this is obviously impossible (different types or sizes).
If you'll do this cast for attributes with different sizes/types, this will be UB and assertion in debug.
You need to explicitly check if
attributes_varrays_not_equal
before of this.And additional cleanup comment: upcast to static type for attributes should be on the caller, not in the template method.
@mod_moder I have addressed your comments about the varrays check.
Renamed
attributes_varrays_are_equal
toattributes_varrays_span_data_equal
and updated it to this:Then, I added
attributes_varrays_not_equal
which is implemented as follows:These 2 functions are then used like this:
Does this make more sense?
Regarding this, I would like to confirm that I understand you correctly.
You want to change the lambda in
convert_to_static_type
to this:And change the
attributes_elements_are_equal
to the following:Order of
attributes_varrays_span_data_equal
andattributes_varrays_not_equal
can be changed (first - early skip of wrong types and sizes, and next early confirm that data is the same). This looks good now.Yes, this is correct.
Great.. Thank you very much. Updated the PR.
Checkout
From your project repository, check out a new branch and test the changes.