Page MenuHome
Differential D16979

Curves: Add initial undo system
ClosedPublic
Actions

Authored by Falk David (filedescriptor) on Wed, Jan 11, 5:57 PM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Commits
rBd650162ecd47: Curves: Add initial undo system
Summary

For now this will only save the current CurvesGeometry for every step.

Diff Detail

Repository
rB Blender
Branch
curves-undo-system (branched from master)
Build Status
Buildable 25325
Build 25325: arc lint + arc unit

Event Timeline

Falk David (filedescriptor) requested review of this revision.Wed, Jan 11, 5:57 PM
Falk David (filedescriptor) created this revision.
Harbormaster completed remote builds in B25325: Diff 59332.Wed, Jan 11, 5:57 PM
Falk David (filedescriptor) updated this revision to Diff 59333.Wed, Jan 11, 5:58 PM

Remove parts of other diff

Harbormaster completed remote builds in B25326: Diff 59333.Wed, Jan 11, 5:58 PM
Philipp Oeser (lichtwerk) added a subscriber: Philipp Oeser (lichtwerk).Wed, Jan 11, 6:00 PM
Falk David (filedescriptor) retitled this revision from Curves: Add initial undo system to Curves: Add initial undo system (WIP).Wed, Jan 11, 6:04 PM
Falk David (filedescriptor) edited the summary of this revision. (Show Details)
Falk David (filedescriptor) edited the summary of this revision. (Show Details)Wed, Jan 11, 6:07 PM
Falk David (filedescriptor) added a reviewer: Hans Goudey (HooglyBoogly).
Falk David (filedescriptor) removed a task: T101890: Curves editmde: support selection operators from sculptmode.Wed, Jan 11, 6:18 PM
Hans Goudey (HooglyBoogly) added a comment.Wed, Jan 11, 9:11 PM

Nice that this is relatively straightforward, thanks for working on it!

source/blender/editors/curves/intern/curves_undo.cc
28–143

Might be nicer to put all this in the blender::ed::curves namespace and add a using to ED_curves_undosys_type

71–72
94

Nitpicky thing

Falk David (filedescriptor) updated this revision to Diff 59347.Thu, Jan 12, 12:11 PM
  • Add namespace and fix minor issues
Harbormaster completed remote builds in B25331: Diff 59347.Thu, Jan 12, 12:11 PM
Falk David (filedescriptor) added a project: Nodes & Physics.Thu, Jan 12, 4:27 PM
Falk David (filedescriptor) updated this revision to Diff 59422.Fri, Jan 13, 6:35 PM
  • Merge branch 'master' into curves-undo-system
  • Cleanup
  • Make sure to use the undo system only in edit mode
  • Fix memory leaks
Harbormaster completed remote builds in B25364: Diff 59422.Fri, Jan 13, 6:35 PM
Falk David (filedescriptor) retitled this revision from Curves: Add initial undo system (WIP) to Curves: Add initial undo system.Fri, Jan 13, 6:37 PM
Hans Goudey (HooglyBoogly) added inline comments.Fri, Jan 13, 7:02 PM
source/blender/editors/curves/intern/curves_undo.cc
32

bke::CurvesGeometry here, right? Then the casting wouldn't be necessary to delete it below.

69

Use static cast here (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast)

124

NULL -> nullptr

Falk David (filedescriptor) updated this revision to Diff 59441.Sat, Jan 14, 6:43 PM
  • Cleanup
Harbormaster completed remote builds in B25370: Diff 59441.Sat, Jan 14, 6:43 PM
Falk David (filedescriptor) added inline comments.Mon, Jan 16, 2:54 PM
source/blender/editors/curves/intern/curves_undo.cc
69

The compiler says that a static cast is not possible here.

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Mon, Jan 16, 3:50 PM
Hans Goudey (HooglyBoogly) requested changes to this revision.Mon, Jan 16, 4:26 PM

Looking at this a bit deeper, IMO it's copying the structure of the existing edit mode implementations a bit more than it needs to. I think with some cleanups possible with C++, the code becomes simpler.

I did changes not mentioned in inline comments here P3445:

  • Use term "objects" instead of "elems"
  • Use Array to store data for each object rather than raw allocation
  • Remove redundant blender:: namespace specification
  • Store bke::CurvesGeometry by value rather than by pointer
  • Remove comments that just said what the next line does
  • Tweak whitespace
  • Add parallelism over multiple objects (not really needed now, but why not!)

My point isn't that each of these things is essential, but just that we should push a bit further to make the code semantically correct and to use some of the tools we have for simplifying basic things.

source/blender/editors/curves/intern/curves_undo.cc
30–50

Since these structs are in the ed::curves namespace, theoretically their names could be simplified.
At the extreme, in an ed::curves::undo namespace, the names could be Step, etc.

52–65

There is a lot of redundancy in these checks right now. For example, a null active object and checking the object type and checking if it's editable happen multiple times.
Ideally functions would be reused properly such that each check is only done one. To me it looks like the new part here is checking for edit mode as well. I'd suggest adding a function like editable_curves_in_edit_mode_poll near the others

69

Ah, right, then reinterpret_cast is the way to go. The main point is avoiding C-style casts.

This revision now requires changes to proceed.Mon, Jan 16, 4:26 PM
Falk David (filedescriptor) marked 5 inline comments as done.Mon, Jan 16, 4:31 PM
Paul Larson (GeorgiaPacific) added a subscriber: Paul Larson (GeorgiaPacific).Tue, Jan 17, 4:37 AM
Falk David (filedescriptor) mentioned this in D17047: Curves: Edit mode selection operators.Thu, Jan 19, 5:53 PM
Falk David (filedescriptor) updated this revision to Diff 59652.Thu, Jan 19, 6:16 PM
  • Merge branch 'master' into curves-undo-system
  • Address feedback
Harbormaster completed remote builds in B25460: Diff 59652.Thu, Jan 19, 6:16 PM
Falk David (filedescriptor) added inline comments.Thu, Jan 19, 6:21 PM
source/blender/editors/curves/intern/curves_undo.cc
52–57

Not sure I agree with how this is written right now. It feels like it's "forcing" the RAII paradigm, but using a C API. I'm all for using more RAII, but maybe ED_undo_editmode_objects_from_view_layer should be converted to C++ first, or a new C++ API added.

Hans Goudey (HooglyBoogly) accepted this revision.Thu, Jan 19, 6:25 PM

Thanks!

source/blender/editors/curves/intern/curves_undo.cc
52–57

Reverting that part is fine with me. It is a bit complex. The part I like more is storing bke::CurvesGeometry by value next to UndoRefID_Object, I think that's more reasonable

139

Is this kept for a specific reason? just curious

This revision is now accepted and ready to land.Thu, Jan 19, 6:25 PM
Falk David (filedescriptor) marked 4 inline comments as done.Thu, Jan 19, 6:26 PM
Charlie Jolly (charlie) awarded a token.Thu, Jan 19, 6:27 PM
Falk David (filedescriptor) updated this revision to Diff 59655.Thu, Jan 19, 6:30 PM
  • Revert name changes to sculpt selection operators
Harbormaster completed remote builds in B25463: Diff 59655.Thu, Jan 19, 6:31 PM
Falk David (filedescriptor) updated this revision to Diff 59657.Thu, Jan 19, 6:32 PM
  • Fix wrong diff
Harbormaster completed remote builds in B25465: Diff 59657.Thu, Jan 19, 6:32 PM
Falk David (filedescriptor) updated this revision to Diff 59658.Thu, Jan 19, 6:41 PM
  • Revert use of BLI_SCOPED_DEFER
Harbormaster completed remote builds in B25466: Diff 59658.Thu, Jan 19, 6:41 PM
Falk David (filedescriptor) mentioned this in D17063: Curves: Add initial transform support.Fri, Jan 20, 12:45 PM
Closed by commit rBd650162ecd47: Curves: Add initial undo system (authored by Falk David (filedescriptor)). · Explain WhyFri, Jan 20, 4:32 PM
This revision was automatically updated to reflect the committed changes.
Falk David (filedescriptor) added a commit: rBd650162ecd47: Curves: Add initial undo system.

Revision Contents
Changeset List

PathSize
release/
scripts/
modules/
bl_keymap_utils/
1 line
6 lines
presets/
keyconfig/
keymap_data/
13 lines
startup/
bl_ui/
23 lines
source/
blender/
editors/
curves/
2 lines
intern/
598 lines
159 lines
include/
3 lines
sculpt_paint/
581 lines
space_api/
1 line
space_view3d/
3 lines
undo/
1 line
2 lines

Diff 59332

View Options

release/scripts/modules/bl_keymap_utils/keymap_hierarchy.py

Loading...
View Options

release/scripts/modules/rna_manual_reference.py

Loading...
View Options

release/scripts/presets/keyconfig/keymap_data/blender_default.py

Loading...
View Options

release/scripts/startup/bl_ui/space_view3d.py

Loading...
View Options

source/blender/editors/curves/CMakeLists.txt

Loading...
View Options

source/blender/editors/curves/intern/curves_ops.cc

Loading...
View Options

source/blender/editors/curves/intern/curves_undo.cc

Loading...
View Options

source/blender/editors/include/ED_curves.h

Loading...
View Options

source/blender/editors/sculpt_paint/curves_sculpt_ops.cc

Loading...
View Options

source/blender/editors/space_api/spacetypes.c

Loading...
View Options

source/blender/editors/space_view3d/space_view3d.cc

Loading...
View Options

source/blender/editors/undo/CMakeLists.txt

Loading...
View Options

source/blender/editors/undo/undo_system_types.c

Loading...