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

GDExtension: Add Engine method to allow registering class reference data #75415

Closed

Conversation

akien-mga
Copy link
Member

This can be used from a GDExtension's init callback to add zlib-compressed class reference XML data for use in the editor help.

This is a proof of concept. I'm not very familiar with GDExtension yet and there might be better ways to implement this.

Adding this data to Engine is a bit arbitrary, especially given that it's for the editor only, but I'm not sure what's a better location. We often store this in EditorNode but it's not available from GDExtension. There's EditorInterface available through EditorPlugin but that sounds like it would require a bit more boilerplate to set up just to pass some data.

I also had to use PackedByteArray to pass data through GDExtension, so it's fairly inefficient to write the data to a char buffer, copy it to a PackedByteArray to pass, and then it ends up being accessed back as a char buffer.

Here's a branch on my Threen extension where I'm using this PR to expose the docs. It requires some extra scons tooling copied from this repo to compress the XML and write it to a header, we can add it to godot-cpp's tools folder so it's available for all projects with a convenient API. https://github.com/akien-mga/threen/compare/classref

I'm a bit out of my comfort zone here, happy to get suggestions / diffs / PRs to this branch / superseding PRs.

@akien-mga akien-mga added this to the 4.1 milestone Mar 28, 2023
@akien-mga akien-mga requested review from bruvzg and reduz March 28, 2023 09:41
@akien-mga akien-mga changed the title GDExtension: Add Engine method to allow register class reference data GDExtension: Add Engine method to allow registering class reference data Mar 28, 2023
@@ -75,6 +81,8 @@ class Engine {
List<Singleton> singletons;
HashMap<StringName, Object *> singleton_ptrs;

Vector<ExtensionClassDoc> extensions_class_doc;
Copy link
Member Author

Choose a reason for hiding this comment

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

Could also be List or LocalVector. As it's passed around via a getter I figured Vector might be correct but I'm no expert here.

core/config/engine.cpp Outdated Show resolved Hide resolved
editor/editor_help.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga force-pushed the gdextension-class-doc branch 6 times, most recently from af9f296 to f3cec78 Compare March 28, 2023 13:03
@RedworkDE
Copy link
Member

IMO the API design of passing a zlib compressed, utf8 encoded xml string is kinda weird. I find simply passing a string would be much more natural of an API, esp from a language where embedding a blob like this isn't very natural or easy.

@akien-mga
Copy link
Member Author

I agree, but this data is included in your library. Compressing it helps reduce binary size.
It could be uncompressed on the extension side before passing it to the engine, but since the engine currently expects compressed data, I went for the simpler option.

@mhilbrunner
Copy link
Member

Could we ship a macro that does the compression at compile time?

@akien-mga
Copy link
Member Author

Yes, the logic from https://github.com/akien-mga/threen/compare/classref could be added as a SCons tool in godot-cpp.

Ideally users should just need to list their source XML files and the path for the generated header, and the rest should be handled by the SCons tool. I'll just need some help to set this us properly, handling Builder/Command/Depends stuff with SCons is always a bit arcane to me.

@Riteo
Copy link
Contributor

Riteo commented May 9, 2023

Whats missing from this PR Godot-side? Looks pretty straight forward for being a PoC.

@dsnopek
Copy link
Contributor

dsnopek commented May 10, 2023

This is on the list to discuss at the next GDExtension meeting!

@Riteo
Copy link
Contributor

Riteo commented May 14, 2023

I couldn't sadly participate to the meeting. So, what are now the plans for this PR?

@dsnopek
Copy link
Contributor

dsnopek commented May 14, 2023

Unfortunately, we only made it about half way through our list of PRs and didn't get to this one. Since the Godot 4.1 feature freeze is coming up we're going to try to have another meeting in one week instead of two

@dsnopek
Copy link
Contributor

dsnopek commented May 18, 2023

IMO the API design of passing a zlib compressed, utf8 encoded xml string is kinda weird. I find simply passing a string would be much more natural of an API, esp from a language where embedding a blob like this isn't very natural or easy.

I agree, but this data is included in your library. Compressing it helps reduce binary size.
It could be uncompressed on the extension side before passing it to the engine, but since the engine currently expects compressed data, I went for the simpler option.

I think it could make sense to have two methods: one that takes compressed data, and another that takes uncompressed data?

For compiled languages, storing the XML docs compressed is natural and fairly easy to do - it just becomes part of the build process. But lets say you're making some reusable Node types via the Python bindings: it'd be much more natural to load the data from an uncompressed file, and send that to Godot.

Of course, rather than having both, we could have just the method that accepts uncompressed data, and it'd be up to the compiled languages to compress and decompress the data themselves. However, I think having both would fine because:

  1. We're always going to have a method in Godot itself that takes the compressed data, because Godot needs it for its own built-in docs, and
  2. The method that takes the compressed data can just call the one that takes uncompressed data, ie. maintaining two methods has really very little overhead

But I expect we'll discuss this one at the GDExtension meeting tomorrow, and I'm curious to see what the rest of the team thinks :-)

@dsnopek
Copy link
Contributor

dsnopek commented May 18, 2023

Adding this data to Engine is a bit arbitrary, especially given that it's for the editor only, but I'm not sure what's a better location. We often store this in EditorNode but it's not available from GDExtension. There's EditorInterface available through EditorPlugin but that sounds like it would require a bit more boilerplate to set up just to pass some data.

I also had to use PackedByteArray to pass data through GDExtension, so it's fairly inefficient to write the data to a char buffer, copy it to a PackedByteArray to pass, and then it ends up being accessed back as a char buffer.

We could also add a new function to the low-level GDExtension interface? That would allow passing a char * rather than having to allocate the PackedByteArray. It also gets the function off of Engine, which isn't the most natural place for it, and prevents folks from calling this from GDScript (which we probably don't want people to do?)

I'm not totally sure about this, though, because we don't want to add too many functions to the low-level GDExtension interface, either...

@dsnopek
Copy link
Contributor

dsnopek commented May 19, 2023

Discussed at the GDExtension meeting, and we think it makes the most sense for the API to take uncompressed data, since how Godot handles this compressed data is an internal detail (which could maybe change in the future if Godot decides to compress its data differently). If godot-cpp or other compiled bindings want to compress the data, they can still do that on their end.

We also think this would be better as part of the GDExtension interface (rather than exposed via ClassDB) because we don't think it makes sense for GDScript to be able to access it.

There are a few options for how to add this to the GDExtension interface:

  • A new function that takes a pointer to the XML data (working basically the same as the new Engine method in this PR)
  • A new function that takes a pointer to a function that will be called by EditorHelp::_gen_doc_thread() to get the XML data

@Riteo
Copy link
Contributor

Riteo commented May 19, 2023

@dsnopek sadly I couldn't attend.

Does the engine recompress the stuff it gets or does it keep it uncompressed in memory?

@dsnopek
Copy link
Contributor

dsnopek commented May 19, 2023

Does the engine recompress the stuff it gets or does it keep it uncompressed in memory?

No, it doesn't get recompressed, it remains uncompressed in memory.

The whole compression thing comes from Godot keeping the builtin docs compressed in the binary to save space - on startup it decompresses them, parses it and ends up creating various objects that hang around in memory until the editor is closed.

@Riteo
Copy link
Contributor

Riteo commented May 19, 2023

@dsnopek

No, it doesn't get recompressed, it remains uncompressed in memory.

All right. Then it makes more sense to send the uncompressed data directly to the engine. As you said, it's both a pretty unimportant implementation detail and a thing that plugins have the freedom to handle however they please. Count me in on the idea of an uncompressed API.

@akien-mga
Copy link
Member Author

Sounds good to me. I won't have much time work on this in the coming weeks so any interested contributor would be welcome to take this over and make a new PR that supersedes mine. Otherwise I'll get back to it whenever I have more bandwidth.

@YuriSizov YuriSizov modified the milestones: 4.1, 4.x Jun 14, 2023
@akien-mga akien-mga added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release and removed cherrypick:4.0 labels Jun 29, 2023
This can be used from a GDExtension's init callback to add zlib-compressed
class reference XML data for use in the editor help.
@akien-mga akien-mga force-pushed the gdextension-class-doc branch from f3cec78 to 4b676a0 Compare June 29, 2023 11:35
alessandrofama added a commit to alessandrofama/wwise-godot-integration that referenced this pull request Aug 24, 2023
While the broader in-engine documentation effort is ongoing (godotengine/godot#75415), this commit includes the required documentation files for both the Wwise and WAAPI interfaces. The documentation will be integrated once the feature becomes available.
alessandrofama added a commit to alessandrofama/wwise-godot-integration that referenced this pull request Aug 30, 2023
While the broader in-engine documentation effort is ongoing (godotengine/godot#75415), this commit includes the required documentation files for both the Wwise and WAAPI interfaces. The documentation will be integrated once the feature becomes available.
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
@akien-mga
Copy link
Member Author

Suoerseded by #83747.

@akien-mga akien-mga closed this Feb 6, 2024
@akien-mga akien-mga deleted the gdextension-class-doc branch February 6, 2024 06:52
@AThousandShips AThousandShips removed this from the 4.x milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants