Skip to content

Add ClassDB::has_instance_binding_callbacks() and check before getting callbacks in internal::get_object_instance_binding()#1223

Closed
Daylily-Zeleen wants to merge 1 commit intogodotengine:masterfrom
Daylily-Zeleen:daylily-zeleen/has_instance_binding_callbacks
Closed

Add ClassDB::has_instance_binding_callbacks() and check before getting callbacks in internal::get_object_instance_binding()#1223
Daylily-Zeleen wants to merge 1 commit intogodotengine:masterfrom
Daylily-Zeleen:daylily-zeleen/has_instance_binding_callbacks

Conversation

@Daylily-Zeleen
Copy link
Contributor

@Daylily-Zeleen Daylily-Zeleen commented Aug 28, 2023

In my case, I need to get all singletons from Engine::get_singleton()->get_singleton(), but if it has not instance callbacks, it will print a error and return a invalid object.
I add ClassDB::has_instance_binding_callbacks() to check if it is available or not.

I think this is beneficial to do some reflection operations like in C#, so I open this pr.

@Daylily-Zeleen Daylily-Zeleen requested a review from a team as a code owner August 28, 2023 06:16
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/has_instance_binding_callbacks branch from a8e18ee to 6a7b970 Compare August 28, 2023 06:19
@dsnopek
Copy link
Collaborator

dsnopek commented Sep 7, 2023

Thanks!

Can you give some more details about the behavior you're getting from Engine::get_singleton()->get_singleton()? Which singletons have this problem and why don't they have instance callbacks? Are the instance callbacks unavailable because you're trying to get it too early, before the class has been registered?

but if it has not instance callbacks, it will print a error and return a invalid object

In what way is it invalid?

I'd really love for Engine::get_singleton()->get_singleton() to always return either nullptr or a valid object, so if there's conditions where it isn't, I think that should probably be fixed, rather than adding ClassDB::has_instance_binding_callbacks().

@Daylily-Zeleen
Copy link
Contributor Author

Daylily-Zeleen commented Sep 8, 2023

Oh, my mistake, it will print an error but return a valid Object *.
Only occur when get ClassDB and GDScriptLanguageProtocol, these two classes are not generated binding callbacks currently.
Here is my test code:

void initialize_example_module(ModuleInitializationLevel p_level) {
	if (p_level != MODULE_INITIALIZATION_LEVEL_SCENE) {
		return;
	}

	PackedStringArray singleton_names = Engine::get_singleton()->get_singleton_list();
	for (const String &singleton_name : singleton_names) {
		Object *singleton = Engine::get_singleton()->get_singleton(singleton_name);
		bool valid = singleton != nullptr;
		bool has_instance_binding_callbacks = ClassDB::has_instance_binding_callbacks(singleton_name);
		if (!valid || !has_instance_binding_callbacks) {
			UtilityFunctions::print_verbose(vformat("Singleton \"%s\" is invalid? %s, has instance binding? %s.", singleton_name, valid, has_instance_binding_callbacks));
		} else {
			UtilityFunctions::print(vformat("Get singleton \"%s\" is success.", singleton_name));
		}
	}
}

In src/core/object.cpp, line 53.
Maybe we should do some check here before Class::get_instance_binding_callbacks()(at line 54) to avoid that print a error message in Class::get_instance_binding_callbacks(), the added method Class::has_instance_binding_callbacks() still useful.

…t_instance_binding_callables() in internal::get_object_instance_binding()
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/has_instance_binding_callbacks branch from 6a7b970 to ba59a2a Compare September 8, 2023 08:53
@Daylily-Zeleen Daylily-Zeleen changed the title Add ClassDB::has_instance_binding_callbacks() Add ClassDB::has_instance_binding_callbacks() and check before getting callbacks in internal::get_object_instance_binding() Sep 8, 2023
@Daylily-Zeleen
Copy link
Contributor Author

@dsnopek By the way, if I want to be a author in "AUTHORS.md", should I have be merged at least 11 commits only in godot repo? Or my commits in this repo will be counted, too?

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 10, 2023

By the way, if I want to be a author in "AUTHORS.md", should I have be merged at least 11 commits only in godot repo? Or my commits in this repo will be counted, too?

I just asked internally and only commits to the main Godot repo are counted. However, you've made some really great GDExtension contributions, both to Godot and godot-cpp - Thank You So Much! I think you'll get there eventually :-)

@Daylily-Zeleen
Copy link
Contributor Author

Thanks for your reply!

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 10, 2023

Oh, my mistake, it will print an error but return a valid Object *.
Only occur when get ClassDB and GDScriptLanguageProtocol, these two classes are not generated binding callbacks currently.

Ah, ok, thanks for the clarification!

Maybe we should do some check here before Class::get_instance_binding_callbacks()(at line 54) to avoid that print a error message in Class::get_instance_binding_callbacks()

Hm, I'm not sure that's what we want to do. There may be classes added to Godot after the creation of the GDExtension, which won't be listed in the instance bindings. In that case, we probably do want an Object * (or whatever is the closest parent class that we do have bindings for) so that the object can still be used somewhat.

See #1165

@Daylily-Zeleen
Copy link
Contributor Author

Issue is fixed by #1165.

@Daylily-Zeleen Daylily-Zeleen deleted the daylily-zeleen/has_instance_binding_callbacks branch October 12, 2023 16:22
@akien-mga akien-mga added the bug This has been identified as a bug label Oct 12, 2023
@akien-mga akien-mga added this to the 4.2 milestone Oct 12, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

archived bug This has been identified as a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants