-
-
Notifications
You must be signed in to change notification settings - Fork 23.8k
GDExtension: Store source of gdextension_interface.h in JSON
#107845
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
GDExtension: Store source of gdextension_interface.h in JSON
#107845
Conversation
fd610ba to
7abb091
Compare
|
Somehow, I completely missed this huge change! 😅 For completeness, I'd also like to mention potential downsides. Some are inherent to this sort of migration and not a flaw of the chosen approach, so they're not necessarily arguments against moving to JSON 🙂 long-term, this will likely simplify a lot!
I wonder if it would be possible to convert header files of older Godot versions to the JSON format and make them available? This would only need to happen for each minor version (as the GDExtension header doesn't change in-between), and would completely solve point (4). I'll look into the detailed changes at a later point. Just one request: could we keep the field names and structure as close as possible to the
|
|
Another question: if the JSON becomes now the source-of-truth, how do contributors modify it? If manually, is there tooling to ensure indentation, order of keys etc., to keep some consistency? |
|
@Bromeon Thanks, those are some very good points :-)
I think it makes sense to always keep the option of generating the C header. So, for language bindings that use premade tooling that can parse a C header, they can keep using that. But with the JSON, they can also do their own additional code generation, without having to make their own parser for the C header.
I agree that dealing with the whole thing at once is kind of hard to read. But I think with individual PRs that only add a couple things at a time, it should be fine. And, it'll also make it harder for certain errors to slip in, especially if we add some validation of the JSON, which you also bring up, and I agree would be a good idea.
Actually, bindings can always use the latest version of the JSON! (And this is true of the current We never change or remove old signatures. You just need to ignore any interface functions with a "since" field that's newer than the Godot version you are targeting. So, a binding could use the JSON from Godot 4.6 to build the Godot 4.1-4.5 compatible versions as well.
That makes sense! I'll update the PR when I have a chance.
Yes, manually. We could (and should!) add some tooling for formatting and validation. I think this would help to prevent the sort of errors and inconsistencies that have already cropped up in the I'll dig into this |
I just want to mention that |
I agree that JSON isn't the best format for human editing. (If I recall correctly, I think @vnen was advocating for using XML for this data at a meeting in the past?) I went for JSON largely because (a) parsing it in Godot is very easy and (b) developers already need to work with the I suppose we could keep the data in some other format (yaml, toml, xml, whatever) and then convert it to JSON to distribute it? That's a lot of converting, though |
Very good point. I was actually considering this at some point for the
Yes, or non-C code creeping in 🙂 #96408
It would make sense if both Personal preference:
I'm not sure if it's even an option to change the format for now, but for a potential Godot 5, I could totally imagine something like TOML. |
7abb091 to
2070c09
Compare
In my latest push, I've attempted to align the structure with
I've decided to go with "kind" (so, we have "a kind of type", which is literally what I would say out loud in normal conversation). And I've also renamed "simple" to "alias", which I think is more descriptive of what it is
I added some simple validation for types, so it shouldn't be possible to accidentally sneak a There's some more validation I'd like to add, including around formatting, which'll come in my next update! |
|
For the C# bindings we have no plans to move to the new JSON, so as long as the C header is still available it doesn't really affect us what the source of truth is. In the C# bindings we use ClangSharp which can generate C# bindings from the C header. It uses Clang directly, so as long as the C header is valid C/C++ code it should work fine. For other languages, keep in mind they may not have great support for TOML, whereas JSON and XML tend to be ubiquitous. It makes sense to use JSON since they'll need to support it for But as mentioned earlier, I don't plan to consume this new format, so I don't have a vested interest in the choice of format. |
|
I don't remember when I suggested XML as a format. It was probably because of the class ref docs, so we could use a similar format. But we usually rely on doctool creating the stuff for us, writing by hand is not really great. Not that I think JSON is much better (at least compared writing plain C code), but since all the tooling is based on JSON already, it makes sense to keep in the same format. Speaking of which, it might be interesting to keep a JSON Schema too, which also helps with validation (though I'm not sure how easy it would be to integrate a schema validator in our system). |
I think making a JSON Schema is a great idea, even if it only ends up as documentation Since we already have Python code in SCons parsing the JSON and looping over it, it may be easier just to validate its structure in there, rather than adding JSON schema validation tooling. But I'll take a look at it! If the schema validator can run as part of the pre-commit hooks, then we can check its format before SCons even runs |
2070c09 to
41deff5
Compare
41deff5 to
cf5c4d2
Compare
cf5c4d2 to
a17b27f
Compare
|
+1 on using JSON as format, since it is already used for
In a different project, I use jsonschema which works very well. I also think having a JSON schema is very good, both for static checking AND to provide some documentation on the expected format (especially useful when dealing with a huge file such as
|
|
For reference: godotengine/godot-proposals#9524 |
| "name": "create_instance_func", | ||
| "type": "GDExtensionClassCreateInstance", | ||
| "description": [ | ||
| "(Default) constructor; mandatory. If the class is not instantiable, consider making it virtual or abstract." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this field is mandatory, would it be useful to add something like a "mandatory" bool so that extensions can handle it differently? For example, when initializing the info struct, this field could exclude a default value to make it obvious to the extension user that this function needs to be provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps! I guess this would serve the same purpose as marking arguments and return values as required in #86079 and languages which have a distinction between nullable and non-nullable could use this information in generating their stubs to interact with the GDExtension interface.
However, I think this is something we could add later in a subsequent PR if GDExtension binding implementors ask for it. This PR is disruptive enough as it is, so I think it makes sense to try and keep the result as simple as possible.
a17b27f to
6a71e66
Compare
6a71e66 to
4e89984
Compare
|
I've just rebased this, and posted unfinished follow-up PR #112125, which shows something I'd like to do with this functionality once it's available Looking back over the comments here, it seems like the main unaddressed piece of review is that it would be good to have a JSON schema for |
4e89984 to
95992b5
Compare
|
I've added a JSON schema for |
95992b5 to
cec3fad
Compare
cec3fad to
2c68179
Compare
akien-mga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks fine to me.
This was discussed several times in GDExtension meetings and chat and this seems to be the direction contributors were in favor of.
|
Thanks! |
|
godot/core/extension/libgodot.h Line 33 in bbe9654
|
|
Ah, good catch, thanks! I'll make a PR in a little bit |
|
Here's the PR: #113126 |
Implements godotengine/godot-proposals#9560
This PR converts the canonical source of truth for the GDExtension interface from the
gdextension_interface.hheader, to a new JSON file (gdextension_interface.json).The header can still be generated from the JSON, and
godot --dump-gdextension-interfacewill still create a header. (This will be the "legacy header" - more on that below.)But now there's a new
godot --dump-gdextension-interface-jsonthat will dump the JSON data.(When we eventually do Godot 5 and can break compat, I'd like to make
--dump-gdextension-interfaceoutput the JSON, and a new--dump-gdextension-interface-headerto do the header. This will make it clear the JSON is the canonical source, but probably lots of folks will still want to use the header.)Advantages
This is something we've talked about at GDExtension team meetings for a long time (years!), which has a number of advantages:
There are already bindings that parse the
gdextension_interface.hto power their own code generation. They will no longer need to deal with the complexities of parsing C! They can just use the JSON data. (FYI, I initially made the JSON in this PR with a script that extracted the data from the header into JSON, and now no one needs to do that ever again :-))We can iterate on the header used within Godot and godot-cpp. Another change we've discussed in the past is replacing some of the
typedef void *s withstruct {}s so that the compiler can help prevent us from mixing up types, but we've been unable to do that, because we don't want to break developers using the "legacy header". Well, now Godot and godot-cpp can generate a modified header from the JSON to use internally, without affecting external users of the legacy headerUp until now we've had to enforce restrictions on what can go into
gdextension_interface.hand its format because we know some bindings parse it. This has meant trying to catch PRs that attempt to add "real code" to the header in review. We won't have to worry about that anymore! Anything added to the JSON will automatically be added to the "legacy header" in the correct format.Result
You can compare the new header generated by Godot, with the old header - or look at this convenient diff!
All the changes are cosmetic, mostly related to spacing or comment type. (I decided to standardize on C-style comments, and putting comments before what they comment, rather than inline.)
I've run the godot-cpp tests with the legacy header generated by Godot, and everything seems to work fine!
Notes
Note 1: Generating the header
There are two places in the PR that take the JSON and generate a C header: in Python code run by SCons to make the header used by Godot, and in Godot itself to generate the "legacy header".
This is intentional!
Right now, they do basically the same thing (with the exception of some type-o's in type names maintained in the legacy header, but fixed in the Python version).
But, as explained above, the goal is that we can continue to iterate on the Python version, so that we can make some improvements to the header used by Godot and godot-cpp. Whereas the header generation code in Godot will stay the roughly same for backwards compatibility.
Note 2: The docs
This PR doesn't attempt to improve the docs at all! It just transfers what we already have into JSON.
Since this PR will conflict with any other changes to
gdextension_interface.h, it runs the risk of stalling any other GDExtension work that needs to touch that file.So, while there may be a temptation to iterate on the docs in this PR, instead, I think we should attempt to merge it with the existing docs as soon as possible in the Godot 4.6 dev cycle. The goal is for the docs to be no worse than the docs we already have.
Afterwards, we can make follow-up PRs to improve the docs.
I'd like for review of this PR to focus on the format of the JSON file, and correctly generating the header.