Fix two signal errors, remove unused break_request signals in profilers

[nathanfranke]

This change fixes two errors I found while running the latest master. Not sure when they came about

---

This signal just doesn't exist. I added it, but it currently is never emitted. Keeping it to be consistent with the other profiler classes

ERROR: In Object of type 'EditorNetworkProfiler': Attempt to connect nonexistent signal 'break_request' to method 'ScriptEditorDebugger._profiler_seeked'.
   at: connect (core/object.cpp:1452)

---

Calling for @rxlecky who authored #27742

(?) Not sure exactly what was happening here. Presumably call_deferred takes a vararg (Vector) but the connect will mess up if the binds argument is another vararg.

Once connect was called, the first value in p_args was Nil, now it correctly holds true/false

ERROR: Error calling method from signal 'play_pressed': 'CanvasItemEditor::_update_override_camera_button': Cannot convert argument 1 from Nil to bool..
   at: emit_signal (core/object.cpp:1228)

[JFonS]

I don't think the fix for NetworkProfiler is correct, "break_request" is never used as a signal in that class so it should not be defined there. Instead we should remove the connect attempt.

I tried looking for the connect code but I could not find it, not sure what is going on here... A full project search for "break_request" shows the 3 usages related to EditorProfiler and nothing else, so I'm quite puzzled by that.

[akien-mga]

@JFonS Your grep-fu failed you ;) It's connected in script_editor_debugger.cpp:

$ rg "break_request"
editor/editor_profiler.cpp
554:                                    emit_signal("break_request");
613:    ADD_SIGNAL(MethodInfo("break_request"));

editor/editor_visual_profiler.cpp
592:                                    //emit_signal("break_request");
682:    ADD_SIGNAL(MethodInfo("break_request"));

editor/script_editor_debugger.cpp
2503:           profiler->connect("break_request", this, "_profiler_seeked");
2511:           visual_profiler->connect("break_request", this, "_profiler_seeked");
2519:           network_profiler->connect("break_request", this, "_profiler_seeked");

The visual profiler also never emits the signal BTW, so it's also buggy.

[JFonS]

Well... turns out pulling from upstream before searching is a wise decision.

I'm not sure why @reduz added the connect call there, but I think it is safe to remove it.

[nathanfranke]

I will push an update in about ten minutes, I may remove visual_profiler->connect since it also seems to be unused (As long as my tests work fine).

[akien-mga]

Thanks!

[rxlecky]

This does not seem correct to me. make_binds creates a Vector<Variant> out of the arguments passed to the function. However, there is no overload of connect function that would take a single parameter of type Vector<Variant>.

[nathanfranke]

I think MessageQueue (backend of call deferred) uses a Vector. Ideally we should fix varargs since they are poorly implemented :| that's something I need to work on in C++

[rxlecky]

MessageQueue::push_call - the overload that Object::call_deferred calls - uses VARIANT_ARG for passing message parameters, not Vector.

[rxlecky]

I dug into this a little and tested your solution and as it turns out, it only silences the error but doesn't solve the issue. By packing the parameters into Vector<Variant>, which doesn't match the parameters of connect, the function call fails and the signal is never connected. However, it fails silently since the overload of push_call that call_deferred uses, suppresses the errors.

[rxlecky]

Eventually, I found the culprit here was Variant to Vector conversion operator. There were changes made to Variant class recently and a small typo made it in (PR #36404). It turns out this conversion function gets used rarely so it slipped the attention and exhibited here as a failed signal call.

I suggest reverting the latter of your changes back to original once #36404 is merged as it should work correctly again.

[nathanfranke]

@rxlecky Would you like to make a commit that reverts my changes to canvas_item_editor_plugin.cpp and fixes any other problems since I have no idea what is happening right now?

[rxlecky]

Also a heads-up to @akien-mga not to cherry-pick this one to 3.2 just yet :)

[rxlecky]

@nathanwfranke oh, it's literally just turning the lines 5416 and 5417 back to what they were before:

editor->call_deferred("connect", "play_pressed", this, "_update_override_camera_button", make_binds(true));
editor->call_deferred("connect", "stop_pressed", this, "_update_override_camera_button", make_binds(false));

I can make the commit if you like, I just thought you'd prefer to do it yourself since it's your PR.

[nathanfranke]

Tried that, there is a conflict with #35864. Still not sure if I should say connect or connect_compat because that was another conflict. I will do the revert just let me know if anything is incorrect

[rxlecky]

The conflict with #35864 is truly strange since it doesn't even touch the same files.

Oh, you are right, I completely forgot about #36368 that got merged soon after your PR. The lines should be as follows to accommodate for the change

editor->call_deferred("connect", "play_pressed", Callable(this, "_update_override_camera_button"), make_binds(true));
editor->call_deferred("connect", "stop_pressed", Callable(this, "_update_override_camera_button"), make_binds(false));

Also, I did not mean to intimidate you in any way, I'm really sorry if that was the case. On the contrary, I'm happy to give a hand or explain if you don't understand something. Just be sure to speak up 🙂

[nathanfranke]

@rxlecky No problem, I put that in the pull request above your comment. Also thanks for the heads up but I figured it out. It's good practice for me anyways.