_get_property_list of parent classes is never called

[Zylann]

Godot version

Godot 4.1

godot-cpp version

4.1

System information

Windows 10 64 bits NVIDIA GeForce GTX 1060

Issue description

I have custom resource A and B, and they both implement _get_property_list. However, it seems A::_get_property_list is never called. This results in a bunch of properties of my extension to be unavailable.
Also, properties of B appear under A, instead of B.

In modules, this method is not virtual, and is still one that is called at "multilevel", that is, all levels of inheritance declaring are supposed to be get called by Godot. In GodotCpp, this is also not a virtual function, it is gathered from inside the GDCLASS macro.
It notably has to be that way, otherwise Godot can't figure out the class each property belongs to.

This problem might also be happening with similar methods _get, _set, _notification maybe.

---

The following is investigation, if you want to jump to conclusion scroll down

---

Checking get_property_list_bind in wrapped.hpp:

godot-cpp/include/godot_cpp/classes/wrapped.hpp

Line 212 in 1009da4

static const GDExtensionPropertyInfo *get_property_list_bind(GDExtensionClassInstancePtr p_instance, uint32_t *r_count) { \

It depend how Godot calls it, so can't easily tell which side is wrong.
If GodotCpp is indeed supposed to return only properties of the class and not its parents, then Godot is wrong, but I wonder how it would query parent classes (maybe it has some data structure to walk parent callbacks?).
If GodotCpp is supposed to return all properties, then it is wrong because it exits too early and does not provide which class each set of properties belong to (it is effectively written to do the former logic).

https://github.com/godotengine/godot/blob/0c2144da908a8223e188d27ed1d31d8248056c78/core/object/object.cpp#L496

In core, Object::get_property_list gathers properties from most-derived to base: script first, then extensions in two steps*: first declared properties from derived to base. Then, it checks the extension's get_property_list and calls it once (supposedly returning all properties from derived to base, but that's where it misses), then core class (from derived to base) via multilevel wrapped up into an internal virtual functions of GDCLASS. Then at the end the script property is added, then metadata.
That's the only use of gdextension.get_property_list.

(*) I'm not sure how exactly declared-properties and _get_property_list-properties are supposed to be ordered correctly with that "two step" execution. The behavior should mimick _get_property_listv: each class goes "class_name header + declared-properties + _get_property_list" packed together, while the code for extensions seems to be doing "each class's declared-properties, and then the instance's _get_property_list". So unless there is special logic in the inspector to re-group things together, order is kinda screwed.

Apart from that, if core is correctly implemented, it would mean GodotCpp has to return properties of ALL extension classes from derived to base, by also including class group separators (it does neither), but it has to stop once it reaches a core class?
Put it differently: whatever _get_property_listv would have returned, just without core classes, and without declared-properties since Godot adds them itself.
That likely also explains why properties are wrongly shown under the parent class group in the inspector, since GodotCpp isn't adding such groups like _get_property_listv does. So they end up below the last extension group that was added with earlier declared-properties.

If documentation existed, that's something that should really be specified in detail.

Steps to reproduce

• Create a custom class A inheriting Resource
• Implement A::_get_property_list and add a property, then implement A::_get and A::_set to make it functional
• Create a custom class B inheriting A
• Implement B::_get_property_list, add another property, then implement B::_get and B::_set to make it functional
• Test to create an instance of B in the Godot inspector: observe only properties of B show up, and A::_get_property_list never executes.

Minimal reproduction project

GDExtensionGetPropertyListNotCalled.zip

• Edit the location of GodotCpp in SConstruct
• Compile with scons target=editor dev_build=yes -j4
• Check project/example.gdextension for your platform, I changed windows.debug.x86_64 to match the right library name, you may edit another if you're on a different platform
• Launch the editor
• In the inspector, create a new CustomResourceB resource
• Observe only B::_get_property_list prints
• Also, observe Property B is displayed under CustomResourceA, which is also wrong.

[Zylann]

Maybe the issue can be fixed by just changing GodotCpp to comply with what Godot expects:

static void get_property_list_gather(m_class *self, ::godot::List<::godot::PropertyInfo> &plist) {
	if (m_class::_get_get_property_list() && m_class::_get_get_property_list() != m_inherits::_get_get_property_list()) {
		/* Class group separator */
		plist.push_back(PropertyInfo(Variant::NIL, get_class_static(), PROPERTY_HINT_NONE, String(), PROPERTY_USAGE_CATEGORY));
		/* Properties */
		self->_get_property_list(&plist);
	}
	m_inherits::get_property_list_gather(self, plist);
}

/* Gets properties declared with `_get_property_list` from the current class, as well as its parent classes, */
/* with class group separators. Core classes are not included (they return null).*/
static const GDExtensionPropertyInfo *get_property_list_bind(GDExtensionClassInstancePtr p_instance, uint32_t *r_count) {
	if (p_instance && m_class::_get_get_property_list()) {
		m_class *cls = reinterpret_cast<m_class *>(p_instance);
		ERR_FAIL_COND_V_MSG(!cls->plist_owned.is_empty() || cls->plist != nullptr || cls->plist_size != 0, nullptr, "Internal error, property list was not freed by engine!");
		get_property_list_gather(cls, cls->plist_owned);
		cls->plist_size = cls->plist_owned.size();
		cls->plist = reinterpret_cast<GDExtensionPropertyInfo *>(memalloc(sizeof(GDExtensionPropertyInfo) * cls->plist_size));
		unsigned int i = 0;
		for (const ::godot::PropertyInfo &E : cls->plist_owned) {
			cls->plist[i].type = static_cast<GDExtensionVariantType>(E.type);
			cls->plist[i].name = E.name._native_ptr();
			cls->plist[i].hint = E.hint;
			cls->plist[i].hint_string = E.hint_string._native_ptr();
			cls->plist[i].class_name = E.class_name._native_ptr();
			cls->plist[i].usage = E.usage;
			++i;
		}
		if (r_count != nullptr)
			*r_count = cls->plist_size;
		return cls->plist;
	}
	return nullptr;
}

That fixes the MRP:

However when I test in my original project, which is significantly more complex:

As I feared, class groups show up multiple times. Godot doesn't do any sort of group packing.

That means the logic in core has to [also?] be fixed, there is no way this can work if class properties are split due to the two-pass logic, or they would have to be packed back together afterwards.

For the record, originally it was a single pass written by Reduz: godotengine/godot@a9aba86
But later, the other piece of code was added by CaptainProton42 godotengine/godot@a9aba86
Which is not correct when more than one class in the hierarchy uses _get_property_list... the PR even indicated it was actually a bad fix.
And then later more stuff was added on top: godotengine/godot@c4770e1

It seems Reduz's original intention was for the extension to also return its declared-properties, not just those returned by _get_property_list. Just the same way _get_property_listv really... and it probably can, it just needs access to ClassDB.

Alternatively, Godot would have to be the one querying each inheritance level, but only belonging to extensions, and somehow the extension has to be able to call the instance's _get_property_list at each level separately.

And it looks like it also can?
https://github.com/godotengine/godot/blob/0c2144da908a8223e188d27ed1d31d8248056c78/core/object/object.cpp#L485C4-L485C4
Reverting my change of GodotCpp and modifying core instead, it seems this would be more effective, makes a lot more sense too:

	// Existing code
	if (_extension) {
		const ObjectGDExtension *current_extension = _extension;
		while (current_extension) {
			p_list->push_back(PropertyInfo(Variant::NIL, current_extension->class_name, PROPERTY_HINT_NONE, String(), PROPERTY_USAGE_CATEGORY));

			ClassDB::get_property_list(current_extension->class_name, p_list, true, this);

			// "Second pass" moved here
			if (current_extension->get_property_list) {
				uint32_t pcount;
				const GDExtensionPropertyInfo *pinfo = current_extension->get_property_list(_extension_instance, &pcount);
				for (uint32_t i = 0; i < pcount; i++) {
					p_list->push_back(PropertyInfo(pinfo[i]));
				}
				if (current_extension->free_property_list) {
					current_extension->free_property_list(_extension_instance, pinfo);
				}
			}

			current_extension = current_extension->parent;
		}
	}
	
	// <Deleted previous "second pass" code>

And it works, now properties become available, where they should be!

However there are still some cases where properties appear in the wrong group, when A implements _get_property_list, B inherits A, but B does not implement _get_property_list. Fixing that is probably going to be on GodotCpp side: maybe the derived class is falling back on its parent's _get_property_list, when it should not. Perhaps it could be fixed separately.

[dsnopek]

Thanks! I was able to reproduce the issue using your MRP (extra thanks for the MRP!) and your PRs fix it in my testing as well. Next I'll do a proper review of the code