Expose ClassDB class_get_api_type() class_get_property_getter() class_get_property_setter() methods

[ZerxZ]

Expose ClassDB::class_get_api_type() ClassDB::class_get_property_getter() ClassDB::class_get_property_setter() for editor-plugins and other languages wrapper generator.
godotengine/godot-proposals#9526

[ZerxZ]

CI Tests Godot Cpp test fails cause.
It is because godot-cpp repository binding_generator.py script replaces extension_api.json, ClassDB with ClassDBSingleton.
But it does not change return_type or arguments_type the enumeration ClassDB::APIType to ClassDBSingleton::APIType.
@dsnopek @AThousandShips

[dsnopek]

Thanks!

I think this change makes sense, and the code looks good.

However, this will need to be squashed into a single commit, per Godot's pull request workflow. See the docs for a way to do that using git rebase: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

I'll take a look at fixing the godot-cpp issue.

[dsnopek]

I just posted PR godotengine/godot-cpp#1445 which will fix godot-cpp with these changes

[ZerxZ]

I just posted PR godotengine/godot-cpp#1445 which will fix godot-cpp with these changes

Thanks @dsnopek @AThousandShips !
I've already rebase the branch in single comment and run in local you PR success build and work.

[ZerxZ]

godot/core/core_bind.cpp

Lines 1565 to 1586 in 048d9d3

	void ClassDB::get_argument_options(const StringName &p_function, int p_idx, List<String> *r_options) const {
	const String pf = p_function;
	bool first_argument_is_class = false;
	if (p_idx == 0) {
	first_argument_is_class = (pf == "get_inheriters_from_class" || pf == "get_parent_class" ||
	pf == "class_exists" || pf == "can_instantiate" || pf == "instantiate" ||
	pf == "class_has_signal" || pf == "class_get_signal" || pf == "class_get_signal_list" ||
	pf == "class_get_property_list" || pf == "class_get_property" || pf == "class_set_property" ||
	pf == "class_has_method" || pf == "class_get_method_list" ||
	pf == "class_get_integer_constant_list" || pf == "class_has_integer_constant" || pf == "class_get_integer_constant" ||
	pf == "class_has_enum" || pf == "class_get_enum_list" || pf == "class_get_enum_constants" || pf == "class_get_integer_constant_enum" ||
	pf == "is_class_enabled" || pf == "is_class_enum_bitfield" || pf == "class_get_api_type" ||
	pf == "class_get_property_getter" || pf == "class_get_property_setter");
	}
	if (first_argument_is_class || pf == "is_parent_class") {
	for (const String &E : get_class_list()) {
	r_options->push_back(E.quote());
	}
	}
	Object::get_argument_options(p_function, p_idx, r_options);
	}

The first_argument_is_class variable in this code is becoming increasingly lengthy and less maintainable. Would it be more suitable to change it to a HashSet<String> field to store these names, and then use the HashSet<String> field to check for existence?

 void ClassDB::get_argument_options(const StringName &p_function, int p_idx, List<String> *r_options) const { 
   const String pf = p_function; 
- 	bool first_argument_is_class = false; 
- 	if (p_idx == 0) { 
- 		first_argument_is_class = (pf == "get_inheriters_from_class" || pf == "get_parent_class" || 
- 				pf == "class_exists" || pf == "can_instantiate" || pf == "instantiate" || 
- 				pf == "class_has_signal" || pf == "class_get_signal" || pf == "class_get_signal_list" || 
- 				pf == "class_get_property_list" || pf == "class_get_property" || pf == "class_set_property" || 
- 				pf == "class_has_method" || pf == "class_get_method_list" || 
- 				pf == "class_get_integer_constant_list" || pf == "class_has_integer_constant" || pf == "class_get_integer_constant" || 
- 				pf == "class_has_enum" || pf == "class_get_enum_list" || pf == "class_get_enum_constants" || pf == "class_get_integer_constant_enum" || 
- 				pf == "is_class_enabled" || pf == "is_class_enum_bitfield" || pf == "class_get_api_type" || 
-				pf == "class_get_property_getter" || pf == "class_get_property_setter"); 
- 	} 
+ 	bool first_argument_is_class = p_idx == 0 && has_first_argument_class_methods(pf);
 	if (first_argument_is_class || pf == "is_parent_class") { 
 		for (const String &E : get_class_list()) { 
 			r_options->push_back(E.quote()); 
 		} 
 	} 
  
 	Object::get_argument_options(p_function, p_idx, r_options); 
 } 

+ HashSet<String> first_argument_class_methods;
+ bool has_first_argument_class_methods(const String p_function) const { 
+	if (!first_argument_class_methods.is_empty()) {
+		return first_argument_class_methods.has(p_function);
+	}
+	auto class_methods = {
+		"get_inheriters_from_class",
+		"get_parent_class",
+		"class_exists",
+		"is_parent_class",
+		"can_instantiate",
+		"instantiate",
+		"class_has_signal",
+		"class_get_signal",
+		"class_get_signal_list",
+		"class_get_property_list",
+		"class_get_property",
+		"class_set_property",
+		"class_has_method",
+		"class_get_method_list",
+		"class_get_integer_constant_list",
+		"class_has_integer_constant",
+		"class_get_integer_constant",
+		"class_has_enum",
+		"class_get_enum_list",
+		"class_get_enum_constants",
+		"class_get_integer_constant_enum",
+		"is_class_enabled",
+		"is_class_enum_bitfield",
+		"class_get_api_type",
+		"class_get_property_getter",
+		"class_get_property_setter"
+	};
+	bool result = false;
+	for (const String &E : class_methods) {
+		first_argument_class_methods.insert(E);
+		if (E == p_function) {
+			result = true;
+		}
+	}
+	return result;
+}

[dsnopek]

The first_argument_is_class variable in this code is becoming increasingly lengthy and less maintainable. Would it be more suitable to change it to a HashSet<String> field to store these names, and then use the HashSet<String> field to check for existence?

I would just leave this as it is for now.

Overall, this PR is looking good to me!

CI is complaining about an extra line in the docs XML:

diff --git a/doc/classes/ClassDB.xml b/doc/classes/ClassDB.xml
index ed670fa..6a58536 100644
--- a/doc/classes/ClassDB.xml
+++ b/doc/classes/ClassDB.xml
@@ -106,7 +106,6 @@
 				Returns the default value of [param property] of [param class] or its ancestor classes.
 			</description>
 		</method>
-
 		<method name="class_get_property_getter" qualifiers="const">
 			<return type="StringName" />
 			<param index="0" name="class" type="StringName" />

However, aside from that, the main thing blocking this is that we need to get godot-cpp PR godotengine/godot-cpp#1445 merged and cherry-picked so that the rest of the tests can pass here.

[ZerxZ]

I would just leave this as it is for now.

You're right. I was just leaving a comment to ask for opinions, and I didn't intend to modify that section of the code. It's not within the scope of this PR.

CI is complaining about an extra line in the docs XML:

I'll fix that section of the code right away. It was my oversight, I didn't notice the extra line breaks.

However, aside from that, the main thing blocking this is that we need to get godot-cpp PR godotengine/godot-cpp#1445 merged and cherry-picked so that the rest of the tests can pass here.

I can only wait for your PR to be merged to pass the tests smoothly. Thanks for taking the time to look at this PR over the past few days. I thought not many people would have the free time to review it. Also, thanks for your hard work, @AThousandShips @dsnopek .

[dsnopek]

Once PR godotengine/godot-cpp#1465 is merged (which cherry-picks the necessary changes to binding_generator.py), then the tests here should finally start passing

[ZerxZ]

Once PR godotengine/godot-cpp#1465 is merged (which cherry-picks the necessary changes to binding_generator.py), then the tests here should finally start passing

Thanks!

[ZerxZ]

cc @AThousandShips @dsnopek

[dsnopek]

The tests are now passing - yay!

This looks good to me, however, it would probably be good to update the PR title and commit message with the right method names.

[Delsin-Yu]

Any chance we can have this in 4.3? As the PR has approved.

[AThousandShips]

We're in feature freeze so I don't expect so, but we'll see what the production team says, I suspect 4.4