Skip to content

Commit baf0b9e

Browse files
ZylannZylann
committed
Don't call parent _get_property_list when a class doesn't define it.
Godot is already supposed to call _get_property_list of parent classes, so this binding function must really only return procedural properties of the class it belongs to, and not parent or child classes.
1 parent 749b0b9 commit baf0b9e

File tree

3 files changed

+58
-30
lines changed

3 files changed

+58
-30
lines changed

‎include/godot_cpp/classes/wrapped.hpp

Lines changed: 28 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,9 @@ class Wrapped {
7171
static GDExtensionBool property_get_revert_bind(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionVariantPtr r_ret) { return false; }
7272
static void to_string_bind(GDExtensionClassInstancePtr p_instance, GDExtensionBool *r_is_valid, GDExtensionStringPtr r_out) {}
7373

74+
// The only reason this has to be held here, is when we return results of `_get_property_list` to Godot, we pass
75+
// pointers to strings in this list. They have to remain valid to pass the bridge, until the list is freed by Godot...
7476
::godot::List<::godot::PropertyInfo> plist_owned;
75-
GDExtensionPropertyInfo *plist = nullptr;
76-
uint32_t plist_size = 0;
7777

7878
void _postinitialize();
7979

@@ -95,9 +95,19 @@ class Wrapped {
9595
GodotObject *_owner = nullptr;
9696
};
9797

98+
namespace internal {
99+
100+
GDExtensionPropertyInfo *create_c_property_list(const ::godot::List<::godot::PropertyInfo> &plist_cpp, uint32_t *r_size);
101+
void free_c_property_list(GDExtensionPropertyInfo *plist);
102+
103+
} // namespace internal
104+
98105
} // namespace godot
99106

100-
#define GDCLASS(m_class, m_inherits) \
107+
// Use this on top of your own classes.
108+
// Note: the trail of `***` is to keep sane diffs in PRs, because clang-format otherwise moves every `\` which makes
109+
// every line of the macro different
110+
#define GDCLASS(m_class, m_inherits) /***********************************************************************************************************************************************/ \
101111
private: \
102112
void operator=(const m_class &p_rval) {} \
103113
friend class ::godot::ClassDB; \
@@ -209,40 +219,29 @@ public:
209219
return false; \
210220
} \
211221
\
222+
static inline bool has_get_property_list() { \
223+
return m_class::_get_get_property_list() && m_class::_get_get_property_list() != m_inherits::_get_get_property_list(); \
224+
} \
225+
\
212226
static const GDExtensionPropertyInfo *get_property_list_bind(GDExtensionClassInstancePtr p_instance, uint32_t *r_count) { \
213-
if (p_instance && m_class::_get_get_property_list()) { \
214-
if (m_class::_get_get_property_list() != m_inherits::_get_get_property_list()) { \
215-
m_class *cls = reinterpret_cast<m_class *>(p_instance); \
216-
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!"); \
217-
cls->_get_property_list(&cls->plist_owned); \
218-
cls->plist = reinterpret_cast<GDExtensionPropertyInfo *>(memalloc(sizeof(GDExtensionPropertyInfo) * cls->plist_owned.size())); \
219-
cls->plist_size = 0; \
220-
for (const ::godot::PropertyInfo &E : cls->plist_owned) { \
221-
cls->plist[cls->plist_size].type = static_cast<GDExtensionVariantType>(E.type); \
222-
cls->plist[cls->plist_size].name = E.name._native_ptr(); \
223-
cls->plist[cls->plist_size].hint = E.hint; \
224-
cls->plist[cls->plist_size].hint_string = E.hint_string._native_ptr(); \
225-
cls->plist[cls->plist_size].class_name = E.class_name._native_ptr(); \
226-
cls->plist[cls->plist_size].usage = E.usage; \
227-
cls->plist_size++; \
228-
} \
229-
if (r_count) \
230-
*r_count = cls->plist_size; \
231-
return cls->plist; \
232-
} \
233-
return m_inherits::get_property_list_bind(p_instance, r_count); \
227+
if (!p_instance) { \
228+
if (r_count) \
229+
*r_count = 0; \
230+
return nullptr; \
234231
} \
235-
return nullptr; \
232+
m_class *cls = reinterpret_cast<m_class *>(p_instance); \
233+
::godot::List<::godot::PropertyInfo> &plist_cpp = cls->plist_owned; \
234+
ERR_FAIL_COND_V_MSG(!plist_cpp.is_empty(), nullptr, "Internal error, property list was not freed by engine!"); \
235+
cls->_get_property_list(&plist_cpp); \
236+
return ::godot::internal::create_c_property_list(plist_cpp, r_count); \
236237
} \
237238
\
238239
static void free_property_list_bind(GDExtensionClassInstancePtr p_instance, const GDExtensionPropertyInfo *p_list) { \
239240
if (p_instance) { \
240241
m_class *cls = reinterpret_cast<m_class *>(p_instance); \
241-
ERR_FAIL_COND_MSG(cls->plist == nullptr, "Internal error, property list double free!"); \
242-
memfree(cls->plist); \
243-
cls->plist = nullptr; \
244-
cls->plist_size = 0; \
245242
cls->plist_owned.clear(); \
243+
/* TODO `GDExtensionClassFreePropertyList` is ill-defined, we need a non-const pointer to free this. */ \
244+
::godot::internal::free_c_property_list(const_cast<GDExtensionPropertyInfo *>(p_list)); \
246245
} \
247246
} \
248247
\

‎include/godot_cpp/core/class_db.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ void ClassDB::_register_class(bool p_virtual) {
188188
is_abstract, // GDExtensionBool is_abstract;
189189
T::set_bind, // GDExtensionClassSet set_func;
190190
T::get_bind, // GDExtensionClassGet get_func;
191-
T::get_property_list_bind, // GDExtensionClassGetPropertyList get_property_list_func;
191+
T::has_get_property_list() ? T::get_property_list_bind : nullptr, // GDExtensionClassGetPropertyList get_property_list_func;
192192
T::free_property_list_bind, // GDExtensionClassFreePropertyList free_property_list_func;
193193
T::property_can_revert_bind, // GDExtensionClassPropertyCanRevert property_can_revert_func;
194194
T::property_get_revert_bind, // GDExtensionClassPropertyGetRevert property_get_revert_func;

‎src/classes/wrapped.cpp

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,4 +60,33 @@ void postinitialize_handler(Wrapped *p_wrapped) {
6060
p_wrapped->_postinitialize();
6161
}
6262

63+
namespace internal {
64+
65+
GDExtensionPropertyInfo *create_c_property_list(const ::godot::List<::godot::PropertyInfo> &plist_cpp, uint32_t *r_size) {
66+
GDExtensionPropertyInfo *plist = nullptr;
67+
// Linked list size can be expensive to get so we cache it
68+
const uint32_t plist_size = plist_cpp.size();
69+
if (r_size != nullptr) {
70+
*r_size = plist_size;
71+
}
72+
plist = reinterpret_cast<GDExtensionPropertyInfo *>(memalloc(sizeof(GDExtensionPropertyInfo) * plist_size));
73+
unsigned int i = 0;
74+
for (const ::godot::PropertyInfo &E : plist_cpp) {
75+
plist[i].type = static_cast<GDExtensionVariantType>(E.type);
76+
plist[i].name = E.name._native_ptr();
77+
plist[i].hint = E.hint;
78+
plist[i].hint_string = E.hint_string._native_ptr();
79+
plist[i].class_name = E.class_name._native_ptr();
80+
plist[i].usage = E.usage;
81+
++i;
82+
}
83+
return plist;
84+
}
85+
86+
void free_c_property_list(GDExtensionPropertyInfo *plist) {
87+
memfree(plist);
88+
}
89+
90+
} // namespace internal
91+
6392
} // namespace godot

0 commit comments

Comments
 (0)