-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Split C++ (godot-cpp) and GDExtension system info into separate categories, children of Scripting. #10631
base: master
Are you sure you want to change the base?
Conversation
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.
To deal with the build errors:
/home/runner/work/godot-docs/godot-docs/about/docs_changelog.rst:348: WARNING: undefined label: 'doc_what_is_gdextension' [ref.ref]
/home/runner/work/godot-docs/godot-docs/about/faq.rst:193: WARNING: undefined label: 'doc_what_is_gdextension' [ref.ref]
...
/home/runner/work/godot-docs/godot-docs/classes/class_gdextension.rst:31: WARNING: unknown document: '../tutorials/scripting/gdextension/what_is_gdextension' [ref.doc]
/home/runner/work/godot-docs/godot-docs/classes/class_gdextension.rst:33: WARNING: unknown document: '../tutorials/scripting/gdextension/gdextension_cpp_example' [ref.doc]
/home/runner/work/godot-docs/godot-docs/classes/class_gdextensionmanager.rst:31: WARNING: unknown document: '../tutorials/scripting/gdextension/what_is_gdextension' [ref.doc]
/home/runner/work/godot-docs/godot-docs/classes/class_gdextensionmanager.rst:33: WARNING: unknown document: '../tutorials/scripting/gdextension/gdextension_cpp_example' [ref.doc]
This anchor _doc_what_is_gdextension
is referenced from three places:
- https://docs.godotengine.org/en/latest/about/faq.html#which-programming-language-is-fastest
- https://docs.godotengine.org/en/latest/tutorials/scripting/gdextension/what_is_gdextension.html#doc-what-is-gdextension
- https://docs.godotengine.org/en/stable/tutorials/export/exporting_for_web.html#thread-and-extension-support
And it's referenced as a URL from a few places in the class reference.
I would recommend:
- Remaking a page
tutorials/scripting/gdextension/what_is_gdextension.rst
- Add
what_is_gdextension
back to thegdextension/index.rst
table of contents - Give the new page these contents:
.. _doc_what_is_gdextension:
What is GDExtension?
====================
Placeholder.
That should resolve the CI errors for now while we figure out exactly how some of the content gets shuffled around or rewritten. It's possible that we want to keep that What is GDExtension
page in some form.
To be clear I recommend fixing the CI issue now but you don't need to start working on the overall rewrite (if needed) until GDExtension maintainers comment
research options thoroughly before starting a project with one of those. | ||
Also, double-check whether the binding is compatible with the Godot version | ||
you're using. | ||
|
||
.. _doc_what_is_gdextension_version_compatibility: | ||
|
||
Version compatibility |
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.
This section may now be out of place, it talks about GDExtension overall rather than godot-cpp specifically.
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.
This section is important to people who'd consider using godot-cpp
: They need to know how compatible their extension will be across versions. It needs to be discussed here, or at least linked to another place where it is discussed.
It is as important to people who'd consider making their own GDExtension from scratch, so it might be warranted documenting it there too. But I'd argue that these people are more familiar with GDExtension anyway and don't need to be told this again explicitly.
@@ -0,0 +1,33 @@ | |||
Other Languages |
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.
It's possible we want to fold this page into the The GDExtension system
page, maybe called Other languages with GDExtension
. But I do see the appeal of listing "other languages" explicitly in the table of contents.
If we keep it, we don't actually use Title Case for titles in the sidebar:
Other Languages | |
Other languages |
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.
I think it's more important for people to find this place out of the use-case of "What language can I use with Godot?" rather than "What bindings exist using GDExtension?", for the same reason we're moving C++ (godot-cpp)
out of GDextension in the first place.
(will adjust the title case)
068bda1
to
13b4515
Compare
I made quite a few changes. Hopefully the CI issues are resolved / better now. But changes are accumulating :D
Here's the current status (parts at least): Looks a lot better still, in my opinion! |
fbea559
to
ac0584d
Compare
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.
Thanks, this is looking really good to me!
I have just a couple notes on some of the new text.
ac0584d
to
d2a085e
Compare
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.
Looks good. I didn't reply to all your replies individually, you made a lot of good points and I trust your overall judgement here, I was mostly calling out things that may need changes. I left some of the conversations unresolved so other reviewers can see them.
There are a couple more docs syntax/conventions things to address, plus the question of what to do about What is GDExtension
and the links from the class reference.
Generally speaking, if you build a custom version of Godot, you should generate an | ||
``extension_api.json`` from it for your GDExtensions, because it may have some differences | ||
from official Godot builds. | ||
Placeholder. |
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.
One more thing: do we want to keep some form of a What is GDExtension
page?
I initially suggested it cause it was the cleanest way to resolve the CI errors.
If we decide to remove:
- I believe we need a PR for the class reference to change the links:
- If we're doing that, we need to find a new link for the general GDExtension link.
- And we might as well properly rename the
getting_started.rst
page if we're already making engine PRs that are changing URLs.
If we decide to keep it, then we need some content, and I'm not sure what.
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.
It definitely makes sense to me to have some page explaining GDExtensions in concept! I would have initially put it into the GDExtension nav page, but since this does not seem to be the way of this wiki, I'm happy to have a separate page for it.
I think I may be able to magic up some contents for it for this PR too, without being too lengthy or controversial. The rest can be added at a later date, then.
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.
A good tool here might be the new What is GDExtension
page linking to the About godot-cpp
page which used to be the What is GDExtension
page. In the absolute worst case we could leave it as a short stub page as a "redirect", we do this in a few other places in the docs already.
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.
Alright, I've written a bit of boilerplate / some initial useful info. Can definitely be expanded, but it's not empty. Let me know if you think it needs more info!
bf69758
to
119f39b
Compare
@tetrapod00 I think I've addressed all the discussions that have been put up so far. CI is still failing (even after renaming the files back to what they were) because they're found under different paths now. I suppose I could move them back to the |
Oh, I missed that To be honest I'm not actually sure exactly what the right protocol is when we need to move a page that is linked from the class reference, so would need a class reference change. CC @mhilbrunner? |
I built locally (after fixing errors by temporarily changing the links in I think the new short What is GDExtension page is good, serves the role of a landing page for the old link, and can be expanded in the future if needed. Needs a GDExtension team review. This looks good overall. What's left from my point of view:
Great work, thanks for being so responsive to reviews. I wish that moving pages was as simple as your first proposed PR. This process also exposed some places where we should probably document how Sphinx and RST work a little better, along with the process for moving and renaming pages. |
…ories, children of Scripting.
I can only return that, you've been incredibly helpful in pushing this change along. Thank you!
Good idea, will do! |
119f39b
to
a2deb99
Compare
This is an alternative to #10617.
Please don't discuss this PR here, discuss the options in #10617 instead (apart from change reviews).
This PR specifically rearranges articles such that
Scripting
now has these children:C++ (godot-cpp)
andThe GDExtension System
are split off from one another because they cater to different target groups (godot-cpp for C++ users, GDExtension to language bindings implementers).Other Languages is taken from the previous GDExtension article, and is targeted at people that want to code in other languages.
I kept the changes as minimal as possible, but rearranged some articles and added intros to make it flow better with the new structure.