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.

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

Set instance and instance binding in Wrapped constructor. #1446

Conversation

Daylily-Zeleen
Copy link
Contributor

@Daylily-Zeleen Daylily-Zeleen commented Apr 24, 2024

Fixes #1039

Refer to this comment. This is my solution to advance the timing of setting instance and instance binding.

There may be some potential problems that I haven't found, please review it carefully.

This is my test class:

class MyLineEdit : public LineEdit {
	GDCLASS(MyLineEdit, LineEdit)
protected:
	static void _bind_methods() {}

public:
	void _on_text_submitted(const String &p_test) {
		UtilityFunctions::print("Submitted: ", p_test);
	}

	MyLineEdit() {
		connect("text_submitted", callable_mp(this, &MyLineEdit::_on_text_submitted));
	}
};

Before this pr, it will push an error:

ERROR: Condition "_instance_bindings != nullptr && _instance_bindings[0].binding != nullptr" is true.
   at: set_instance_binding (core/object/object.cpp:1824)

@Daylily-Zeleen Daylily-Zeleen requested a review from a team as a code owner April 24, 2024 14:28
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/set_instance_and_instance_biding_in_Wrapped_constructor branch 3 times, most recently from 76100c1 to 0230024 Compare April 24, 2024 16:10
@Daylily-Zeleen Daylily-Zeleen marked this pull request as draft April 24, 2024 16:18
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/set_instance_and_instance_biding_in_Wrapped_constructor branch from 0230024 to a57118d Compare April 24, 2024 16:21
@Daylily-Zeleen Daylily-Zeleen marked this pull request as ready for review April 24, 2024 16:32
@Daylily-Zeleen Daylily-Zeleen changed the title Set instance and instance binding in Wrapped constructor. Set instance and instance binding in Wrapped constructor. Apr 24, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Apr 24, 2024

As pointed out in a comment on a similar issue, moving this setup to the Wrapped constructor is tricky because there are two paths for extension classes to be created: from within the extension, and from Godot.

I haven't had a chance to really review the code here, but I think we need automated tests that ensure both paths work correctly. This should either be part of this PR or in a PR that's merged before it, since this is one of the things that has gotten broken and fixed in godot-cpp a couple of times.

@AThousandShips AThousandShips added bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation labels Apr 24, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Apr 24, 2024
@Daylily-Zeleen
Copy link
Contributor Author

As pointed out in a comment on a similar issue, moving this setup to the Wrapped constructor is tricky because there are two paths for extension classes to be created: from within the extension, and from Godot.

I don't know if there are other ways to create extension class objects that I have neglected.
I found that create extension class from extension and create from godot are the same way.
In extension, we use memnew() directly.
In godot, create extension classe through GDExtensionClassCreationInfo3::create_instance_func, it is point to ClassDB::_create_instance_func in godot-cpp, and use memnew(), too.

Wrapped have another constructor Wrapped(GodotObject *p_godot_object), it can only be used to godot classes' create_callback, I'm not sure about its function. We don't use this constructor in godot-cpp, so this constructor should not relate about this pr.

I haven't had a chance to really review the code here, but I think we need automated tests that ensure both paths work correctly. This should either be part of this PR or in a PR that's merged before it, since this is one of the things that has gotten broken and fixed in godot-cpp a couple of times.

About test, I think the content in "test" folder already done this thing. Both creating extensions node(Example) in godot editor, and creating extension object by memnew(ExampleRef) in extension self.

If there have other situations need to be tested, please point them out.

@godotengine godotengine deleted a comment from Daylily-Zeleen Apr 25, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Apr 25, 2024

About test, I think the content in "test" folder already done this thing. Both creating extensions node(Example) in godot editor, and creating extension object by memnew(ExampleRef) in extension self.

Yeah, we're already creating objects in both ways, but we aren't really testing it, ie. the tests won't necessarily fail if we get this wrong.

Looking over old issues/PRs where we broke this previously, we commonly got particular error messages, for example:

ERROR: Condition "_instance_bindings != nullptr" is true

In the current code, I think that message would actually be different, but perhaps making the CI fail if we got those sort of error messages would help here?

Anyway, given that this is tricky, and we have a history of breaking and fixing this multiple times in the past, I'd really like to have some support from the automated tests to help us get this right. :-)

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/set_instance_and_instance_biding_in_Wrapped_constructor branch from a57118d to acd8e4c Compare April 25, 2024 17:21
@Daylily-Zeleen
Copy link
Contributor Author

Daylily-Zeleen commented Apr 25, 2024

I found the reson of this error ERROR: Condition "_instance_bindings != nullptr && _instance_bindings[0].binding != nullptr" is true.:

godot::internal::get_object_instance_binding() will pass the binding_callbacks to Object::get_instnce_binding() in godot, and set instance binding if it has not been set.

In my example, connect will check the custom callble's validity, and godot::internal::get_object_instance_binding() will be called for cheacking.
Before this pr, instance binding still not be set at this time, and Object::get_instnce_binding() will set it. After construction of extension object, instance binding will be set again (by Wrapped::_postinitialize()), and the error will be printed.

We can draw a conclusion, the key point of the new test is to check whether the binding is set many times unexpectedly, instead of creating extension class in godot-cpp and godot.

But I can't find a elegant way to do this check currently, maybe we need some new things which provided by godot.


godot::internal::get_object_instance_binding() has been used in many type conversion situations. To ensure users can use them in extension class constructors, we must let instance binding have been set before extension class constructors.

So I added a new test to check whether the binding was set before the constructor of extension class.

@dsnopek
Copy link
Collaborator

dsnopek commented May 1, 2024

Thanks for your work on this PR!

I haven't had a chance to test this yet, but skimming the code I think this could work. I worry a bit about the extra complexity, and if this is going to lead to more function calls during object construction. A few of the new functions are marked as _ALWAYS_INLINE_ which is good. Do we really need the Wrapped::_initialize() function? It seems like the only place its called is in Wrapped::Wrapped(const StringName)?

Anyway, I still need to spend some more time to try this out.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/set_instance_and_instance_biding_in_Wrapped_constructor branch from acd8e4c to fbaf79a Compare May 2, 2024 05:05
@Daylily-Zeleen
Copy link
Contributor Author

Yes, Wrapped::_initialize() is redundant, thanks for your reminder.

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this!

I've finally gotten around to doing some testing and deeper review - I have a couple of notes.

However, overall, I think I'm personally in favor of going forward with this. While C++ doesn't make this easy for us, I feel like the added complexity isn't too bad, and this does fix a big compatibility issue between what developers can do in modules vs GDExtensions.

include/godot_cpp/classes/wrapped.hpp Outdated Show resolved Hide resolved
include/godot_cpp/classes/wrapped.hpp Outdated Show resolved Hide resolved
include/godot_cpp/classes/wrapped.hpp Outdated Show resolved Hide resolved
test/src/example.cpp Show resolved Hide resolved
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/set_instance_and_instance_biding_in_Wrapped_constructor branch 4 times, most recently from 2c6ef7a to 3b4ff60 Compare May 16, 2024 17:44
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/set_instance_and_instance_biding_in_Wrapped_constructor branch 3 times, most recently from de0b3d0 to c233430 Compare May 18, 2024 15:30
@dsnopek
Copy link
Collaborator

dsnopek commented Aug 16, 2024

@dmlary Godot objects descending from Object aren't meant to be allocated on the stack. Also, in your case, you're using a RefCounted object, but not using the Ref<T> template, which means it won't actually be reference counted.

Instead of:

CylinderMesh m;

... you should use:

Ref<CylinderMesh> m;
m.instantiate();

@dmlary
Copy link

dmlary commented Aug 16, 2024

Out of curiosity, why not? And why is a stack allocated instance ref counted behind the scenes? The object instance isn’t going into the scene; I’m only creating the instance because CylinderMesh:: create_mesh_array() isn’t exposed.

Generally, is there a place I can read more about what is going on here? I would expect to be able to declare classes on the stack and have the initializer do the proper thing. I’d like to learn more so I can avoid any other footguns.

@dsnopek
Copy link
Collaborator

dsnopek commented Aug 16, 2024

Out of curiosity, why not?

One of godot-cpp's core design pillars is that code should match (as closely as possible) code in the engine itself, such that (ideally) you can copy C++ code between Godot and godot-cpp, and have it work the same.

In Godot, objects aren't fully setup just by their constructor, you need to use memnew() to do the full process, which we also do in godot-cpp (although, we do different setup than in Godot itself).

And why is a stack allocated instance ref counted behind the scenes?

Ref<T> is a smart pointer - it does the memnew() internally for new objects, but also is in charge of incrementing and decrementing the reference count. Again, this is the same as in the engine.

If we weren't trying to match Godot's internal API, we could perhaps bake the incrementing/decrementing of the reference count into the classes constructor/destructor.

Generally, is there a place I can read more about what is going on here?

Unfortunately, there isn't a lot of documentation on how Godot's internal C++ API are designed. Personally, my primary reference is Godot's source code itself, but I know that isn't ideal. :-)

@Daylily-Zeleen
Copy link
Contributor Author

Daylily-Zeleen commented Aug 16, 2024

@dsnopek It seems that it is not uncommon to instantiate a godot object without using memnew().
May we remove ERR_PRINT() and CRASH_NOW_MSG() in Wrapped::Wrapped(const StringName p_godot_class)?
If I remember correctly, define a godot object without memnew() in the old logic will not set instance binding and binding callbacks, too.

In most cases, a stack allocated godot object like Mutex, we are not use its signals, so define at stack (has not instance binding) is reasonable.
But if we remove the crashing expression, beginer users may improperly use signals of a stack allocated object.
This seems like a dilemma.

@dsnopek
Copy link
Collaborator

dsnopek commented Aug 16, 2024

@dsnopek It seems that it is not uncommon to instantiate a godot object without using memnew().

Yeah, it's interesting how many people were apparently doing things that should never have worked! And, even if they did sort of work, they were probably not hitting other issues by luck, and/or causing memory leaks they just weren't aware of.

May we remove ERR_PRINT() and CRASH_NOW_MSG() in Wrapped::Wrapped(const StringName p_godot_class)?

Personally, I think we should keep it as-is. We do need to improve our documentation to better educate developers in the first place, but I think this is primarily a transition thing, and after a couple weeks folks will have adjusted.

In most cases, a stack allocated godot object like Mutex, we are not use its signals, so define at stack (has not instance binding) is reasonable.

I don't think signals are the only thing we have to worry about. There's potentially a lot of things that could go wrong if the Godot objects aren't fully setup, since everything in Godot is going to assume that all objects are.

But if we remove the crashing expression, beginer users may improperly use signals of a stack allocated object.
This seems like a dilemma.

Having a loud error with a clue, is better than silently "succeeding" with future hidden traps. :-)

@Daylily-Zeleen
Copy link
Contributor Author

Could we find out all situations which need instance binding and add some check for them?

@dsnopek
Copy link
Collaborator

dsnopek commented Aug 16, 2024

Could we find out all situations which need instance binding and add some check for them?

Is your thought that we could allow users to use not-fully-setup objects, but then add checks to the situations where we know the result would be particularly bad?

Aside from that being tricky to do (I don't know how we'd know an instance binding should exist to check for it being missing), I don't think that's a very sustainable solution. I personally think it would be better to focus on how we can guide users to use the API correctly.

However, just to for clarity (for those who don't know the technical details), I think the main situation we need an instance binding is any time an object is passed from GDExtension to Godot and back to GDExtension. Otherwise, multiple redundant wrapper objects could be created. Those won't necessarily cause crashes (although, they could if you delete more than one of the wrappers), but it would leak memory, and cause unexpected situations, for example, where you pass an object to Godot, but get a different one when it passes it back to you (even if its the same object on the Godot side), making == not do what you think it should.

@Daylily-Zeleen
Copy link
Contributor Author

If we can find and add check for all situations which need instance binding, we can add a method to setup binding manually.
When users use a stack allocated godot object uncorrectly, they will get an error, and they can setup binding manually or just use memnew() instead of allocating at stack.

I think some simple godot objects to be allocated at stack is reasonable, like Mutex, RegEx and RandomNumberGenerator.

@dsnopek
Copy link
Collaborator

dsnopek commented Aug 16, 2024

I think some simple godot objects to be allocated at stack is reasonable, like Mutex, RegEx and RandomNumberGenerator.

I disagree.

Setting the instance binding when creating an object is part of correctly using the GDExtension interface. I don't think we should be selectively saying that sometimes for some classes we don't do the complete process of creating an object.

@Daylily-Zeleen
Copy link
Contributor Author

Daylily-Zeleen commented Aug 16, 2024

The requirement is to support allocating godot objects at stack.
The idea result is to fully setup godot object both at stack and heap.

I had a sudden inspiration, to add a macro, for example, defiene it as #define stack_alloc() xxxx, let it do the same thing like memnew(), but not use new and the return value is not a pointer.

But this plan will break the code style which similar to godot source (because godot side support allocate objects at stack naturally ).

I think I have exhausted my all solutions on this issue, no one is perfect😌.

@dsnopek
Copy link
Collaborator

dsnopek commented Aug 16, 2024

But this plan will break the code style which similar to godot source (because godot side support allocate objects at stack naturally ).

Even within Godot itself, classes that descend from Object should not be created on the stack - they need to be created with memnew() to get fully setup. The potential issues are different because the setup is different, but it's still a situation where it may sort of work in some circumstances, but can lead to unexpected problems or crashes in others.

@Daylily-Zeleen
Copy link
Contributor Author

Daylily-Zeleen commented Aug 16, 2024

Even within Godot itself, classes that descend from Object should not be created on the stack

Almost RegEx are allocate at stack in godot, here is an example.
I had met a unexpected behavior when useing Ref<RefEx> in godot module, and use RegEx can get a correct result.
Anyway, the issue between Ref<RegEx> and RegEx may not occur in godot-cpp, here I just point out that, sometime godot will allocate godot objects at stack, too.

@dmlary
Copy link

dmlary commented Sep 15, 2024

In Godot, objects aren't fully setup just by their constructor, you need to use memnew() to do the full process, which we also do in godot-cpp (although, we do different setup than in Godot itself).

I'm going to document a knock-on effect of this, and propose an option.

I'm working on a GDExtension conversion of godotengine/godot#85890 to implement a GridMap with hex-shaped cells. One of the big changes is converting from Vector3i to a CellId class to abstract away the internal coordinate system (important for hex cell coordinates). This is a pile-of-data class that just holds three ints, but with a lot of helper methods such as neighbors, distance, etc.

Internally the C++ code uses CellId to calculate all manner of things: distance between cells, global position for drawing meshes, selecting regions of cells, etc. There are dozens of places where I create instances of CellId on the stack, and use it to generate some result. I also use static consts such as CellId::Origin for other calculations. Here's an explicit example where the class is simply used as a calculator; this is a common pattern throughout the c++ code.

// pick a point on the middle of an edge to find the closest edge of the cells
float max = CellId(GRID_RADIUS, -(int)GRID_RADIUS / 2, 0)
	.unit_center()
	.length_squared();

The HexMap class has GDScript bindings that want to return CellId, but that's only possible if CellId is a RefCounted godot::Object (or return *CellId and leak memory). If I do that, I have to change all of the existing uses of the class to dynamically allocate and initialize a pile of bindings that will never be used. So the example code above becomes:

CellId *edge_cell = memnew(CellId(GRID_RADIUS, -(int)GRID_RADIUS / 2, 0));
float max = edge_cell->unit_center().length_squared();
memdelete(edge_cell);

I'm not doing this for both performance reasons and it gets ungodly ugly in the more complex places like cell iteration.

(ugly) Work-around

The work-around I'm using right now is that for any type I use natively in C++, that also needs to be accessible in GDScript, I create a wrapper class such as CellIdRef that only holds a CellId. Then I return Ref<CellIdRef> in GDScript bound functions. Then I have to bind every method in CellId in CellIdRef. This is a pretty boilerplate-heavy way to operate.

Proposal

Would it be possible to add a compiler flag to turn off this guard rail?

There are cases such as for CellId where it is completely legitimate to use a class in C++ without ever initializing the bindings. And to expose the class to GDScript, it's correctly bound by the call to Ref<T>.instantiate().

I understand the guardrail was added to catch a common problem of passing uninitialized godot::Objects into the engine, but the side effect is that you're barring common use of C++ classes that just happen to be godot::Object to be passed into GDScript.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 15, 2024

If your purpose is to specifically expose such types to the scripting API, then it would be an extremely bad idea to make those types incomplete.

Instead, consider exposing setters and getters on the parent class for a proxied access to the properties of this data structure that you want to expose. This would also be a more conventional way to design the API in Godot.

PS. You don't actually need the set method in your example. You can pass arguments to the constructor in memnew. Although with so many arguments it may look too heavy on the reader.

@dmlary
Copy link

dmlary commented Sep 15, 2024

If your purpose is to specifically expose such types to the scripting API, then it would be an extremely bad idea to make those types incomplete.

@YuriSizov I called out in my comment that I was not exposing incomplete types to the GDScript layer. The classes will be properly bound when they reach the scripting api because they can only reach GDScript by being returned via Ref<T>, which will call memnew() during Ref<T>.instantiate().

The purpose of this is to use the types in C++, because they’re safe to use in C++ without setting up the engine bindings. There are a sufficient number of instances where these types are created, used, and discarded without crossing the boundary into the core engine to consider allowing the behavior.

You can pass arguments to the constructor in memnew.

EDIT: I found the proper syntax; it's not an argument to memnew() it's passing the class constructor memnew(CellId(1,2,3)). I did initially try this, but didn't properly create the needed constructor. I'll update the code sample in my previous comment, to reflect this capability.

This still leaves the performance hit associated with dynamically allocating and releasing the type when it is safe to use on the stack.

ORIGINAL (incorrect):
That may be the case in the godot engine c++ code, but it does not look to be the case for godot-cpp. The memnew macro doesn’t take any additional args beyond class name, and I don’t see any macro args in the call to new.

#define memnew(m_class) (::godot::_pre_initialize<std::remove_pointer_t<decltype(new ("", "") m_class)>>(), ::godot::_post_initialize(new ("", "") m_class))

@Daylily-Zeleen
Copy link
Contributor Author

Daylily-Zeleen commented Sep 15, 2024

@dmlary In my opinion, only some native godot objects worth to be used without memnew().

If you don't want to expose a class to scripting, the class don't need to inherit from godot classes.
If your class need godot features such as signals, set() and call() and so on, you must inherit from native godot classes and use memnew() for safety.

In some case, you can consider to combine them by using multiple Inheritance.


Proposal

Would it be possible to add a compiler flag to turn off this guard rail?

There are cases such as for CellId where it is completely legitimate to use a class in C++ without ever initializing the bindings. And to expose the class to GDScript, it's correctly bound by the call to Ref<T>.instantiate().

I understand the guardrail was added to catch a common problem of passing uninitialized godot::Objects into the engine, but the side effect is that you're barring common use of C++ classes that just happen to be godot::Object to be passed into GDScript.

As my previous comment show, some classes in godot source code are used in stack allocated. To force require users use memnew() is break the principle that make godot-cpp similar to godot source.
So I think this is a proper way to allow use stack allocated godot object, and we can enable this check and crash by default for safety.

@dmlary
Copy link

dmlary commented Sep 15, 2024

So I think this is a proper way to allow use stack allocated godot object, and we can enable this check and crash by default for safety.

@Daylily-Zeleen I can open a PR with this change.

dmlary added a commit to dmlary/godot-cpp that referenced this pull request Sep 15, 2024
The guard in `Wrapped::Wrapped(const StringName)` detects the partial
initialization of `godot::Object` subclasses.  This is needed because
using `new` to allocate a `godot::Object` subclass will result in
unexpected crashes because the bindings for the instance have not been
set.

The side effect of this guard is that it also prevents any use of a
`godot::Object` subclass on the stack.

There are godot native types such as `godot::RegEx` that are safe to use
on the stack, and are used that way throughout the godot source.
Similarly, custom classes that subclass `godot::Object` may also be used
safely on the stack[^1].

In this commit, I add a compiler flag to disable this guard.  This will
allow godot-cpp users who understand the safety implications to use
`godot::Object` subclasses on the stack.

[^1]: godotengine#1446 (comment)
@YuriSizov
Copy link
Contributor

I called out in my comment that I was not exposing incomplete types to the GDScript layer. The classes will be properly bound when they reach the scripting api because they can only reach GDScript by being returned via Ref\u003CT>, which will call memnew() during Ref\u003CT>.instantiate().

Okay, I did not understand that initially. Then using two different types, plain one and a wrapper, makes sense for how Godot is designed.

@dmlary
Copy link

dmlary commented Sep 15, 2024

[...] using two different types, plain one and a wrapper, makes sense for how Godot is designed.

@YuriSizov the problem with using two different types is that you end up having to write a lot of boilerplate for that wrapper class. Any function you expose that doesn't require wrapping or unwrapping a type in Ref<T> is going to end up being a straight pass-through function to the wrapped type. This is an unnecessary maintenance cost as classes grow.

I've opened PR #1585 to allow GDExtension creators to use a compiler flag to disable the guard that prevents the use of godot::Object subclasses from being used on the stack. The default is to leave the guard in place so people learn about the need for memnew(), but it allows advanced usage for people who don't want to pay the performance & maintenance cost.

@YuriSizov on a separate note, I found Ref<T> doesn't have the same support for a constructor to call similar to memnew(). This makes the examples below ugly when we need to copy an existing class instance into Ref<T>.

Full example using a wrapper class

I've been going down this path in my repo, and I wanted to provide a concrete example of what using a separate wrapper class looks like.

I've cleaned up one of the C++ classes I'm working with. This class CellId can be thought of as the equivalent of Vector3 but in a different coordinate system. It provides many calculation functions, but only contains three integers.

class CellId {
public:
	// axial hex coordinates, and y coordinate
	int q, r, y;

	CellId() : q(0), r(0), y(0){};
	CellId(int q, int r, int y) : q(q), r(r), y(y){};

	// get the position of the center of this cell, assuming cell radius=1,
	// height=1
	Vector3 unit_center() const;

	// check if a point falls within this cell
	bool contains_point(Vector3 &point) const;

	// calculate the distance from another hex cell
	unsigned distance(const CellId &) const;

	// get the CellId of the neighbor in the specified direction
	CellId get_neighbor(int direction) const;
};

I use this class in the following ways:

  • Allocate on the stack in C++ and use only in C++ without passing to godot scripting layer
    • The class is used as a calculator, similar to Vector3
  • Allocate on the heap and return to GDScript
    • this allows GDScripts to make use of all the calculation functions in the class
    • if there were a way to expose the class to GDScript without inheriting Object, I would do it

For the CellId class I cherry-picked four functions that are a representative sample of the types of functions that would be exposed to GDScript:

  • unit_center() godot native return type, no args
  • contains_point() native return type, godot native arg
  • distance() native return type, custom type arg
  • get_neighbor() custom return type, native arg

The wrapper class for CellId that exposes these four functions looks like this:

class CellIdWrapper : public RefCounted {
	GDCLASS(CellIdWrapper, RefCounted)

	CellId cell_id;

public:
	CellIdWrapper(const CellId &cell_id) : cell_id(cell_id){};
	CellIdWrapper(){};

	Vector3 unit_center() const { return cell_id.unit_center(); };

	bool contains_point(Vector3 point) const {
		return cell_id.contains_point(point);
	}

	unsigned distance(const Ref<CellIdWrapper> other) const {
		return cell_id.distance(other->cell_id);
	}

	Ref<CellIdWrapper> get_neighbor(int direction) const {
		Ref<CellIdWrapper> ref;
		ref.instantiate();
		ref->cell_id = cell_id.get_neighbor(direction);
		return ref;
	};

protected:
	static void _bind_methods();
};

Of the four types of functions, only two of them require GDScript specific code. unit_center() and contains_point() are pure proxy functions that shouldn't need to be written. Only functions that need to wrap/unwrap a Ref<T> need customized code versus their C++ specific implementations.

Example with guard disabled, allowing stack allocation

When the guard is disabled, we gain the ability to use CellId as a stack-allocated class in C++, but also can set up the bindings when it is passed into GDScript. Here is the CellId class when the guard is disabled. The class is correctly bound in the functions that support GDScript.

class CellId : public RefCounted {
	GDCLASS(CellId, RefCounted)

public:
	// axial hex coordinates, and y coordinate
	int q, r, y;

	CellId() : q(0), r(0), y(0){};
	CellId(int q, int r, int y) : q(q), r(r), y(y){};

	// get the position of the center of this cell, assuming cell radius=1,
	// height=1
	Vector3 unit_center() const;

	// check if a point falls within this cell
	bool contains_point(Vector3 &point) const;

	// calculate the distance from another hex cell
	unsigned distance(const CellId &) const;
	unsigned _distance(const Ref<CellId> other) const {
		return distance(**other);
	}

	// get the CellId of the neighbor in the specified direction
	CellId get_neighbor(int direction) const;
	Ref<CellId> _get_neighbor(int direction) const {
		CellId neighbor = get_neighbor(direction);
		Ref<CellId> ref;
		ref.instantiate();
		// this part is ugly, but it's the most direct method I could find to keep comparison clear
		ref->q = neighbor.q;
		ref->r = neighbor.r;
		ref->y = neighbor.y;
		return ref;
	};

protected:
	static void _bind_methods();
};

The key change is that the GDScript-specific functions get renamed to _distance() and _get_neighbor(). Renaming is necessary for _distance() because ClassDB::bind_method() has issues with overloaded functions. _get_neighbor() has to be renamed because overloaded functions with the same arguments must return the same type.

Overall the difference lies in that we can bind the existing unit_center() and contains_point() functions without creating extranious wrappers. We also don't have to maintain an additional wrapper class.

Another possible solution

If ClassDB::bind_method() supported binding to a method on a instance variable, then it would be significantly more reasonable to maintain a wrapper class. For example binding unit_center() to CellIdWrapper.cell_id.unit_center(). In this case only Ref<T> wrapper functions would need to be written, with the rest bound to the methods on the wrapped type.

This approach works well for my case, but doesn't help anyone who wants to use native godot types like RegEx on the stack.

If this path is taken, it would be nice if we could also override the name of the class used in GDScript. I'm already getting tired of typing CellIdRef in my gdscript tests.

@YuriSizov
Copy link
Contributor

on a separate note, I found Ref<T> doesn't have the same support for a constructor to call similar to memnew(). This makes the examples below ugly when we need to copy an existing class instance into Ref<T>.

Sorry, didn't have a chance to read through the whole thing, but wanted to quickly point out that you are allowed to assign memnew'd instances to Ref<T>, e.g. from my own code:

Ref<SiOPMWavePCMData> pcm_data = memnew(SiOPMWavePCMData(p_data, (int)(p_sampling_note * 64), p_src_channel_count, p_channel_count));

I don't think this introduces any performance differences, as it should simply use the assigned instance in place of the internally initialized one.

And also, yes, I understand the boilerplate argument. I am of an opinion that the style of the godot-cpp project should stay self-consistent, and one of the goals of this project is to let you code in a way that can be migrated between extensions and modules. This is by no means the only way you can use the GDExtension API of the engine, and if the chosen style and considerations that godot-cpp has to deal with don't let you code comfortably, you can definitely roll out your own bindings.

But for the official project I'd prefer if it would stick to an authoritative approach rather than flexibility to accommodate different styles, approaches, and targets. I understand that your main argument is performance, but the engine itself seems to exist fine within the same limitations, so it doesn't strike me as such a critical problem as to address it in the general purpose binding that is godot-cpp.

@dmlary
Copy link

dmlary commented Sep 16, 2024

[...] you are allowed to assign memnew'd instances to Ref [...]

@YuriSizov oh, that's nice! Thank you for the tip.

I understand that your main argument is performance, but the engine itself seems to exist fine within the same limitations, [...]

@YuriSizov But it actually doesn't. Godot explicitly made Vector3, Vector2, and all similar classes to CellId native godot types and not Godot Objects for performance & usability reasons. Imagine if every Vector3 had to be memnew'd; the engine would slow to a crawl.

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 16, 2024

I agree with a lot of what @YuriSizov wrote above:

Okay, I did not understand that initially. Then using two different types, plain one and a wrapper, makes sense for how Godot is designed.

I agree, that this would be a good way to handle this.

You can have plain C++ classes that you use internally, which you can use however you want. But if you want to bind a class with Godot via godot-cpp, then there are some rules that need to be followed. Separating those into separate classes (internal ones vs bound Godot wrappers) makes a lot of sense to me.

I am of an opinion that the style of the godot-cpp project should stay self-consistent, and one of the goals of this project is to let you code in a way that can be migrated between extensions and modules.

I agree with this as well. One of godot-cpp's design goals is to mimick Godot's internal APIs as much as possible, and that includes the rules for how to use classes that descend from Object and RefCounted.

I understand that there are a few places in Godot where it isn't presently following these rules. However, as I've said previously, I'm of the opinion that those are bugs that should be fixed in Godot, not something we should be attempting to allow from godot-cpp.

This is by no means the only way you can use the GDExtension API of the engine, and if the chosen style and considerations that godot-cpp has to deal with don't let you code comfortably, you can definitely roll out your own bindings.

Yes, godot-cpp isn't the only possible way to build a C++ binding for GDExtension. It has a particular set of goals and that has influenced its design choices.

@dmlary
Copy link

dmlary commented Sep 16, 2024

Ok, it seems y'all are pretty fixed on preserving the guard. I can accept that.

It seems like the accepted solution for performance is a C++-only class, and a wrapper class for sharing values to GDScript.

To reduce the maintenance overhead on the wrapper class, is there any way to add the ability to bind a GDScript method to an instance variable in the godot object? In concrete terms from the example1 binding CellIdWrapper.get_unit_center() to CellIdWrapper.cell_id.unit_center().

Footnotes

  1. https://github.com/godotengine/godot-cpp/pull/1446#issuecomment-2351787206

dmlary added a commit to dmlary/godot-hex-map that referenced this pull request Sep 21, 2024
Had to add a class for wrapping HexMapCellId to pass to GDScript.

There's a whole series of problems here.  First of all,
ClassDB::bind_method() can only bind methods that return a type that can
be cast to a Variant.  From some resources the only two viable options
were Ref<T> or a pointer to an Object subclass[^1].  Pointer means we
need to somehow manage memory that we've entirely passed up to GDScript.
Instead we use Ref<T>.

Second problem, you cannot use Object subclasses on the stack in
godot-cpp[^2].  This means if we make HexMapCellId a subclass of
godot::Object, we cannot use it on the stack, or easily construct it
using an initializer.  Insteat we'd have to use `memnew()` all over the
place.

So we're using a wrapper class `HexMapCellId < public godot::RefCounted`.
It's a boiler-plate heavy approach, but it works for now.

Did propose to godot-cpp to add a compiler flag to disable the
guard-rail that keeps us from using `godot::Object` subclasses on the
stack[^3].

[^1]: https://forum.godotengine.org/t/returning-custom-classes-from-c-gdextension/65920/18
[^2]: godotengine/godot-cpp#1446 (comment)
[^3]: godotengine/godot-cpp#1446 (comment)
dmlary added a commit to dmlary/godot-hex-map that referenced this pull request Sep 21, 2024
Have two approches, C++ class and GDScript wrapper class `HexMapCellId`
and `HexMapCellIdRef`, or unified class in `HexMapIterAxial`.  The
unified approach requires an upstream change[^1] to allow stack
allocation of `godot::Object` classes.

`HexMapIterAxial` being an Object means all the doctest tests are now
broken, as it cannot find the `godot::String` implementation pointers.
The tests crash currently because of this.

We do have tests in the godot demo project, using GUT.  They confirm
that `HexMapCellId.get_neighbors()` is properly working from GDScript.

I'm not sure which direction I'll go in the long term.

I'd like to be able to unit test in C++ and GDScript, but I don't want
to have to maintain a pile of pass-through proxy methods in the wrapper
class.  I made one suggestion[^2] in godot-cpp to allow binding methods
to class instance methods, but I don't even know if that's possible.

[^1]: godotengine/godot-cpp#1585
[^2]: godotengine/godot-cpp#1446 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom classes can't pass themselves into calls to other Godot APIs in their constructor
7 participants