Paths

Table of Contentst

Curves: fix edit mode detection
ClosedPublic
Actions

Authored by Kévin Dietrich (kevindietrich) on Mar 14 2022, 11:00 AM.

Details

Reviewers

Jacques Lucke (JacquesLucke)
Hans Goudey (HooglyBoogly)

Commits

rBfc8bcd26c0be: Curves: fix edit mode detection

Summary

This adds missing cases to detect edit mode for Curves objects.
Unlike other object types, Curves do not have specific edit data,
rather we edit the original data directly, and rely on Object.mode.

For this, BKE_object_data_is_in_editmode had to be modified to
take a pointer to the object. This affects two places: the outliner
and the dependency graph. For the former place, the object pointer
is readily available, and we can use it. For the latter, the object
pointer is not available, however since it is used to update edit
mode pointers, and since Curves do not have such data, we can
safely pass null to the function here.

This also fixes the assertion failure that happens when closing a file
in edit mode.

Diff Detail

Repository

rB Blender

Branch

curves_edit_curves (branched from master)

Build Status

Buildable 21017
Build 21017: arc lint + arc unit

Event Timeline

Kévin Dietrich (kevindietrich) requested review of this revision.Mar 14 2022, 11:00 AM

Kévin Dietrich (kevindietrich) created this revision.

Harbormaster completed remote builds in B21017: Diff 49236.Mar 14 2022, 11:00 AM

Jacques Lucke (JacquesLucke) added inline comments.Mar 14 2022, 11:23 AM

source/blender/blenkernel/intern/curves.cc
373	Looks like this should use id deletion code (i.e. call `curves_free_data` possibly indirectly). Duplicating the deletion code here seems wrong.
397	I don't quite understand why all of that is necessary. Why do we need a copy of the `Curves` data? And if we need it, why not use generic ID copy code instead of doing everything manually?
source/blender/makesdna/DNA_curves_types.h
115	Looks like this could be a C++ type defined in e.g. `BKE_curves.hh`.
116	Do we really need this pointer?
122	I don't quite understand why this is necessary if we always edit the original `Curves` data directly, instead of a copy of it. Or is there a reason for why we have to edit a copy of it?

It seems that edit mode only gets a copy of the original data, while the draw code operates on the original. I am not sure if I missed something, or if other modes like sculpt behave differently, but throughout the code base, edit mode is a special case. So if I want to display changes in edit mode (e.g. this point is selected), I have to somehow carry the change over to the original data. Using some EditCurves makes it easier as the pointer is passed to COW blocks. So the draw code can just check if it is present and use it use for filling the draw buffers with the updated data.

source/blender/blenkernel/intern/curves.cc
397	The copy is so that edit data is in the same state as the original data, then for the `Curves` data block, it depends if we just use a `CurvesGeometry`. Overall it could indeed some code reuse.
source/blender/makesdna/DNA_curves_types.h
116	For now this is mostly for convenience. We could just have `CurvesGeometry` which might imply some more changes for initialization. Then, maybe in the future some editing tools might want to also have to the surface object, or other properties.
122	As far as I could tell, we did not edit the original `Curves` data.

Kévin Dietrich (kevindietrich) added a child revision: D14262: Curves edit mode: show dots for points.Mar 14 2022, 12:28 PM

Personally I don't think we should add a separate storage for edit mode data. I think edit mode should just edit the original object data directly, rather than working on a copy of it.
Conceptually, there seems to be no reason to work on a copy. One of the nice simplifications of the new curves data is that we use the same data structure for edit mode, sculpt mode, etc.
If we need a way to detect whether the data is in edit mode, there must be simpler ways to do that, e.g. a flag or using ID.orig_id.

In terms of precedent, grease pencil data doesn't have a separate copy for edit mode.

I agree it's better to not work on a copy. For meshes this adds a lot of code complexity and performance issues, unless there's a good reason I'd try to stay away from that.

Hans Goudey (HooglyBoogly) requested changes to this revision.Mar 28 2022, 8:42 PM

This revision now requires changes to proceed.Mar 28 2022, 8:42 PM

Use a flag to set/detect edit mode. Using id_orig (as suggested
in a comment) was not reliable apparently.

I am not too sure about the logic to unset the flag when going
out of edit mode.

Harbormaster completed remote builds in B21405: Diff 49993.Apr 1 2022, 3:04 PM

Kévin Dietrich (kevindietrich) retitled this revision from Curves: add an EditCurves structure to Curves: use a flag to detect edit mode.Apr 1 2022, 3:04 PM

Kévin Dietrich (kevindietrich) edited the summary of this revision. (Show Details)

Do you know the reason that using Object.mode isn't enough to detect whether the object is in edit mode? It seems a redundant to store a flag on the data too, but maybe there's some complexity I'm missing.

I encountered Error: Unable to execute 'Toggle Edit Mode', error changing modes after switching to sculpt mode -> edit mode -> sculpt mode -> edit mode.

In D14330#389660, @Hans Goudey (HooglyBoogly) wrote:

Do you know the reason that using Object.mode isn't enough to detect whether the object is in edit mode? It seems a redundant to store a flag on the data too, but maybe there's some complexity I'm missing.

I don't really know, it does not seem to be used for any other object type. So maybe there is some secret knowledge.

I encountered Error: Unable to execute 'Toggle Edit Mode', error changing modes after switching to sculpt mode -> edit mode -> sculpt mode -> edit mode.

I'll check on that.

CV_DATA_EDITMODE should be cleared on file load I think, similar to pointers getting clear for other types. And probably also datablock copy, though would have to check what other types do regarding depsgraph copy-on-write.

Do you know the reason that using Object.mode isn't enough to detect whether the object is in edit mode?

I don't really know, it does not seem to be used for any other object type. So maybe there is some secret knowledge.

Maybe there is some secret knowledge, but I wouldn't bet on it. I think we should just rely on Object.mode for now. Will be easy enough to add this flag when we run into actual problems. This patch looks good for the case when the flag is needed.

It would appear that not relying Object.mode helps with instances somewhat, so all objects are considered in edit mode if the linked data is. Which is standard Blender behavior.

Further, the flag is used here in BKE_object_data_is_in_editmode. As far as Curves are concerned, this is only used for outliner draw (to display an edit mode icon next to hair data). Either we make a special case for Curves there, or we would need to transfer the mode to the Curves in a new Curves.mode member so BKE_object_data_is_in_editmode still works for the outliner; then this patch would not really be different in size. Not sure if this has less maintenance overhead than reusing Curves.flag with a specific value.

I see, now we've got some reasoning. Don't see a better way to make BKE_object_data_is_in_editmode work without the flag right now. So the flag is fine, please mention that reasoning in the commit message.

It would appear that not relying Object.mode helps with instances somewhat, so all objects are considered in edit mode if the linked data is. Which is standard Blender behavior.

That's actually not true anymore, as of 0f89bcdbebf5094bd816318774b67c677790ea99 I'm pretty sure.

The change to is_object_data_in_editmode looks pretty simple, since the object is already available. The use in deg_update_copy_on_write_datablock seems trickier, though that check shouldn't be needed for curves.
I don't have a really strong feeling about this, but IMO it's not really about saving lines of code in a few places, but making the whole mental model simpler by avoiding storing redundant information.

Use Object.mode for the edit mode detection. After some
tests this seems to work fine.

This also includes a fix for missing updates when switching
to sculpt mode. Without it, the viewport is not properly
updated and edit data is still drawn until we start a stroke.
This can be observed by applying D14262 and doing the mode
switch. It will be committed separately, but included here
for ease of testing just in case.

Kévin Dietrich (kevindietrich) retitled this revision from Curves: use a flag to detect edit mode to Curves: fix edit mode detection.Apr 5 2022, 6:08 PM

Kévin Dietrich (kevindietrich) edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B21475: Diff 50123.Apr 5 2022, 6:09 PM

Thanks for looking into that! I like that there is a single source of truth now. I do wonder BKE_object_data_is_in_editmode even makes sense now, since one object being in edit mode doesn't mean other users of the same data are also in edit mode anymore. But I don't have a strong opinion either way.

Hans Goudey (HooglyBoogly) added inline comments.Apr 5 2022, 6:22 PM

source/blender/editors/sculpt_paint/curves_sculpt_ops.cc
665–666 ↗	(On Diff #50123)	It seems like it would be better add a redraw for the mode change without reevaluating the modifier stack (which I think `ID_RECALC_COPY_ON_WRITE` does). I don't know which flag to use in that case. Or maybe just the notifier is enough?

Jacques Lucke (JacquesLucke) accepted this revision.Apr 5 2022, 6:38 PM

This revision is now accepted and ready to land.Apr 5 2022, 6:38 PM

Kévin Dietrich (kevindietrich) added inline comments.Apr 5 2022, 6:57 PM

source/blender/editors/sculpt_paint/curves_sculpt_ops.cc
665–666 ↗	(On Diff #50123)	Using just the notifier does not work. We need to carry over the new mode, `ID_RECALC_COPY_ON_WRITE` is the only one I knew that gets that done, maybe there is a different way?

Hans Goudey (HooglyBoogly) added inline comments.Apr 5 2022, 7:09 PM

source/blender/editors/sculpt_paint/curves_sculpt_ops.cc
665–666 ↗	(On Diff #50123)	I'm not aware of any. The comment makes the purpose clear enough for now I think, we can change this later if it becomes a problem.