-
-
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
Improve GDExtension tutorial based on own experience #10570
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.
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.
4cba951
to
75911f3
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.
One more little nitpick and then I think it's ready to go
75911f3
to
e7d802e
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. 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.
Well, @tetrapod00 I'd say it's blocking now... I don't know what to do with it, adding |
e7d802e
to
43363f6
Compare
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 |
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 :-) |
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.
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]>
89d5577
to
a5db650
Compare
Fresh from the oven, rebased, squashed and ready to merge! o7 |
(original summary)
_process
in GDScript that took me by surpriseSConstruct
file to make it less intimidating.