Update godot internal API to match godot-cpp API

[Ughuuu]

Describe the project you are working on

Godot Sandbox In-editor scripting and sandboxing for Godot and many other addons/modules

Describe the problem or limitation you are having in your project

Writting an addon using gdextension (godot-cpp) and building it also as a module (godot).

1. The functions don't match (eg. godot uses functions as they are, godot-cpp uses underscore) eg.:

virtual void get_script_property_list(List<PropertyInfo> *r_propertys) const // godot
//vs
virtual TypedArray<Dictionary> _get_script_property_list() const override; // godot-cpp

2. Some functions parameters don't match, godot has sometimes extra params:

virtual void get_script_property_list(List<PropertyInfo> *r_propertys) const
//vs
virtual TypedArray<Dictionary> _get_script_property_list() const override;

3. Some functions don't exist:

modules/sandbox/src/cpp/resource_saver_cpp.cpp: In static member function 'static void ResourceFormatSaverCPP::init()':
modules/sandbox/src/cpp/resource_saver_cpp.cpp:26:24: error: 'get_singleton' is not a member of 'ResourceSaver'
   26 |         ResourceSaver::get_singleton()->add_resource_format_saver(cpp_saver);

Here godot-cpp has the get_singleton function, godot doesn't. Would be nice to even duplicate that function so the code works same for both.

4. Some function names don't match:

modules/sandbox/src/rust/resource_saver_rust.cpp:87:59: error: 'class EditorInterface' has no member named 'get_resource_filesystem'; did you mean 'get_resource_file_system'?
   87 |                         EditorInterface::get_singleton()->get_resource_filesystem()->scan();
      |                                                           ^~~~~~~~~~~~~~~~~~~~~~~
      |                                                           get_resource_file_system

Here it seems that one function is caleld get_resource_file_system, one is called get_resource_filesystem. Would be nice that if they match.

NOTE: for all these cases, there are multiple cases, not just these individual ones. These are classes or problems.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Part 1. The includes

There is already proposal for making sure godot-cpp generates includes that point to both godot-cpp and godot: https://github.com/godotengine/godot-proposals/issues/9212. There is also a PR for it https://github.com/godotengine/godot-cpp/pull/1415 that adds a compatibility layer to godot-cpp:

eg.

#ifdef GODOT_MODULE
// The next included file is generated: if we had `godot_repo="..."` it'll contain the Godot includes, if not, it'll just be an empty file.
#include <godot_cpp/compat/HEADER.hpp>
#else

// ... the code that's there now ...

#endif

Part 2. Some macros

After this, the writter of the addon, needs to just update the using namespace godot; part and create a SCsub and can use the existing build systems for both gdextension and module builds.

Eg.

#include <godot_cpp/classes/resource_saver.hpp>
GODOT_NAMESPACE

class ResourceFormatSaverELF : public ResourceFormatSaver {
    GDCLASS(ResourceFormatSaverELF, ResourceFormatSaver);

protected:
    static void GODOT_CPP_FUNC (bind_methods)() {}

For the defines, they would look like:

#ifdef GODOT_MODULE
#define GODOT_CPP_FUNC(name) name
#define GODOT_NAMESPACE
#else
#define GODOT_CPP_FUNC(name) _##name
#define GODOT_NAMESPACE using namespace godot;
#endif

Part 3. Godot changes

As said above in the limitations chapter, some functions in godot either don't match, don't exist, have extra parameters or different names. Where it's possible to fix this by a macro, thats ok. Where no, it would be really nice if we change the function to match godot-cpp. I would even say we don't do that ugly macro and instead make the godot function (o underscore) also match the godot-cpp function naming.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Changin godot internal API (functions) to match godot-cpp one. (it would be incremental change).

Downsides of this proposal:

If people have modules, they need to update them. But if they do, they would in theory be able to migrate also to support addons too out of the box, so it would be a necesary change for those people. A big project that does this, supports both addon/module is godot voxel, and you can see there how many ifdefs there are.

If this enhancement will not be used often, can it be worked around with a few lines of script?

-

Is there a reason why this should be core and not an add-on in the asset library?

-

EDIT: Updated 1. with another function example, not bind_methods.

[AThousandShips]

1) The functions don't match (eg. godot uses functions as they are, godot-cpp uses underscore) eg.:

Godot itself does use underscores as well, the method is _bind_methods not bind_methods

2) Some functions parameters don't match, godot has sometimes extra params:

This is because of performance benefits of using a passed List as an argument, however passing data that way is not possible in GDScript or extensions so it can't be passed that way

Using the version that takes an argument in c++ is generally more performant as you don't need to create and pass data in the same way

[Ughuuu]

Updated 1. with another function example, not bind_methods. There are multiple like these also.

2. Not sure about the performance benefits, I will let @dsnopek chime in if he knows more on this. If there is no way for 2. to be fixed to match, then probably only way to make this match is some codegen on the input source or some ugly macros. Also, while on this, if it is a performance benefit or not, that is one thing, but it might also matter if it is a hot path or not. (Eg update function that gets called every ms, or if it is just called once). Also, there are a lot of addons that might be ported to be both modules, and would benefit the whole godot community as a whole. This kind of change might make it much much easier for those to be ported.

[AThousandShips]

The new example for 1 isn't the same method though, the first is a public method the second is a private internal bind, the former calls the latter, it's like get and _get

[Ughuuu]

I see, I just know that generally in code we do this to mitigate this issue in order to support both and no . Not 100% sure of the internals of it:

If there are different cases for them, I can create another bullet point (for public and private methods). This proposal is just a a discussion one to see if/what can be changed in godot to match the functions so the work is less for people doing ports to module/addon.

[AThousandShips]

The difference is that the one without the underscore should be called, and the one without shouldn't, except internally, like how you don't call _get in GDScript, there's also the internal methods that are binds when the interface differs, where one returns Vector<T> in the engine but TypedArray<T> for binds, and the bound one has a prefix, and shouldn't be called in the engine

[dsnopek]

@Ughuuu:

virtual void get_script_property_list(List<PropertyInfo> *r_propertys) const // godot
//vs
virtual TypedArray<Dictionary> _get_script_property_list() const override; // godot-cpp

I'd personally be for changing the signature (the arguments and return value) of these sort of virtual methods to match godot-cpp. (The underscore thing is a little trickier, because in some cases the non-underscore version is a non-virtual C++ function that may call the Godot virtual method, and so we need to have two separate names to distinguish them.)

@AThousandShips:

3. Some functions parameters don't match, godot has sometimes extra params:

This is because of performance benefits of using a passed List as an argument, however passing data that way is not possible in GDScript or extensions so it can't be passed that way

Using the version that takes an argument in c++ is generally more performant as you don't need to create and pass data in the same way

I'm not 100% sure there would be a performance issue. In the "olden days", I think you would definitely be right: using the linked list passed by reference means we're just allocating a new item each time we add one, without any re-allocation or copying. But with modern CPUs, I suspect it would be a wash overall: re-allocating the array each time it was expanded would be somewhat slower, but we would gain when looping over the array because they'd be in contiguous memory.

I think the only way to really know would be to try it and measure.

---

The rest of things in the proposal I'm less convinced about: I think the macros and includes could be handled on the godot-cpp side.

However, there are some other naming mismatches that I think we should fix on the Godot side, for example, these PRs from @aaronfranke:

• https://github.com/godotengine/godot/pull/98039
• https://github.com/godotengine/godot/pull/98132

In general, I think we've got to look at each category of API mismatch on a case-by-case basis.

[AThousandShips]

For performance you have a lot of overhead with typed arrays, because of checking and Variant and storage etc.

[dsnopek]

Ah, and I forgot we're actually using Dictionary for the PropertyInfo ones too... :-/

We could maybe do more on the godot-cpp side to normalize those? Like, we could hand-write the virtual functions that take List<PropertyInfo> & and automatically convert them to TypedArray<Dictionary> before returning the data to Godot?

[aaronfranke]

For the problem of Dictionary vs PropertyInfo, can we allow the Dictionary version (that starts with an underscore) to be used from both modules and GDExtension, while the PropertyInfo version (no underscore prefix) can be kept as internal-only? This way it's possible to use the Dictionary version in both, even if it's a bit slower than it otherwise could be.