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.

Sign up for GitHub

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

Jump to bottom

Update using_multiple_threads.rst #8752

Merged
merged 2 commits into from
Nov 19, 2024
Merged

Update using_multiple_threads.rst #8752

merged 2 commits into from
Nov 19, 2024

Conversation

i-snyder
Copy link
Contributor

This is my first attempt at updating the documentation, so I apologize for any mistakes. I'm starting with adding a C++ demo to for the threading example. It doesn't mirror the GDScript exactly, as I found this to be a clearer example of what the thread was doing when testing with multiple threads. All credit to "coder" from this Godot Forum thread: https://forum.godotengine.org/t/gdextension-c-async/36756/7?u=i-snyder

@AThousandShips
Copy link
Member

You should target master not 4.2, changes can then be cherry picked, see here.

If you need more information about the update process please see: here. If wanted I can fix the branch for you.

@AThousandShips AThousandShips added the needs work Needs additional work by the original author, someone else or in another repo. label Jan 11, 2024
@i-snyder
Copy link
Contributor Author

I'm sorry I missed that this wasn't pulled from master already -- if you don't mind fixing it for me this time just to make sure I don't mess it up I'd appreciate it. I will make sure to do that next time.

@AThousandShips
Copy link
Member

Will do!

@AThousandShips AThousandShips changed the base branch from 4.2 to master January 11, 2024 18:41
AThousandShips
AThousandShips reviewed Jan 11, 2024
View reviewed changes
@AThousandShips AThousandShips added area:manual Issues and PRs related to the Manual/Tutorials section of the documentation and removed needs work Needs additional work by the original author, someone else or in another repo. labels Jan 11, 2024
@i-snyder
Copy link
Contributor Author

You are awesome, thank you so much! I'm sorry, I read several of the guides on naming conventions etc but I'll do better next time.

@AThousandShips
Copy link
Member

It's okay! It takes time to learn, it took me ages to get it right myself

@i-snyder
Copy link
Contributor Author

By the way, do I need to "Resolve conversation" on all of those or will those close up when the merge happens?

@AThousandShips
Copy link
Member

You go ahead and do it when you feel done with them, it's not required but communicates, doesn't happen automatically

@AThousandShips
Copy link
Member

I'd suggest waiting to resolve them until you've pushed to make sure you've fixed things, you need to apply the changes, it won't happen automatically

@i-snyder
Copy link
Contributor Author

I'm so sorry, this is my first time with this process in the browser, I thought I was accepting your suggestions 🤦‍♂️I also just noticed the "add to batch" option until the last resolution 😛 I believe I have now updated it all with your suggestions, please let me know if there's anything else I missed. Thanks so much for your patience with me!

@AThousandShips
Copy link
Member

Will take a new look over the code but please squash your commits into one, if you can, see here.

@AThousandShips
Copy link
Member

Otherwise I'd say the other examples should have their own extension code, it feels incomplete to just have this one be expanded on

@AThousandShips AThousandShips requested a review from a team January 13, 2024 17:32
@i-snyder
Copy link
Contributor Author

Thanks so much, this is great! I'm actually just about to finish up the other examples for this page, so I'll add those to my next commit with all of your corrections 💪

@i-snyder
Copy link
Contributor Author

I'll be adding the other examples sometime today or tomorrow, so you can hold off reviewing until I do that commit. Thanks!

@i-snyder
Copy link
Contributor Author

Ok, all cpp demos have been added. Note that on these lines:
case NOTIFICATION_EXIT_TREE, NOTIFICATION_WM_CLOSE_REQUEST: { // Thread must be disposed (or "joined"), for portability.

I've added the case NOTIFICATION_WM_CLOSE_REQUEST so it will show output when the window is closed during testing in the Editor.

@i-snyder i-snyder force-pushed the patch-1 branch 2 times, most recently from 08fd641 to 17e0ea1 Compare January 14, 2024 00:12
AThousandShips
AThousandShips reviewed Jan 14, 2024
View reviewed changes
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.

Stylewise, will go over the code itself in a while 🙂

@i-snyder
Copy link
Contributor Author

Thanks so much! How can I make sure the spacing is done correctly -- is there a way for me to check the .rst file? I may have missed it in the documentation, but I've just been copy-pasting from Visual Studio into Notepad++ for the rst file, but do I need to replace tabs with spaces? I really appreciate all your help :)

@AThousandShips
Copy link
Member

Not that I know of, for editing for this I'd say to make sure your editor uses spaces and not tabs

@i-snyder i-snyder force-pushed the patch-1 branch 2 times, most recently from c557bdb to 8b159f1 Compare January 14, 2024 16:30
@i-snyder @AThousandShips
Update using_multiple_threads.rst
f14a3e8 
Update using_multiple_threads.rst

Adding C++ demos. It doesn't mirror the GDScript exactly, as I found this to be a clearer example of what the thread was doing when testing with multiple threads. All credit to "coder" from this Godot Forum thread: https://forum.godotengine.org/t/gdextension-c-async/36756/7?u=i-snyder

With applied suggestions from AThousandShips code review (many thanks!!)

Co-Authored-By: A Thousand Ships <[email protected]>
@i-snyder
Copy link
Contributor Author

Ok I've tested everything here and should be good to go. I made some minor modifications:

  1. As I was testing watching the Console, I saw that two of the examples weren't terminating the thread properly because of the check if (worker.is_valid() && worker->is_alive()) {, where worker->is_alive() is false unless the thread is still running. Removing that check seems to work properly.
  2. I've made the print output a bit more verbose to clarify what you're looking at when testing the different examples.

NOTE: the Output in the Editor won't show the complete output because of this open PR: godotengine/godot#79031

The Console will show correct output. Thanks so much!

i-snyder
i-snyder commented Sep 29, 2024
View reviewed changes
Copy link
Contributor Author

@i-snyder i-snyder left a comment

Choose a reason for hiding this comment

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

Sorry this is super old, not sure if I needed to click here to finalize? :D

@AThousandShips
Copy link
Member

The GDExtension team needs to give this a pass too

@i-snyder
Copy link
Contributor Author

The GDExtension team needs to give this a pass too

Do I need to do anything to have them take a look? I don't know how long to expect a pull request to be completed. Thanks so much for guiding me!

@AThousandShips
Copy link
Member

Will link this PR in the developer chat, feel free to join as well!

paddy-exe
paddy-exe reviewed Nov 13, 2024
View reviewed changes
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.

Just left some minor feedback. Really great work on this!

@i-snyder
Update using_multiple_threads.rst
9872960 
Removed the // THREADING comment and added note to use Runtime classes in Godot 4.3+ from @padde-exe's feedback
@mhilbrunner mhilbrunner merged commit 7855e17 into godotengine:master Nov 19, 2024
1 check passed
@mhilbrunner
Copy link
Member

Merged. Thanks and congrats on your first merged contribution! Thanks for incorporating all the feedback, sorry this took a while :)

@i-snyder i-snyder deleted the patch-1 branch November 19, 2024 17:10
@i-snyder
Copy link
Contributor Author

Merged. Thanks and congrats on your first merged contribution! Thanks for incorporating all the feedback, sorry this took a while :)

Thank you, and no worries! Make sure AThousandShips gets lots of brownie points for being so patient with me on this :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhilbrunner mhilbrunner mhilbrunner approved these changes

@paddy-exe paddy-exe paddy-exe approved these changes

@AThousandShips AThousandShips AThousandShips approved these changes

Assignees
No one assigned
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement topic:gdextension
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@i-snyder @AThousandShips @mhilbrunner @paddy-exe