Differential D16979 Diff 59724 source/blender/editors/curves/intern/curves_undo.cc

Changeset View

Standalone View

View Options

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

This file was added.

/* SPDX-License-Identifier: GPL-2.0-or-later */

/** \file

* \ingroup edcurves

#include "BKE_context.h"

#include "BKE_curves.hh"

#include "BKE_main.h"

#include "BKE_object.h"

#include "BKE_undo_system.h"

#include "CLG_log.h"

#include "DEG_depsgraph.h"

#include "ED_curves.h"

#include "ED_undo.h"

#include "MEM_guardedalloc.h"

#include "WM_api.h"

#include "WM_types.h"

static CLG_LogRef LOG = {"ed.undo.curves"};

namespace blender::ed::curves::undo {

/* -------------------------------------------------------------------- */

/** \name Implements ED Undo System

* \note This is similar for all edit-mode types.

HooglyBooglyUnsubmitted

Done

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

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

* \{ */

struct StepObject {

UndoRefID_Object obedit_ref = {};

bke::CurvesGeometry geometry = {};

};

struct CurvesUndoStep {

UndoStep step;

Array<StepObject> objects;

};

static bool step_encode(bContext *C, Main *bmain, UndoStep *us_p)

{

CurvesUndoStep *us = reinterpret_cast<CurvesUndoStep *>(us_p);

const Scene *scene = CTX_data_scene(C);

ViewLayer *view_layer = CTX_data_view_layer(C);

HooglyBooglyUnsubmitted

Done

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.

HooglyBoogly: Since these structs are in the `ed::curves` namespace, theoretically their names could be…

uint objects_num = 0;

Object **objects = ED_undo_editmode_objects_from_view_layer(scene, view_layer, &objects_num);

new (&us->objects) Array<StepObject>(objects_num);

threading::parallel_for(us->objects.index_range(), 8, [&](const IndexRange range) {

for (const int i : range) {

filedescriptorAuthorUnsubmitted

Done

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.

filedescriptor: Not sure I agree with how this is written right now. It feels like it's "forcing" the RAII…

HooglyBooglyUnsubmitted

Not Done

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

HooglyBoogly: Reverting that part is fine with me. It is a bit complex. The part I like more is storing `bke…

Object *ob = objects[i];

const Curves &curves_id = *static_cast<Curves *>(ob->data);

StepObject &object = us->objects[i];

object.obedit_ref.ptr = ob;

object.geometry = bke::CurvesGeometry::wrap(curves_id.geometry);

}

});

HooglyBooglyUnsubmitted

Done

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

HooglyBoogly: There is a lot of redundancy in these checks right now. For example, a null active object and…

MEM_SAFE_FREE(objects);

bmain->is_memfile_undo_flush_needed = true;

HooglyBooglyUnsubmitted

Done

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

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

filedescriptorAuthorUnsubmitted

Done

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

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

HooglyBooglyUnsubmitted

Done

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

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

return true;

}

HooglyBooglyUnsubmitted

Done

Object **objects = ED_undo_editmode_objects_from_view_layer(scene, view_layer, &objects_len);

- us->elems = static_cast<CurvesUndoStep_Elem *>(

- MEM_callocN(sizeof(*us->elems) * objects_len, __func__));

+ us->elems = MEM_cnew_array<CurvesUndoStep_Elem>(objects_len);

us->elems_len = objects_len;

HooglyBoogly:

static void step_decode(

bContext *C, Main *bmain, UndoStep *us_p, const eUndoStepDir /*dir*/, bool /*is_final*/)

{

CurvesUndoStep *us = reinterpret_cast<CurvesUndoStep *>(us_p);

ED_undo_object_editmode_restore_helper(C,

&us->objects.first().obedit_ref.ptr,

us->objects.size(),

sizeof(decltype(us->objects)::value_type));

BLI_assert(BKE_object_is_in_editmode(us->objects.first().obedit_ref.ptr));

for (const StepObject &object : us->objects) {

Curves &curves_id = *static_cast<Curves *>(object.obedit_ref.ptr->data);

/* Overwrite the curves geometry. */

bke::CurvesGeometry::wrap(curves_id.geometry) = object.geometry;

DEG_id_tag_update(&curves_id.id, ID_RECALC_GEOMETRY);

}

ED_undo_object_set_active_or_warn(CTX_data_scene(C),

HooglyBooglyUnsubmitted

Done

static void curves_undosys_step_decode(

- bContext *C, Main *bmain, UndoStep *us_p, const eUndoStepDir /* dir */, bool /* is_final */)

+ bContext *C, Main *bmain, UndoStep *us_p, const eUndoStepDir /*dir*/, bool /*is_final*/)

{

CurvesUndoStep *us = (CurvesUndoStep *)us_p;

Nitpicky thing

HooglyBoogly: Nitpicky thing

CTX_data_view_layer(C),

us->objects.first().obedit_ref.ptr,

us_p->name,

&LOG);

bmain->is_memfile_undo_flush_needed = true;

WM_event_add_notifier(C, NC_GEOM | ND_DATA, nullptr);

}

static void step_free(UndoStep *us_p)

{

CurvesUndoStep *us = reinterpret_cast<CurvesUndoStep *>(us_p);

us->objects.~Array();

}

static void foreach_ID_ref(UndoStep *us_p,

UndoTypeForEachIDRefFn foreach_ID_ref_fn,

void *user_data)

{

CurvesUndoStep *us = reinterpret_cast<CurvesUndoStep *>(us_p);

for (const StepObject &object : us->objects) {

foreach_ID_ref_fn(user_data, ((UndoRefID *)&object.obedit_ref));

}

/** \} */

} // namespace blender::ed::curves::undo

HooglyBooglyUnsubmitted

Done

NULL -> nullptr

HooglyBoogly: `NULL` -> `nullptr`

void ED_curves_undosys_type(UndoType *ut)

{

using namespace blender::ed;

ut->name = "Edit Curves";

ut->poll = curves::editable_curves_in_edit_mode_poll;

ut->step_encode = curves::undo::step_encode;

ut->step_decode = curves::undo::step_decode;

ut->step_free = curves::undo::step_free;

ut->step_foreach_ID_ref = curves::undo::foreach_ID_ref;

ut->flags = UNDOTYPE_FLAG_NEED_CONTEXT_FOR_ENCODE;

HooglyBooglyUnsubmitted

Not Done

Is this kept for a specific reason? just curious

HooglyBoogly: Is this kept for a specific reason? just curious

ut->step_size = sizeof(curves::undo::CurvesUndoStep);

}