Remind developers about memnew() in crash message when missing binding callbacks
#1510
Conversation
dsnopek
added
the
enhancement
| @@ -84,8 +84,7 @@ Wrapped::Wrapped(const StringName p_godot_class) { | |||
| godot::internal::gdextension_interface_object_set_instance_binding(_owner, godot::internal::token, this, _constructing_class_binding_callbacks); | |||
| _constructing_class_binding_callbacks = nullptr; | |||
| } else { | |||
| ERR_PRINT("BUG: create a Godot Object without binding callbacks."); | |||
| CRASH_NOW_MSG("BUG: create a Godot Object without binding callbacks."); | |||
| CRASH_NOW_MSG("BUG: Godot Object created without binding callbacks. Did you forget to use memnew()?"); | |||
There was a problem hiding this comment.
Could we include the derived class type that ultimately caused this error?
For example, given the following use case:
class MyObject : public RefCounted {
GDCLASS(MyObject, RefCounted);
...
};
class OtherObject : public Control {
GDCLASS(OtherObject, Control);
MyObject _object;
};This would trigger this crash because of the instance variable of MyObject in OtherObject. If others face this issue, it would be helpful that this error message would indicate that the failure was related to the construction of MyObject. The class type argument to this Wrapped ctor is only RefCounted IIRC, which is quite meaningless :(
There was a problem hiding this comment.
It would be really nice to put the name of the derived class in there! Unfortunately, I don't know a way to do it. :-/
Calling get_class_static() here will always give us "Wrapped". We could add a virtual _get_class_namev() method like we have in Godot itself, but I don't think it'll work in the Wrapped() constructor because the virtual table won't have been setup yet, and we'd just get "Wrapped" again.
If someone can figure out how to do this, though, I'd happily include it!
There was a problem hiding this comment.
Would _constructing_extension_class_name not always refer to this class? When I was fiddling with debugging, I printed this out in the if-block before before it was reset to null, and I found it was quite helpful to identify the offender, but maybe I was just simply lucky?
There was a problem hiding this comment.
Only if memnew() is used - it's memnew() that calls _pre_initialize() which calls Wrapped::_set_construct_info() in order to set that.
If memnew() is not used, then _constructing_extension_class_name will either be nullptr or wrong. And, unfortunately, it's the case where memnew() is not used that we want to show this message :-)
New developers not knowing the need to use
memnew()instead ofnewis a common cause for support requests.After PR #1446, Godot will crash if a developer forgot to use
memnew().This PR attempts to improve that message by mentioning
memnew(), in order to help folks learn about it, and hopefully reduce the number of support requests related to it. :-)This also removes the
ERR_PRINT()that duplicates the message - I'm not sure why that was there, but I don't think it should be necessary.