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

Global main thread ID #1045

Merged
merged 1 commit into from
Feb 12, 2025
Merged

Conversation

TitanNano
Copy link
Contributor

In #1043 I need to identify the main thread without accessing the related engine API.

By storing the current thread ID during initialization, we can identify the main thread from that point on. I only found one other use of the main thread ID in godot-ffi but having access to the ThreadId of the main-thread could also be useful to user code.

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1045

@TitanNano TitanNano mentioned this pull request Feb 10, 2025
4 tasks
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks, good idea!

I'm not sure if I'm doing premature optimization here, but since initialization happens exactly once, I'm not a huge fan of all future code needing to obtain a lock to read the thread ID. This might even be slower than thread::current().id(), depending on how that is implemented/cached.

In the library we have ManualInitCell<T> for this purpose. The invariant is that the value is only accessed after initialization, which means it needs to be unsafe, which is a bit annoying for ergonomics. In Debug mode, misuse would still be caught.

Thoughts? If it's not used in hot paths, we could keep things simple with OnceLock.

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: ffi Low-level components and interaction with GDExtension API labels Feb 10, 2025
@TitanNano
Copy link
Contributor Author

Thoughts? If it's not used in hot paths, we could keep things simple with OnceLock.

Yeah, I think that is a good idea. I definitely need to check the main thread ID every time a new async task is created, and it's hard to say if that will end up in someone's hot path. It being unsafe is unfortunate, but manageable.

@TitanNano
Copy link
Contributor Author

@Bromeon this probably also fits better into the global module than meta right?

@TitanNano TitanNano force-pushed the jovan/main_thread_id branch 2 times, most recently from 04484f3 to 4d3d3ee Compare February 10, 2025 22:56
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

@Bromeon this probably also fits better into the global module than meta right?

Not really, that module is for global symbols from Godot itself.

Maybe init/mod.rs?
This functionality is only needed in godot-core, not in godot-ffi?

@TitanNano
Copy link
Contributor Author

Maybe init/mod.rs?
This functionality is only needed in godot-core, not in godot-ffi?

Hmm, yes, I think it could be used in godot-ffi as well. I'll move it there and expose the fn main_thread_id() from godot-core::init as well? I think this could be useful for user-code too, so I think it should be public.

@TitanNano TitanNano force-pushed the jovan/main_thread_id branch from 4d3d3ee to f0485c1 Compare February 11, 2025 19:32
@Bromeon
Copy link
Member

Bromeon commented Feb 12, 2025

Hmm, yes, I think it could be used in godot-ffi as well. I'll move it there and expose the fn main_thread_id() from godot-core::init as well? I think this could be useful for user-code too, so I think it should be public.

Good idea!

Can you add the panic docs under its own section # Panics? That would also keep the brief description of main_thread_id short. You can see how it renders in the link from the bot: https://godot-rust.github.io/docs/gdext/pr-1045/godot/init/index.html -- this will be updated with a latency of several minutes, whenever you push.

Do you think it also makes sense to add a function is_main_thread() -> bool accompanying it?

@TitanNano TitanNano force-pushed the jovan/main_thread_id branch from f0485c1 to 38a9949 Compare February 12, 2025 22:08
@TitanNano TitanNano force-pushed the jovan/main_thread_id branch from 38a9949 to 9718ffc Compare February 12, 2025 22:13
@TitanNano
Copy link
Contributor Author

Do you think it also makes sense to add a function is_main_thread() -> bool accompanying it?

Looks like a convenient shortcut, I have added it.

@Bromeon Bromeon added feature Adds functionality to the library and removed quality-of-life No new functionality, but improves ergonomics/internals labels Feb 12, 2025
@Bromeon Bromeon added this pull request to the merge queue Feb 12, 2025
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Merged via the queue into godot-rust:master with commit b2bff88 Feb 12, 2025
16 checks passed
@TitanNano TitanNano deleted the jovan/main_thread_id branch February 12, 2025 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: ffi Low-level components and interaction with GDExtension API feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants