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

Add example of GDExtension written pure C #8751

Closed
wants to merge 1 commit into from

Conversation

vnen
Copy link
Member

@vnen vnen commented Jan 11, 2024

This is the same code as the C++ example, but written without any bindings using the GDExtension API directly. It is somewhat long because it needs to touch a lot of aspects and create boilerplate that would normally come from the bindings.

I also want to point out that the GDExtension API is not meant to be used directly like this, so it is more of a reference for people who wants to create their own bindings.

Production edit: closes godotengine/godot-roadmap#77

@vnen vnen added enhancement content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:gdextension labels Jan 11, 2024
Copy link
Contributor

@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.

Wow! George, this is truly monumental work! Great job :-)

File structure
--------------

To organize our files, we're gonna split in mainly two folders:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To organize our files, we're gonna split in mainly two folders:
To organize our files, we're gonna split into mainly two folders:


Lastly, there's another source of information we need to refer to, which is the JSON
file with the Godot API reference. This file won't be used by the code directly, we
will only use to extract some information manually.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
will only use to extract some information manually.
will only use it to extract some information manually.


The following ``SConstruct`` file is a simple one that will build your extension
to the current platform that you are using, be it Linux, macOS, or Windows. This
will be a non-optimized build for debugging purposes. It also assumes a 64-bits
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
will be a non-optimized build for debugging purposes. It also assumes a 64-bits
will be a non-optimized build for debugging purposes. It also assumes a 64-bit

Initializing the extension
--------------------------

The first bit of code will be responsible to initialize the extension. This is
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The first bit of code will be responsible to initialize the extension. This is
The first bit of code will be responsible for initializing the extension. This is

needed. It also sets the initialization level which varies per extension. Since
we plan to add a custom node, the ``SCENE`` level is enough.

We will fill the ``initialize`` function later to register our custom class.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this would be easier to understand with the full name of the function in this example (as opposed to the field on the struct):

Suggested change
We will fill the ``initialize`` function later to register our custom class.
We will fill the ``initialize_gdexample_module`` function later to register our custom class.

Variant can cause crashes.

Then it proceeds to extract the argument using the constructor we setup before.
The one with no arguments instead set the return value after calling the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The one with no arguments instead set the return value after calling the
The one with no arguments instead sets the return value after calling the

}

Since this function is already being called by the initialization process, we
can stop here. This function is much straightforward after we created all the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
can stop here. This function is much straightforward after we created all the
can stop here. This function is much more straightforward after we created all the

bind_property("GDExample", "speed", GDEXTENSION_VARIANT_TYPE_FLOAT, "get_speed", "set_speed");
}

If you build the extension with ``SCons``, you'll see in the Godot editor the new property shown
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems odd upper case, because the command would be all lower case.

Suggested change
If you build the extension with ``SCons``, you'll see in the Godot editor the new property shown
If you build the extension with ``scons``, you'll see in the Godot editor the new property shown

For now it will do nothing but update the private field we created. We'll come
back to this after the method is properly bound.

Virtual methods are a bit different for the regular bindings. Instead of
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Virtual methods are a bit different for the regular bindings. Instead of
Virtual methods are a bit different from the regular bindings. Instead of

calling the functions (such as wrong arguments). For the sake of simplicity
we're not gonna handle that here.

At then end we need to destruct the Variants we created. While technically the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
At then end we need to destruct the Variants we created. While technically the
At the end we need to destruct the Variants we created. While technically the

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Will try do a grammar and language review in the next few days, but looks very interesting!

#include "init.h"

void initialize_gdexample_module(void *p_userdata, GDExtensionInitializationLevel p_level)
{
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest following the code style for this code as well, with brackets on the declaration

class, but it makes clearer to separate the concerns and let our class register
its own metadata.

The ``object`` field is necessary because our class will inherit a Godot class.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The ``object`` field is necessary because our class will inherit a Godot class.
The ``object`` field is necessary because our class will inherit a Godot class.

Copy link
Contributor

@paddy-exe paddy-exe left a comment

Choose a reason for hiding this comment

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

Left some minor comments on grammar and suggestions for improvements. Phenomenal work on this!


#endif // INIT_H

The functions declared here have the signature expected by the GDExtension API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The functions declared here have the signature expected by the GDExtension API.
The functions declared here has the expected signature by the GDExtension API.

Comment on lines +218 to +220
In order to make an actual node, first we'll create a C struct to hold data and
functions that will act as methods. The plan is to make this a custom node that
inherits from :ref:`Sprite2D <class_Sprite2D>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In order to make an actual node, first we'll create a C struct to hold data and
functions that will act as methods. The plan is to make this a custom node that
inherits from :ref:`Sprite2D <class_Sprite2D>`.
In order to make an actual Node, first we'll create a C struct to hold data and
functions that will act as methods. The plan is to make this a custom Node that
inherits from :ref:`Sprite2D <class_Sprite2D>`.

Comment on lines +252 to +254
The only things special here are the ``object`` field, which holds a pointer to
the Godot object, and the ``gdexample_bind_methods`` function, which will
register the metadata of our custom class (properties, methods, and signals).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The only things special here are the ``object`` field, which holds a pointer to
the Godot object, and the ``gdexample_bind_methods`` function, which will
register the metadata of our custom class (properties, methods, and signals).
Noteworthy here is the ``object`` field, which holds a pointer to
the Godot object, and the ``gdexample_bind_methods`` function, which will
register the metadata of our custom class (properties, methods, and signals).

reference to such objects when calling methods on the parent class, for
instance.

Let's create the source counterpart of this header. Make the file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Let's create the source counterpart of this header. Make the file
Let's create the source counterpart of this header. Create the file

Comment on lines +286 to +288
Very boring stuff as for now we don't have anything to do with those functions, so
they'll stay empty for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Very boring stuff as for now we don't have anything to do with those functions, so
they'll stay empty for a while.
As we don't have anything to do with those functions yet, they'll stay empty
for a while.

Comment on lines +1623 to +1625
In order to make our node do stuff, we'll need to call Godot methods. Not only
the GDExtension API functions as we've being doing so far, but actual engine
methods, as we would do with scripting. This naturally require some extra setup.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In order to make our node do stuff, we'll need to call Godot methods. Not only
the GDExtension API functions as we've being doing so far, but actual engine
methods, as we would do with scripting. This naturally require some extra setup.
In order to make our Node do stuff, we'll need to call Godot methods. Not only
the GDExtension API functions as we've being doing so far, but actual engine
methods, as we would do with scripting. This naturally requires some extra setup.

...
}

The only special thing here is the ``Vector2`` constructor, which we request the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The only special thing here is the ``Vector2`` constructor, which we request the
The only noteworthy part here is the ``Vector2`` constructor, in which we request the

name. This index can be retrieved from the ``extension_api.json`` file. Note we
also need a new local helper to get it.

Note that we don't get anything for the methods struct here. This is because
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note that we don't get anything for the methods struct here. This is because
Be aware that we don't get anything for the methods struct here. This is because

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of Note since that was just used for the prior sentence

destruct_property(&args_info[0]);
}

This one is very similar to the function to bind methods. The main difference is
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This one is very similar to the function to bind methods. The main difference is
This implementation is very similar to the function to bind methods. The main difference is

destructors.variant_destroy(&ret);
}

This helper has some boilerplate but is quite straightforward. It sets up the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This helper has some boilerplate but is quite straightforward. It sets up the
This helper function has some boilerplate code but is quite straightforward. It sets up the

Copy link
Contributor

Choose a reason for hiding this comment

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

quite straightforward should also be avoided as it sounds like it's trivial to understand. Perhaps just end the sentence after boilerplate

Copy link
Contributor

@31 31 left a comment

Choose a reason for hiding this comment

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

I've only skimmed it so far, but this looks amazing. Was just about to try to work through how to use these APIs on my own (based on what I think godot-cpp is doing), glad it took me long enough to get to it that I noticed this PR first. 😄

An overall wish I would have if it's feasible is a TOC (and/or multiple pages) so it's easier to understand the scope of what's being provided and poke around.

Comment on lines +1149 to +1150
The complete version is more involved. First, it creates ``String``s and
``StringName``s for the needed fields, by allocating memory and calling their
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the s is making rendering mess up? Looks like this on my local build (cherry-picked on top of a63a33a in master):

image

...
}

Here we create ``StringName``s for the class and method we want to get, then use
Copy link
Contributor

Choose a reason for hiding this comment

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

Another ``s rendering weird.

@vnen
Copy link
Member Author

vnen commented Jan 15, 2024

BTW, thanks for all the reviews. I did kind of forget this during weekend (on purpose to rest) but in the next few days I'll address all the issues pointed out.

void gdexample_call_virtual_with_data(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name, void *p_virtual_call_userdata, const GDExtensionConstTypePtr *p_args, GDExtensionTypePtr r_ret)
{
// If it is the "_process" method, call it with a helper.
if (is_string_name_equal(p_name, "_process"))
Copy link
Member

@rburing rburing Jan 19, 2024

Choose a reason for hiding this comment

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

This is re-creating the StringName "_process" every process frame, isn't it? It would be nice to avoid that here.

Edit: Comparing p_virtual_call_userdata to &gdexample_process would be fast.

@mhilbrunner
Copy link
Member

Closing as salvageable, as this has been gathering dust for quite a while and needs the feedback adressed before it can be merged.

Anyone feel free to take this, apply fixes for the feedback and open a new PR :)

@AThousandShips AThousandShips added archived and removed needs work Needs additional work by the original author, someone else or in another repo. labels Nov 18, 2024
@dsnopek
Copy link
Contributor

dsnopek commented Dec 12, 2024

I'd like to salvage this. I'll make a new PR soon.

@dsnopek
Copy link
Contributor

dsnopek commented Dec 12, 2024

PR #10403 is my remake of this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived area:manual Issues and PRs related to the Manual/Tutorials section of the documentation content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features enhancement topic:gdextension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants