Skip to content
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

Jump to bottom

Avoid creating most objects that Godot is going to use placement new to initialize (mark 2) #1379

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Avoid creating most objects that Godot is going to use placement new to initialize (mark 2) #1379

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
14 changes: 14 additions & 0 deletions binding_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,8 @@ def generate_builtin_class_header(builtin_api, size, used_classes, fully_used_cl
result.append("")
result.append("\tstatic struct _MethodBindings {")

result.append("\t\tGDExtensionTypeFromVariantConstructorFunc from_variant_constructor;")

if "constructors" in builtin_api:
for constructor in builtin_api["constructors"]:
result.append(f'\t\tGDExtensionPtrConstructor constructor_{constructor["index"]};')
Expand Down Expand Up @@ -494,6 +496,9 @@ def generate_builtin_class_header(builtin_api, size, used_classes, fully_used_cl
result.append("\tstatic void init_bindings();")
result.append("\tstatic void _init_bindings_constructors_destructor();")

result.append("")
result.append(f"\t{class_name}(const Variant *p_variant);")

result.append("")
result.append("public:")

Expand Down Expand Up @@ -818,6 +823,10 @@ def generate_builtin_class_source(builtin_api, size, used_classes, fully_used_cl

result.append(f"void {class_name}::_init_bindings_constructors_destructor() {{")

result.append(
f"\t_method_bindings.from_variant_constructor = internal::gdextension_interface_get_variant_to_type_constructor({enum_type_name});"
)

if "constructors" in builtin_api:
for constructor in builtin_api["constructors"]:
result.append(
Expand Down Expand Up @@ -899,6 +908,11 @@ def generate_builtin_class_source(builtin_api, size, used_classes, fully_used_cl

copy_constructor_index = -1

result.append(f"{class_name}::{class_name}(const Variant *p_variant) {{")
dsnopek marked this conversation as resolved.
Show resolved Hide resolved
result.append("\t_method_bindings.from_variant_constructor(&opaque, p_variant->_native_ptr());")
result.append("}")
result.append("")

if "constructors" in builtin_api:
for constructor in builtin_api["constructors"]:
method_signature = f"{class_name}::{class_name}("
Expand Down
3 changes: 1 addition & 2 deletions include/godot_cpp/variant/variant.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ class ObjectID;
class Variant {
uint8_t opaque[GODOT_CPP_VARIANT_SIZE]{ 0 };

_FORCE_INLINE_ GDExtensionVariantPtr _native_ptr() const { return const_cast<uint8_t(*)[GODOT_CPP_VARIANT_SIZE]>(&opaque); }

friend class GDExtensionBinding;
friend class MethodBind;

Expand Down Expand Up @@ -145,6 +143,7 @@ class Variant {
static GDExtensionTypeFromVariantConstructorFunc to_type_constructor[VARIANT_MAX];

public:
_FORCE_INLINE_ GDExtensionVariantPtr _native_ptr() const { return const_cast<uint8_t(*)[GODOT_CPP_VARIANT_SIZE]>(&opaque); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep the underscore here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other variant types have a public _native_ptr() method, so this would match them. For some reason this one was private instead of public. In any case, it's not really part of the public API (since this method isn't in Godot), but we need these to be public for technical reasons.

Variant();
Variant(std::nullptr_t n) :
Variant() {}
Expand Down
84 changes: 33 additions & 51 deletions src/variant/variant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,123 +303,131 @@ Variant::operator float() const {
}

Variant::operator String() const {
String result;
to_type_constructor[STRING](result._native_ptr(), _native_ptr());
return result;
return String(this);
}

Variant::operator Vector2() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible?

Copy link
Collaborator Author

@dsnopek dsnopek Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible, but we decided to postpone it to later.

This PR fixes the memory leak. Avoiding initializing those other results would be a slight performance improvement, but those aren't leaking memory, so fixing them isn't as urgent. The @todo's are there to remind us to fix it eventually, though :-)

Vector2 result;
to_type_constructor[VECTOR2]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Vector2i() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Vector2i result;
to_type_constructor[VECTOR2I]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Rect2() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Rect2 result;
to_type_constructor[RECT2]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Rect2i() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Rect2i result;
to_type_constructor[RECT2I]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Vector3() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Vector3 result;
to_type_constructor[VECTOR3]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Vector3i() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Vector3i result;
to_type_constructor[VECTOR3I]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Transform2D() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Transform2D result;
to_type_constructor[TRANSFORM2D]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Vector4() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Vector4 result;
to_type_constructor[VECTOR4]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Vector4i() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Vector4i result;
to_type_constructor[VECTOR4I]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Plane() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Plane result;
to_type_constructor[PLANE]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Quaternion() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Quaternion result;
to_type_constructor[QUATERNION]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator godot::AABB() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
godot::AABB result;
to_type_constructor[AABB]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Basis() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Basis result;
to_type_constructor[BASIS]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Transform3D() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Transform3D result;
to_type_constructor[TRANSFORM3D]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Projection() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Projection result;
to_type_constructor[PROJECTION]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator Color() const {
// @todo Avoid initializing result before calling constructor (which will initialize it again)
Color result;
to_type_constructor[COLOR]((GDExtensionTypePtr)&result, _native_ptr());
return result;
}

Variant::operator StringName() const {
StringName result;
to_type_constructor[STRING_NAME](result._native_ptr(), _native_ptr());
return result;
return StringName(this);
}

Variant::operator NodePath() const {
NodePath result;
to_type_constructor[NODE_PATH](result._native_ptr(), _native_ptr());
return result;
return NodePath(this);
}

Variant::operator godot::RID() const {
godot::RID result;
to_type_constructor[RID](result._native_ptr(), _native_ptr());
return result;
return godot::RID(this);
}

Variant::operator Object *() const {
Expand Down Expand Up @@ -447,81 +455,55 @@ Variant::operator ObjectID() const {
}

Variant::operator Callable() const {
Callable result;
to_type_constructor[CALLABLE](result._native_ptr(), _native_ptr());
return result;
return Callable(this);
}

Variant::operator Signal() const {
Signal result;
to_type_constructor[SIGNAL](result._native_ptr(), _native_ptr());
return result;
return Signal(this);
}

Variant::operator Dictionary() const {
Dictionary result;
to_type_constructor[DICTIONARY](result._native_ptr(), _native_ptr());
return result;
return Dictionary(this);
}

Variant::operator Array() const {
Array result;
to_type_constructor[ARRAY](result._native_ptr(), _native_ptr());
return result;
return Array(this);
}

Variant::operator PackedByteArray() const {
PackedByteArray result;
to_type_constructor[PACKED_BYTE_ARRAY](result._native_ptr(), _native_ptr());
return result;
return PackedByteArray(this);
}

Variant::operator PackedInt32Array() const {
PackedInt32Array result;
to_type_constructor[PACKED_INT32_ARRAY](result._native_ptr(), _native_ptr());
return result;
return PackedInt32Array(this);
}

Variant::operator PackedInt64Array() const {
PackedInt64Array result;
to_type_constructor[PACKED_INT64_ARRAY](result._native_ptr(), _native_ptr());
return result;
return PackedInt64Array(this);
}

Variant::operator PackedFloat32Array() const {
PackedFloat32Array result;
to_type_constructor[PACKED_FLOAT32_ARRAY](result._native_ptr(), _native_ptr());
return result;
return PackedFloat32Array(this);
}

Variant::operator PackedFloat64Array() const {
PackedFloat64Array result;
to_type_constructor[PACKED_FLOAT64_ARRAY](result._native_ptr(), _native_ptr());
return result;
return PackedFloat64Array(this);
}

Variant::operator PackedStringArray() const {
PackedStringArray result;
to_type_constructor[PACKED_STRING_ARRAY](result._native_ptr(), _native_ptr());
return result;
return PackedStringArray(this);
}

Variant::operator PackedVector2Array() const {
PackedVector2Array result;
to_type_constructor[PACKED_VECTOR2_ARRAY](result._native_ptr(), _native_ptr());
return result;
return PackedVector2Array(this);
}

Variant::operator PackedVector3Array() const {
PackedVector3Array result;
to_type_constructor[PACKED_VECTOR3_ARRAY](result._native_ptr(), _native_ptr());
return result;
return PackedVector3Array(this);
}

Variant::operator PackedColorArray() const {
PackedColorArray result;
to_type_constructor[PACKED_COLOR_ARRAY](result._native_ptr(), _native_ptr());
return result;
return PackedColorArray(this);
}

Variant &Variant::operator=(const Variant &other) {
Expand Down
Loading