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.

Sign up for GitHub

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

Fixes to allow object-less callables throughout Godot #82695

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Fixes to allow object-less callables throughout Godot #82695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/core_bind.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,8 +1211,7 @@ void Thread::_start_func(void *ud) {
Ref<Thread> t = *tud;
memdelete(tud);

Object *target_instance = t->target_callable.get_object();
if (!target_instance) {
if (!t->target_callable.is_valid()) {
t->running.clear();
ERR_FAIL_MSG(vformat("Could not call function '%s' on previously freed instance to start thread %s.", t->target_callable.get_method(), t->get_id()));
}
Expand Down
33 changes: 23 additions & 10 deletions core/object/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,8 +1109,7 @@ Error Object::emit_signalp(const StringName &p_name, const Variant **p_args, int
Error err = OK;

for (const Connection &c : slot_conns) {
Object *target = c.callable.get_object();
if (!target) {
if (!c.callable.is_valid()) {
// Target might have been deleted during signal callback, this is expected and OK.
continue;
}
Expand All @@ -1133,7 +1132,8 @@ Error Object::emit_signalp(const StringName &p_name, const Variant **p_args, int
continue;
}
#endif
if (ce.error == Callable::CallError::CALL_ERROR_INVALID_METHOD && !ClassDB::class_exists(target->get_class_name())) {
Object *target = c.callable.get_object();
if (ce.error == Callable::CallError::CALL_ERROR_INVALID_METHOD && target && !ClassDB::class_exists(target->get_class_name())) {
//most likely object is not initialized yet, do not throw error.
} else {
ERR_PRINT("Error calling from signal '" + String(p_name) + "' to callable: " + Variant::get_callable_error_text(c.callable, args, argc, ce) + ".");
Expand Down Expand Up @@ -1313,8 +1313,14 @@ void Object::get_signals_connected_to_this(List<Connection> *p_connections) cons
Error Object::connect(const StringName &p_signal, const Callable &p_callable, uint32_t p_flags) {
ERR_FAIL_COND_V_MSG(p_callable.is_null(), ERR_INVALID_PARAMETER, "Cannot connect to '" + p_signal + "': the provided callable is null.");

Object *target_object = p_callable.get_object();
ERR_FAIL_NULL_V_MSG(target_object, ERR_INVALID_PARAMETER, "Cannot connect to '" + p_signal + "' to callable '" + p_callable + "': the callable object is null.");
if (p_callable.is_standard()) {
// FIXME: This branch should probably removed in favor of the `is_valid()` branch, but there exist some classes
// that call `connect()` before they are fully registered with ClassDB. Until all such classes can be found
// and registered soon enough this branch is needed to allow `connect()` to succeed.
ERR_FAIL_NULL_V_MSG(p_callable.get_object(), ERR_INVALID_PARAMETER, "Cannot connect to '" + p_signal + "' to callable '" + p_callable + "': the callable object is null.");
} else {
ERR_FAIL_COND_V_MSG(!p_callable.is_valid(), ERR_INVALID_PARAMETER, "Cannot connect to '" + p_signal + "': the provided callable is not valid: " + p_callable);
}

SignalData *s = signal_map.getptr(p_signal);
if (!s) {
Expand Down Expand Up @@ -1352,14 +1358,18 @@ Error Object::connect(const StringName &p_signal, const Callable &p_callable, ui
}
}

Object *target_object = p_callable.get_object();

SignalData::Slot slot;

Connection conn;
conn.callable = target;
conn.signal = ::Signal(this, p_signal);
conn.flags = p_flags;
slot.conn = conn;
slot.cE = target_object->connections.push_back(conn);
if (target_object) {
dsnopek marked this conversation as resolved.
Show resolved Hide resolved
slot.cE = target_object->connections.push_back(conn);
}
if (p_flags & CONNECT_REFERENCE_COUNTED) {
slot.reference_count = 1;
}
Expand Down Expand Up @@ -1398,9 +1408,6 @@ void Object::disconnect(const StringName &p_signal, const Callable &p_callable)
bool Object::_disconnect(const StringName &p_signal, const Callable &p_callable, bool p_force) {
ERR_FAIL_COND_V_MSG(p_callable.is_null(), false, "Cannot disconnect from '" + p_signal + "': the provided callable is null.");
dsnopek marked this conversation as resolved.
Show resolved Hide resolved

Object *target_object = p_callable.get_object();
ERR_FAIL_NULL_V_MSG(target_object, false, "Cannot disconnect '" + p_signal + "' from callable '" + p_callable + "': the callable object is null.");

SignalData *s = signal_map.getptr(p_signal);
if (!s) {
bool signal_is_valid = ClassDB::has_signal(get_class_name(), p_signal) ||
Expand All @@ -1420,7 +1427,13 @@ bool Object::_disconnect(const StringName &p_signal, const Callable &p_callable,
}
}

target_object->connections.erase(slot->cE);
if (slot->cE) {
Object *target_object = p_callable.get_object();
if (target_object) {
target_object->connections.erase(slot->cE);
}
}

s->slot_map.erase(*p_callable.get_base_comparator());

if (s->slot_map.is_empty() && ClassDB::has_signal(get_class_name(), p_signal)) {
Expand Down
22 changes: 16 additions & 6 deletions core/object/undo_redo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,17 +136,22 @@ void UndoRedo::add_do_method(const Callable &p_callable) {
ERR_FAIL_COND(action_level <= 0);
ERR_FAIL_COND((current_action + 1) >= actions.size());

Object *object = p_callable.get_object();
ERR_FAIL_NULL(object);
ObjectID object_id = p_callable.get_object_id();
Object *object = ObjectDB::get_instance(object_id);
ERR_FAIL_COND(object_id.is_valid() && object == nullptr);

Operation do_op;
do_op.callable = p_callable;
do_op.object = p_callable.get_object_id();
do_op.object = object_id;
if (Object::cast_to<RefCounted>(object)) {
do_op.ref = Ref<RefCounted>(Object::cast_to<RefCounted>(object));
}
do_op.type = Operation::TYPE_METHOD;
do_op.name = p_callable.get_method();
if (do_op.name == StringName()) {
// There's no `get_method()` for custom callables, so use `operator String()` instead.
do_op.name = static_cast<String>(p_callable);
}

actions.write[current_action + 1].do_ops.push_back(do_op);
}
Expand All @@ -161,18 +166,23 @@ void UndoRedo::add_undo_method(const Callable &p_callable) {
return;
}

Object *object = p_callable.get_object();
ERR_FAIL_NULL(object);
ObjectID object_id = p_callable.get_object_id();
Object *object = ObjectDB::get_instance(object_id);
ERR_FAIL_COND(object_id.is_valid() && object == nullptr);

Operation undo_op;
undo_op.callable = p_callable;
undo_op.object = p_callable.get_object_id();
undo_op.object = object_id;
if (Object::cast_to<RefCounted>(object)) {
undo_op.ref = Ref<RefCounted>(Object::cast_to<RefCounted>(object));
}
undo_op.type = Operation::TYPE_METHOD;
undo_op.force_keep_in_merge_ends = force_keep_in_merge_ends;
undo_op.name = p_callable.get_method();
if (undo_op.name == StringName()) {
// There's no `get_method()` for custom callables, so use `operator String()` instead.
undo_op.name = static_cast<String>(p_callable);
}

actions.write[current_action + 1].undo_ops.push_back(undo_op);
}
Expand Down
7 changes: 7 additions & 0 deletions core/variant/callable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ void Callable::callp(const Variant **p_arguments, int p_argcount, Variant &r_ret
r_call_error.expected = 0;
r_return_value = Variant();
} else if (is_custom()) {
if (!is_valid()) {
r_call_error.error = CallError::CALL_ERROR_INSTANCE_IS_NULL;
r_call_error.argument = 0;
r_call_error.expected = 0;
r_return_value = Variant();
return;
}
custom->call(p_arguments, p_argcount, r_return_value, r_call_error);
} else {
Object *obj = ObjectDB::get_instance(ObjectID(object));
Expand Down
4 changes: 2 additions & 2 deletions scene/animation/tween.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,7 @@ bool CallbackTweener::step(double &r_delta) {
return false;
}

if (!callback.get_object()) {
if (!callback.is_valid()) {
dsnopek marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

Expand Down Expand Up @@ -740,7 +740,7 @@ bool MethodTweener::step(double &r_delta) {
return false;
}

if (!callback.get_object()) {
if (!callback.is_valid()) {
return false;
}

Expand Down
14 changes: 0 additions & 14 deletions servers/physics_2d/godot_area_2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,6 @@ void GodotArea2D::set_space(GodotSpace2D *p_space) {
}

void GodotArea2D::set_monitor_callback(const Callable &p_callback) {
ObjectID id = p_callback.get_object_id();

if (id == monitor_callback.get_object_id()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like some optimization and should work without an object too (though the condition probably needs changing). Why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For object-less callables this will always fall into the optimized path because get_object_id() will always return ObjectID(0), so for those callables its no good, and why I removed it. Im not sure what this condition could be changed to.

I tried to see what this optimization and the code that follows was about, but I'm not so familiar with the physics servers inner workings. This code goes all they way back to the original opensource commit, so no explanation there. When I tried to follow all the paths that could lead here it seemed that the optimized path doesn't ever get used in the current code, tho I didn't test it.

In short, I don't know why that code is there in the first place, how to properly change it to support object-less callables (tho maybe just adding p_callback.is_standard() && to the condition would be enough?), and it doesn't appear that removing it would harm anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like id.is_valid() && id == monitor_callback.get_object_id()?
This code allows to swap callbacks without clearing shapes if the object is the same. So the condition I suggested would make it skipped if the new callback has no object.

That said, I'm also not very familiar with physics code either. This "optimization" doesn't look very important; normally the callbacks are changed only when toggling area's monitoring property, which is rather uncommon thing to do.

monitor_callback = p_callback;
return;
}

_unregister_shapes();

monitor_callback = p_callback;
Expand All @@ -100,13 +93,6 @@ void GodotArea2D::set_monitor_callback(const Callable &p_callback) {
}

void GodotArea2D::set_area_monitor_callback(const Callable &p_callback) {
ObjectID id = p_callback.get_object_id();

if (id == area_monitor_callback.get_object_id()) {
area_monitor_callback = p_callback;
return;
}

_unregister_shapes();

area_monitor_callback = p_callback;
Expand Down
8 changes: 4 additions & 4 deletions servers/physics_2d/godot_body_2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ void GodotBody2D::integrate_velocities(real_t p_step) {
return;
}

if (fi_callback_data || body_state_callback.get_object()) {
if (fi_callback_data || body_state_callback.is_valid()) {
get_space()->body_add_to_state_query_list(&direct_state_query_list);
}

Expand Down Expand Up @@ -676,7 +676,7 @@ void GodotBody2D::call_queries() {
Variant direct_state_variant = get_direct_state();

if (fi_callback_data) {
if (!fi_callback_data->callable.get_object()) {
if (!fi_callback_data->callable.is_valid()) {
set_force_integration_callback(Callable());
} else {
const Variant *vp[2] = { &direct_state_variant, &fi_callback_data->udata };
Expand All @@ -692,7 +692,7 @@ void GodotBody2D::call_queries() {
}
}

if (body_state_callback.get_object()) {
if (body_state_callback.is_valid()) {
body_state_callback.call(direct_state_variant);
}
}
Expand All @@ -719,7 +719,7 @@ void GodotBody2D::set_state_sync_callback(const Callable &p_callable) {
}

void GodotBody2D::set_force_integration_callback(const Callable &p_callable, const Variant &p_udata) {
if (p_callable.get_object()) {
if (p_callable.is_valid()) {
if (!fi_callback_data) {
fi_callback_data = memnew(ForceIntegrationCallbackData);
}
Expand Down
12 changes: 0 additions & 12 deletions servers/physics_3d/godot_area_3d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,6 @@ void GodotArea3D::set_space(GodotSpace3D *p_space) {
}

void GodotArea3D::set_monitor_callback(const Callable &p_callback) {
ObjectID id = p_callback.get_object_id();
if (id == monitor_callback.get_object_id()) {
monitor_callback = p_callback;
return;
}

_unregister_shapes();

monitor_callback = p_callback;
Expand All @@ -108,12 +102,6 @@ void GodotArea3D::set_monitor_callback(const Callable &p_callback) {
}

void GodotArea3D::set_area_monitor_callback(const Callable &p_callback) {
ObjectID id = p_callback.get_object_id();
if (id == area_monitor_callback.get_object_id()) {
area_monitor_callback = p_callback;
return;
}

_unregister_shapes();

area_monitor_callback = p_callback;
Expand Down
8 changes: 4 additions & 4 deletions servers/physics_3d/godot_body_3d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ void GodotBody3D::integrate_velocities(real_t p_step) {
return;
}

if (fi_callback_data || body_state_callback.get_object()) {
if (fi_callback_data || body_state_callback.is_valid()) {
get_space()->body_add_to_state_query_list(&direct_state_query_list);
}

Expand Down Expand Up @@ -759,7 +759,7 @@ void GodotBody3D::call_queries() {
Variant direct_state_variant = get_direct_state();

if (fi_callback_data) {
if (!fi_callback_data->callable.get_object()) {
if (!fi_callback_data->callable.is_valid()) {
set_force_integration_callback(Callable());
} else {
const Variant *vp[2] = { &direct_state_variant, &fi_callback_data->udata };
Expand All @@ -771,7 +771,7 @@ void GodotBody3D::call_queries() {
}
}

if (body_state_callback.get_object()) {
if (body_state_callback.is_valid()) {
body_state_callback.call(direct_state_variant);
}
}
Expand All @@ -798,7 +798,7 @@ void GodotBody3D::set_state_sync_callback(const Callable &p_callable) {
}

void GodotBody3D::set_force_integration_callback(const Callable &p_callable, const Variant &p_udata) {
if (p_callable.get_object()) {
if (p_callable.is_valid()) {
if (!fi_callback_data) {
fi_callback_data = memnew(ForceIntegrationCallbackData);
}
Expand Down