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

Improve GDExtension tutorial based on own experience #10570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Maarrk
Copy link
Contributor

@Maarrk Maarrk commented Jan 29, 2025

  • Changed code sample to runtime class to match GDScript default.
    • Recorded a new animation showing the desired result inside game window.
  • Changed the wording around SConstruct file to make it less intimidating.
  • Added link to IDE setup in building from source documentation.
(original summary)

  • Clarified that there is a difference from _process in GDScript that took me by surprise
  • Changed the wording around SConstruct file to make it less intimidating.
  • Added some hints for usage with third party libraries:
  • Things that can be done with SCons to improve editor functionality, especially relevant due to lack of C++ documentation

@AThousandShips AThousandShips requested a review from a team January 29, 2025 10:48
@AThousandShips AThousandShips added area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:gdextension labels Jan 29, 2025
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.

First off: Thank you very much for your contribution!
The first two reworded/added section could be IMHO added with some more work. The last section should not be added due to it oversimplifying the process of integrating external libraries.

I personally have been already researching this topic for a while now and do have plans to write the documentation page for this. This would include how to set up external libraries with package managers like vcpkg and conan but also integrating one via git submodules. I have simple projects for these three cases working already but haven't had the time to write the page yet.

@Maarrk Maarrk force-pushed the pr-gdextension-tooling branch from 4cba951 to 75911f3 Compare January 29, 2025 16:22
@Maarrk Maarrk requested a review from paddy-exe January 29, 2025 16:24
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.

One more little nitpick and then I think it's ready to go

@Maarrk Maarrk force-pushed the pr-gdextension-tooling branch from 75911f3 to e7d802e Compare January 30, 2025 14:44
@Maarrk Maarrk requested a review from paddy-exe January 30, 2025 14:48
Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

Looks good. Ideally the GIF would be an webm video instead, if we're already taking the time to update it (see here for guidelines), but I don't consider that blocking.

@Maarrk
Copy link
Contributor Author

Maarrk commented Jan 31, 2025

Well, @tetrapod00 I'd say it's blocking now...

obraz

I don't know what to do with it, adding :width: 100% changed nothing

@Maarrk Maarrk force-pushed the pr-gdextension-tooling branch from e7d802e to 43363f6 Compare January 31, 2025 08:29
@Maarrk Maarrk marked this pull request as draft January 31, 2025 08:29
@Maarrk
Copy link
Contributor Author

Maarrk commented Jan 31, 2025

Marking as draft so it's not merged until we resolve the video width issue.

I think it's worth doing it because the gif was 2MB for 5 seconds, and webm is 300KB for 10 seconds. I think the static parts of the window compressed really well

@Maarrk Maarrk marked this pull request as ready for review January 31, 2025 19:10
@Maarrk
Copy link
Contributor Author

Maarrk commented Jan 31, 2025

Thanks for the alignment solution!

It's funny how the PR has almost completely different scope than what I started with, but I'm happy to help you get what's best according to people with experience.

I currently don't have access to the PC, so I can do the squash tomorrow evening. But if you have a convenient way to do it, feel free to merge :-)

Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

Built locally (using a variation on the command snippet from your other PR 😛) and the video renders fine (but left-aligned until the larger fix is merged).

Changed code sample to runtime class to match GDScript default.
Recorded a new animation showing the desired result inside game window.
Changed the wording around SConstruct file to make it less intimidating.
Added link to IDE setup in building from source documentation.

Co-authored-by: tetrapod <[email protected]>
@Maarrk Maarrk force-pushed the pr-gdextension-tooling branch from 89d5577 to a5db650 Compare February 1, 2025 20:35
@Maarrk
Copy link
Contributor Author

Maarrk commented Feb 1, 2025

Fresh from the oven, rebased, squashed and ready to merge! o7

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.

5 participants