Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make UndoRedo use Callables #43872

Closed
wants to merge 1 commit into from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 25, 2020

This PR makes UndoRedo use Callables instead of method names and hardcoded argument list. I added add_undo_method_compat and changed all old calls to this. We need to port them later and cleanup needlessly bound methods.

Likely fixes #26483
And likely fixes #36895

Sumbitting as draft, because I'm not yet sure what to do with

if (method_callback) {
method_callback(method_callbck_ud, obj, op.name, VARIANT_ARGS_FROM_ARRAY(op.args));
}

Either I need to figure out how to change it to Callable or extract arguments from callable and push them here. For now this part is commented out, everything else seems to work.

Bugsquad edit: This closes godotengine/godot-proposals#5038.

@Riteo
Copy link
Contributor

Riteo commented Nov 26, 2020

Why does CI fail all jobs except Windows builds and static checks?

@bruvzg
Copy link
Member

bruvzg commented Nov 26, 2020

Why does CI fail all jobs except Windows builds and static checks?

Click "Details" next to the failed job to check build logs.

core/object/undo_redo.cpp:308:24: error: non-const lvalue reference to type 'Variant' cannot bind to a temporary of type 'Variant'
                                c.call(nullptr, 0, Variant(), ce);
                                                   ^~~~~~~~~
./core/variant/callable.h:70:66: note: passing argument to parameter 'r_return_value' here
        void call(const Variant **p_arguments, int p_argcount, Variant &r_return_value, CallError &r_call_error) const;
                                                                        ^
1 error generated.

MSVC always have different rules.

@KoBeWi KoBeWi force-pushed the undo_call_me_maybe branch from 618a0f1 to 8477791 Compare January 12, 2021 23:17
@KoBeWi KoBeWi marked this pull request as ready for review January 12, 2021 23:17
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 12, 2021

Ok, finished this. I had to add CallableCustomBind.get_binds() and with some dynamic casting I managed to resolve the callback issue. It was something with live edit, seems to work now. At least when you change properties in the inspector, but when you move nodes it doesn't seem to synchronize properly, no idea why.

@reduz
Copy link
Member

reduz commented Jul 2, 2021

For the record, I suggest closing this. Using callables in UndoRedo means that contributors will be able to provide function pointers instead of instance/string. This breaks live editing, hence it should not be used.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 2, 2021

This breaks live editing, hence it should not be used.

Live editing uses method pointer, so I just extract it from the callable (including arguments, I added a method for that). So live editing still works (except one small issue, see my previous comment).
Actually no. Live editing just uses mostly property callbacks, that's why it was ok in my tests (except the one case that used a method).

In worst case we could determine if a method is relevant for live editing (most of them aren't) and show a warning/error if it can't be a callable. I mean, it could use a callable, but the method just still needs to be bound and we'd provide its name in addition to the callable (right now with my changes the op.name field is unused).

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 1, 2022

Closing, as there is too many conflicts. Could be revisited after #59564 is merged.

The status is this:
Godot has live scene edit, i.e. when you run the game from the editor, any change you do in the editor (not on remote tree) is synced via debugger to the running scene. This is done via UndoRedo actions - the method name is sent to the running instance and is executed remotely. Method pointer Callables don't provide method names, so they can't be called remotely.
HOWEVER, as I mentioned sometime before, most of the editor changes sent via live edit comes from property changes, which don't use methods. They just send property name and value. Also most editor changes done via methods aren't synced anyway (node renames, animation edits etc). So IMO we could change everything to Callables, but still keep the bindings for methods that need to be synchronized (live edit with named Callables should be possible, because you can get the method name). It's probably a few % of the methods, so it shouldn't be a problem. We'd change all methods to callable_mp and use Callable(object, "method") for methods that matter for live edit. Although reduz was opposed to doing it last time it was mentioned (you need to know whether the method is relevant for live edit or not and it will confuse users etc)
Another option is that we could keep all bindings and use named callables everywhere. This is rather meh IMO.

tl;dr this can be revived in the future, but should be done carefully to avoid breaking live edit™

@akien-mga
Copy link
Member

Superseded by #66070.

@KoBeWi KoBeWi deleted the undo_call_me_maybe branch February 11, 2023 21:21
@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 11, 2023

Not really. The current equivalent of this PR would be making EditorUndoRedoManager use callables, but it has the problems I mentioned in my above comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants