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

It is not possible to send null as the last argument of an UndoRedo method #36895

Closed
Zylann opened this issue Mar 8, 2020 · 6 comments
Closed
Milestone

Comments

@Zylann
Copy link
Contributor

Zylann commented Mar 8, 2020

Godot 3.2

To register a method to be executed when doing or undoing a change, UndoRedo.add_do_method() or UndoRedo.add_undo_method() have to be used.
Those methods were implemented by declaring 5 Variant arguments, all null by default.
Unfortunately, the argument count is then determined by checking how many remaining arguments are null, including cases where you want to call the method with null.
That means it is impossible to write this:

undo_redo.add_do_method(object, "foo", 42, true, null)

Because the null at the end will be confused as absence of argument. Then when committing the action, undoing or redoing it, it won't work and this kind of error will be printed:

ERROR: UndoRedo::_process_operation_list: Error calling method from signal 'foo': Method expected 3 arguments, but called with 2.

See

for (int i = 0; i < VARIANT_ARG_MAX; i++) {

I had to workaround this limitation by binding extra methods in my objects, and check if variants are null before using add_do_method and add_undo_method.

To fix this, UndoRedo should not determine argument count in this way. Instead, it should use reflection to query how many arguments the called method actually has. Other solutions include sending an array explicitely, adding a count argument, or having a Variant::UNDEFINED type, but that would break compatibility.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 10, 2020

Related to #29204

@kopcion
Copy link

kopcion commented Apr 19, 2020

I've been working on this bug. For now, I only managed to solve this issue by having map in UndoRedo class with argc for current functions, but I would like to do some more, so feedback would be great. I don't see how to apply reflection to this problem, nor what exactly do you mean by reflection in c++ (until recently it wasn't included afaik).

Here is basic project reproducing the bug.
Issue#36895.zip

kopcion added a commit to kopcion/godot that referenced this issue May 10, 2020
kopcion added a commit to kopcion/godot that referenced this issue May 10, 2020
kopcion added a commit to kopcion/godot that referenced this issue May 30, 2020
	Added map with <function, argc> pairing used to determine
	the proper argc.
@Jummit
Copy link
Contributor

Jummit commented Oct 27, 2020

Minimal reproduction project I made for #43141:

undo_test.zip

Note that Object.call handles this correctly, so maybe one could look how it does it.

@me2beats
Copy link

me2beats commented Jan 4, 2022

just faced this too, so I do it in my own functions like

undo.add_do_method(self, "_do")

@akien-mga
Copy link
Member

akien-mga commented Feb 11, 2023

Fixed by #66070.

Edit: Not sure actually, based on #43872 (comment).

@akien-mga akien-mga added this to the 4.0 milestone Feb 11, 2023
@akien-mga akien-mga reopened this Feb 11, 2023
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 11, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Feb 11, 2023

This was actually resolved by #58929

@KoBeWi KoBeWi closed this as completed Feb 11, 2023
@KoBeWi KoBeWi modified the milestones: 4.1, 4.0 Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants