Skip to content

Add GDExtension C++ snippets to tutorials/best_practices #9837

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

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

dementive
Copy link
Contributor

C++ documentation for tutorials/best_practices. I mostly have this done but it definitely needs some feedback/review/testing.

General question about the rest of the cpp documentation going forward: Should I add the GDCLASS, empty constructor, and _bind_methods to every class?

These are technically required for the C++ code to compile. However, adding these makes it harder for the reader to get the point the documentation is actually trying to make because they have to parse through a lot more boilerplate code. So im not sure it is actually useful to include in snippets outside of the initial tutorial documentation.

Progress

  1. scene_organization.rst - Done

  2. scenes_versus_scripts.rst - Decided writing these snippets in C++ isn't really relevant. GDExtension doesn't really have a concept of "scripts" since everything is an actual Node/Object.

  3. godot_interfaces.rst - TODO

  4. godot_notifications.rst - Done, some of the code in the first block isn't correct. Might want to check how I did the _unhandled_input to make sure it is ideal, this is how I've been doing it in my code but not sure if it is necessarily the "correct" way to do it even though it does work.

  5. data_preferences.rst - This isn't really entirely relevant to C++ either. Should probably be a separate GDExtension article for this that explains all the data structures in godot-cpp/templates and this article should link to that new article. Since a majority of the time (at least in my code) you are using the template data structures instead of the Variant one's unless you are exporting the code to gdscript.

  6. logic_preferences.rst - Done

  7. autoloads_versus_internal_nodes.rst - There should probably be a note that GDExtension actually does use singletons via Engine::get_singleton()->register_singleton. Maybe a new GDExtension example page on how to create singletons because its pretty common.

I can get the new singleton and data structures articles started if we think they would be good to add, but might want to do it in a separate PR idk what would be best.

@AThousandShips AThousandShips added area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:gdextension labels Aug 30, 2024
@dementive
Copy link
Contributor Author

dementive commented Sep 24, 2024

I think this should be all good to merge now on my end unless someone finds more problems with it in review. I was planning to do godot_interfaces.rst but lost motivation to chug through the example snippets by myself.

Same with #9821. I believe they should both be ready to merge once reviewed.

@AThousandShips
Copy link
Member

Will go over this again today

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.

Can't currently speak to the correctness, but this should be the final pass of formatting

@dsnopek
Copy link
Contributor

dsnopek commented Oct 5, 2024

Thanks, these changes seem good to me!

However, this'll need to be squashed to a single commit in order to be merged. See https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

@dementive
Copy link
Contributor Author

I rebased/squashed all the commits from this PR and did the same for #9821, both PRs should both be good to go now.

@mhilbrunner mhilbrunner merged commit d0d797c into godotengine:master Nov 4, 2024
1 check passed
@mhilbrunner
Copy link
Member

Merged. Thanks and congrats on your first merged contribution! Amazing work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:gdextension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants