GPv3: Implement edit mode undo #117072
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#117072
Loading…
Reference in New Issue
No description provided.
Delete Branch "mont29/blender:gp_undo"
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?
Code seems to work now, though it still needs way more testing.
Layer
copy constructor does not currently handles masks?).BKE_customdata
API.NOTE: This PR currently contains changes (the new assignment operator for the
LayerGroup
class...) that should be committed separately.@ -0,0 +38,4 @@
* \{ */
struct StepDrawingGeometry {
bke::CurvesGeometry geometry = {};
bke::CurvesGeometry
already has a default constructor, so the= {}
is unnecessary here@ -0,0 +39,4 @@
struct StepDrawingGeometry {
bke::CurvesGeometry geometry = {};
uint32_t index;
Might as well use
int
like the style guide recommends@ -0,0 +95,4 @@
}
}
new (&drawings_geometry) Array<StepDrawingGeometry>(drawings_geometry_num);
These two arrays are already default constructed when their owner
StepObject
was constructed. The only reason placement new was used there is that theGreasePencilUndoStep
is allocated by C code that doesn't properly initialize its members. SoGreasePencilUndoStep
is still uninitialized but allocated memory, meaning we have to construct theStepObject
array in place. That doesn't apply here. So it should be fine to usethis->drawings_geometry.reinitialize(drawings.size())
@ -0,0 +100,4 @@
uint32_t drawings_geometry_idx = 0;
uint32_t drawings_reference_idx = 0;
for (int idx = 0; idx < grease_pencil.drawing_array_num; idx++) {
Seems preferable to use the bke wrapped grease pencil types. Then this loop can be written like:
#109123 (outdated though)
@ -0,0 +68,4 @@
void decode(GreasePencil & /* grease_pencil */) const {}
};
struct StepObject {
I think this also needs to encode:
GreasePencil.layers_data
)But, I thought you told me that the layer tree could not be modified in Edit mode?
We may have missunderstood each other then. In edit mode, the user should be able to freely edit the layers and the keyframes (stored on the layer frames map).
@ -841,0 +882,4 @@
runtime = MEM_new<blender::bke::greasepencil::LayerRuntime>(__func__);
}
for (int i = 0; i < frames_storage.num; i++) {
frames_for_write().add_new(frames_storage.keys[i], frames_storage.values[i]);
Retrieve
frames_for_write
once and store it in a variable@ -899,1 +947,4 @@
LayerGroup &LayerGroup::operator=(const LayerGroup &other)
{
this->~LayerGroup();
This need to check if other points to
this
, where it shouldn't do anything@ -1109,0 +1175,4 @@
child->as_group().prepare_for_dna_write();
break;
}
default:
Typically we avoid adding default to make sure we get compile warnings when adding new enum values. Same at a few other places in this PR
@ -0,0 +71,4 @@
struct StepDrawingGeometryBase {
/* Index of this drawing in the original combined array of all drawings in GreasePencil ID. */
uint index;
uint index
->int index
Can you explain why you are so strong about using
int
everywhere?In this case, the value should never ever be negative, so using
uint
makes more sense to me...See https://developer.blender.org/docs/handbook/guidelines/c_cpp/#integer-types
Indices should be signed, especially since the drawing API uses signed integers everywhere else.
@ -0,0 +81,4 @@
bke::CurvesGeometry geometry;
void encode(const GreasePencilDrawing &drawing_geometry,
const int64_t drawing_index,
Can use
int
here tooindex_range
generatesint64_t
, would rather keep the conversion toint
as close to the actual assignment as possible. Also allows for the assert that given value is actually within valid positiveint
range.@ -0,0 +96,4 @@
MutableSpan<GreasePencilDrawingBase *> drawings = grease_pencil.drawings();
if (drawings[index] == nullptr) {
drawings[index] = reinterpret_cast<GreasePencilDrawingBase *>(
MEM_new<blender::bke::greasepencil::Drawing>(__func__));
We're already in the
blender
namespace here, no need to specify that again. Same below@ -0,0 +176,4 @@
drawings_reference.reinitialize(drawings_reference_num);
uint drawings_geometry_idx = 0;
uint drawings_reference_idx = 0;
uint
->int
@blender-bot build
First pass. I also tested this a bit locally and it generally seems to work fine. Sometimes I've had cases where there were lots of undo steps that didn't seem to do anything. Not sure what that is about.
@ -841,0 +875,4 @@
/* Re-create frames data in runtime map. */
/* NOTE: Avoid re-allocating runtime data to reduce 'false positive' change detections from
* memfile undo. */
if (runtime) {
Should use
this->runtime
so that the scope is clear.(resolving these for the time being, this is adding way too much noise for current stage of review and is not clearly agreed on AFAIK).
'Addressed' mainly by moving to classes and private members, using
this
now when accessing methods or public members.@ -1108,1 +1169,4 @@
void LayerGroup::prepare_for_dna_write()
{
LISTBASE_FOREACH (TreeNode *, child, &children) {
this->children
@ -1109,0 +1185,4 @@
void LayerGroup::update_from_dna_read()
{
LISTBASE_FOREACH (TreeNode *, child, &children) {
Same as above.
@ -0,0 +49,4 @@
*
* Each drawing type has its own array in the undo #StepObject data.
*
* NOTE: Current code assumes that drawing can only be added or removed, but never re-ordered.
Hm is this still true? When e.g. deleting a keyframe, or deleting a layer, the order of the drawings in the array changes.
Yes, but I indeed was suspecting this assumption was not correct. Will fix, thx.
@ -0,0 +94,4 @@
void decode(GreasePencil &grease_pencil, StepDecodeStatus & /*decode_status*/) const
{
MutableSpan<GreasePencilDrawingBase *> drawings = grease_pencil.drawings();
if (drawings[index] == nullptr) {
Use
this->index
@ -0,0 +210,4 @@
void encode_layers(const GreasePencil &grease_pencil, StepEncodeStatus & /*encode_status*/)
{
layers_num = int(grease_pencil.layers().size());
Use
this->
, same for other occurences.@ -0,0 +278,4 @@
Scene *scene = CTX_data_scene(C);
ViewLayer *view_layer = CTX_data_view_layer(C);
uint objects_num = 0;
Object **objects = ED_undo_editmode_objects_from_view_layer(scene, view_layer, &objects_num);
Would be good to use
BLI_SCOPED_DEFER([&]() { MEM_SAFE_FREE(objects); });
here. And remove the free further down.@ -0,0 +281,4 @@
Object **objects = ED_undo_editmode_objects_from_view_layer(scene, view_layer, &objects_num);
us->scene_ref.ptr = scene;
new (&us->objects) Array<StepObject>(objects_num);
I think you can just use
us->objects.reinitialize(objects_num)
From what I understood (hopefully @HooglyBoogly can confirm or tell me am wrong ;)), this is not possible here because the
GreasePencilUndoStep
is created from a C-type allocation (malloc
), so no constructor is called forus->objects
.At least Curves undo code also does the same 'placement new' (see curves_undo.cc#L61).
This is also why we need an explicit call the this array's destructor in the
free
callback below.Yes, you're right. I assumed the constructor was called.
@blender-bot build
@mont29 Ah sorry, you need to pull now before pushing again...
@ -0,0 +114,4 @@
};
class StepDrawingGeometry : public StepDrawingGeometryBase {
private:
Class members are private by default, no need to specify it here
@mont29 Seems like this can be moved out of WIP state? Or do you have some more TODOs that you need to work on?
WIP: undo handling for GreasePencil v3.to Implement edit mode undo for GreasePencil v3.Implement edit mode undo for GreasePencil v3.to GPv3: Implement edit mode undo for GreasePencil v3GPv3: Implement edit mode undo for GreasePencil v3to GPv3: Implement edit mode undoDid another pass, found a few more things.
@ -1566,1 +1578,4 @@
void GreasePencil::resize_drawings(const int new_num)
{
using namespace blender;
This should assert when
new_num
is not greater than 0.@ -1567,0 +1589,4 @@
}
else { /* if (new_num < prev_num) */
const int shrink_num = prev_num - new_num;
MutableSpan<GreasePencilDrawingBase *> old_drawings = this->drawings().drop_front(prev_num);
Shouldn't this be
drop_front(new_num)
? Seem like we want to free all the drawings except the ones that will remain after shrinking, so that should benew_num
number of drawings.@ -0,0 +141,4 @@
drawing_geometry.base.flag = flag_;
drawing_geometry.geometry.wrap() = geometry_;
/* TODO: Check if there is a way to tell if both stored and current geometry are still the
The copy-assignment constructor will use the copy constructor which will copy the shared caches. These caches remain valid (because any other user that tags them dirty will make a copy for itself), so we shouldn't tag them here.
I had to add this to avoid crashes in drawing code, especially when deleting some points in a curve. As far as I understand, it's because here we copy only the
geometry
part, not the wholeDrawing
data, so the runtime data stays in its previous state, and may not be compatible with current data ingeometry
anymore?Ah, sorry, I missread the code.
Maybe it would be better to just store the
bke::greasepencil::Drawing
then?Otherwise, we do have to tag everything, yes.
Since runtime caches are shared, could be an option... Although it would still end up having a significant impact on memory used by undo of greasepencil imho?
Hm, the drawing is basically just the curves geometry. And the caches will be mostly shared. So I don't think so? We could do some measurements if it's a concern.
The cache won't be shared anymore once user edit the geometry. This means that it can end up, for the edited drawing, with copies of all caches in undo steps that are only relevant for the undo steps.
Indeed measurement is needed, I suspect the overhead will vary widely depending on the use case (one giant single drawing vs. hundreds of simpler drawings, where only one drawing is edited at a time).
There could always be some code that manually frees up memory in undo steps by clearing caches that aren't shared with currently visible geometry. That would be nice for other undo systems as well.
@ -515,0 +515,4 @@
/**
* Low-level resizing of drawings array. Only allocates new entries in the array, no drawings are
* created in case of size increase.
This comment should mention that drawings will be deleted when the array is shrunk (might be obvious, but better to be explicit about this stuff).
Seems like this is done. Only remaining questions for me are:
Drawing
directly or if that will result in too many caches being duplicated over timeAfter talking with @filedescriptor yesterday, here are our conclusions regarding these two points:
This would likely not be an ideal solution. It would end up storing a lot of extra data from the caches, used only by that undo step. Unless some form of
non-owning
shared pointer can be added - but this is not existing currently.In general, the idea of storing caches in undo steps is not great anyway.
However, it should be possible to check the the related geometry remains unchanged and therefore the cached triangulated curve does not need to be tagged as dirty.
We agreed that this is not very high priority. The usage of shared pointers makes it non-trivial to compute the amount of memory actually used by an undo step, and even worse, this amount will change over time (since we can assume that once undo steps are the sole owners of a shared pointer, then they actually 'consume' that memory). Proper support for this will likely require changes to how the undo stack itself handles this 'used memory' info.
d4f29eac6d is a very basic initial attempt at detecting 'modified' drawings (between current one and data stored in undo step).
@HooglyBoogly @JacquesLucke would be very interested to know if you think this is a valid idea? If yes, this likely needs to be worked on in its own separate task anyway.
I'd try to stay away from the term "shallow copy" in this context because it's quite ambiguous imo. Also you don't really check if one was copied from the other, but if they are same. So maybe something like
is_equal_fast
is more correct. Should still have a comment for why it's fast of course.Besides that, it would of course be nice to side-step this kind of check completely by using implicit-sharing (which can't be done yet, but would be good to check if this could in theory be applied here).
I think this should work too. I'd just phrase it slightly differently. The undo system can modify/delete the shared cache data if it is sure that it is the sole owner of the data. This is a new concept and probably requires small changes to
BLI_implicit_sharing.hh
but currently I don't see a fundamental reason for why this shouldn't work.d4f29eac6d
to4d04c6a7a7
4d04c6a7a7
toe633079568
Rebased on Main on rewrote commit history, this should be ready for merge.
@blender-bot build