Skip to content

Don't call parent _get_property_list when a class doesn't define it (for internal binding). #1184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 26, 2023
Merged

Don't call parent _get_property_list when a class doesn't define it (for internal binding). #1184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 28 additions & 29 deletions include/godot_cpp/classes/wrapped.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ class Wrapped {
static GDExtensionBool property_get_revert_bind(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionVariantPtr r_ret) { return false; }
static void to_string_bind(GDExtensionClassInstancePtr p_instance, GDExtensionBool *r_is_valid, GDExtensionStringPtr r_out) {}

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

void _postinitialize();

Expand All @@ -95,9 +95,19 @@ class Wrapped {
GodotObject *_owner = nullptr;
};

namespace internal {

GDExtensionPropertyInfo *create_c_property_list(const ::godot::List<::godot::PropertyInfo> &plist_cpp, uint32_t *r_size);
void free_c_property_list(GDExtensionPropertyInfo *plist);

} // namespace internal

} // namespace godot

#define GDCLASS(m_class, m_inherits) \
// Use this on top of your own classes.
// Note: the trail of `***` is to keep sane diffs in PRs, because clang-format otherwise moves every `\` which makes
// every line of the macro different
#define GDCLASS(m_class, m_inherits) /***********************************************************************************************************************************************/ \
private: \
void operator=(const m_class &p_rval) {} \
friend class ::godot::ClassDB; \
Expand Down Expand Up @@ -209,40 +219,29 @@ public:
return false; \
} \
\
static inline bool has_get_property_list() { \
return m_class::_get_get_property_list() && m_class::_get_get_property_list() != m_inherits::_get_get_property_list(); \
} \
\
static const GDExtensionPropertyInfo *get_property_list_bind(GDExtensionClassInstancePtr p_instance, uint32_t *r_count) { \
if (p_instance && m_class::_get_get_property_list()) { \
if (m_class::_get_get_property_list() != m_inherits::_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!"); \
cls->_get_property_list(&cls->plist_owned); \
cls->plist = reinterpret_cast<GDExtensionPropertyInfo *>(memalloc(sizeof(GDExtensionPropertyInfo) * cls->plist_owned.size())); \
cls->plist_size = 0; \
for (const ::godot::PropertyInfo &E : cls->plist_owned) { \
cls->plist[cls->plist_size].type = static_cast<GDExtensionVariantType>(E.type); \
cls->plist[cls->plist_size].name = E.name._native_ptr(); \
cls->plist[cls->plist_size].hint = E.hint; \
cls->plist[cls->plist_size].hint_string = E.hint_string._native_ptr(); \
cls->plist[cls->plist_size].class_name = E.class_name._native_ptr(); \
cls->plist[cls->plist_size].usage = E.usage; \
cls->plist_size++; \
} \
if (r_count) \
*r_count = cls->plist_size; \
return cls->plist; \
} \
return m_inherits::get_property_list_bind(p_instance, r_count); \
if (!p_instance) { \
if (r_count) \
*r_count = 0; \
return nullptr; \
} \
return nullptr; \
m_class *cls = reinterpret_cast<m_class *>(p_instance); \
::godot::List<::godot::PropertyInfo> &plist_cpp = cls->plist_owned; \
ERR_FAIL_COND_V_MSG(!plist_cpp.is_empty(), nullptr, "Internal error, property list was not freed by engine!"); \
cls->_get_property_list(&plist_cpp); \
return ::godot::internal::create_c_property_list(plist_cpp, r_count); \
} \
\
static void free_property_list_bind(GDExtensionClassInstancePtr p_instance, const GDExtensionPropertyInfo *p_list) { \
if (p_instance) { \
m_class *cls = reinterpret_cast<m_class *>(p_instance); \
ERR_FAIL_COND_MSG(cls->plist == nullptr, "Internal error, property list double free!"); \
memfree(cls->plist); \
cls->plist = nullptr; \
cls->plist_size = 0; \
cls->plist_owned.clear(); \
/* TODO `GDExtensionClassFreePropertyList` is ill-defined, we need a non-const pointer to free this. */ \
::godot::internal::free_c_property_list(const_cast<GDExtensionPropertyInfo *>(p_list)); \
} \
} \
\
Expand Down
2 changes: 1 addition & 1 deletion include/godot_cpp/core/class_db.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ void ClassDB::_register_class(bool p_virtual) {
is_abstract, // GDExtensionBool is_abstract;
T::set_bind, // GDExtensionClassSet set_func;
T::get_bind, // GDExtensionClassGet get_func;
T::get_property_list_bind, // GDExtensionClassGetPropertyList get_property_list_func;
T::has_get_property_list() ? T::get_property_list_bind : nullptr, // GDExtensionClassGetPropertyList get_property_list_func;
T::free_property_list_bind, // GDExtensionClassFreePropertyList free_property_list_func;
T::property_can_revert_bind, // GDExtensionClassPropertyCanRevert property_can_revert_func;
T::property_get_revert_bind, // GDExtensionClassPropertyGetRevert property_get_revert_func;
Expand Down
29 changes: 29 additions & 0 deletions src/classes/wrapped.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,33 @@ void postinitialize_handler(Wrapped *p_wrapped) {
p_wrapped->_postinitialize();
}

namespace internal {

GDExtensionPropertyInfo *create_c_property_list(const ::godot::List<::godot::PropertyInfo> &plist_cpp, uint32_t *r_size) {
GDExtensionPropertyInfo *plist = nullptr;
// Linked list size can be expensive to get so we cache it
const uint32_t plist_size = plist_cpp.size();
if (r_size != nullptr) {
*r_size = plist_size;
}
plist = reinterpret_cast<GDExtensionPropertyInfo *>(memalloc(sizeof(GDExtensionPropertyInfo) * plist_size));
unsigned int i = 0;
for (const ::godot::PropertyInfo &E : plist_cpp) {
plist[i].type = static_cast<GDExtensionVariantType>(E.type);
plist[i].name = E.name._native_ptr();
plist[i].hint = E.hint;
plist[i].hint_string = E.hint_string._native_ptr();
plist[i].class_name = E.class_name._native_ptr();
plist[i].usage = E.usage;
++i;
}
return plist;
}

void free_c_property_list(GDExtensionPropertyInfo *plist) {
memfree(plist);
}

} // namespace internal

} // namespace godot