godot-rust / godot-rust/gdext

Safely integrate Godot cross-thread calls (e.g. `AudioStreamPlayback::mix`)

Open
#1,131 9 comments 0 reactions 0 assignees View on GitHub
breaking-change c: engine c: threads feature
Dominant language
Rust
Stars
5.2k
Forks
312
Avg merge
11h 10m
Merged PRs (30d)
10

Description

This is a rough proposal for how to safely implement functions that currently require `experimental-threads`. It's not completely fleshed out but I wanted to see if there's any interest before I expand on it further.

If you're not taking proposals on how to solve this problem at the moment, or you don't find this proposal useful, I won't be offended if you close the issue.

Take `AudioStreamPlayback` as an example:

```rust
unsafe fn mix(&mut self, buffer: * mut AudioFrame, rate_scale: f32, frames: i32,) -> i32;
```

Godot calls `mix` from a non-main thread, which means that access to `self` in this function is unsound. (Without `experimental-threads` this condition is checked at runtime by godot-rust, which will panic before `mix` is even called).

My proposal is to remove the `self` parameter from functions that are called from a non-main thread, and replace it with a user-defined `state` parameter, which must impl `Clone + Send + 'static` and can therefore be passed between threads safely. For example:

```rust
trait IAudioStreamPlayback /* : ... */
where
AudioState: Clone + Send + 'static
{
/* ... */
unsafe fn mix(state: AudioState, buffer: * mut AudioFrame, rate_scale: f32, frames: i32) -> i32;
/* ... */
}
```

A sensible choice for `AudioState` might be for example `Arc>`, where `S` is some arbitrary struct. The `AudioState` could then be safely read and mutated from multiple threads. But the idea is that the actual type of `AudioState` should be left up to the programmer as long as it implements `Clone + Send + 'static`.

The state object should be attached to the class struct, for example:

```rust
#[derive(GodotClass)]
#[class(base=AudioStreamPlayback)]
struct Example {
#[audio_state]
state: Arc>,
}

#[godot_api]
impl IAudioStreamPlayback> for Example {
unsafe fn mix(state: Arc>, buffer: * mut AudioFrame, rate_scale: f32, frames: i32) -> i32 {
/ * ... */
frames
}
}

struct ExampleState {
position: usize,
}
```

godot-rust would then have some glue code that takes care of cloning `state` and passing it through to `IAudioStreamPlayback::mix` when `mix` is called from Godot.

It would be illegal to mark a field `#[export]` if it is also marked `#[audio_state]`.

This approach is roughly what I'm using in my own project, although I of course have to implement the glue manually each time I extend `AudioStreamPlayback`.

I *think* this general approach should be logically extensible to other cases where Godot calls functions from non-main threads.

If you're interested in something like this approach I would be interested in having a go at implementing it.

Contributor guide

Open the contributing guide

Research direction

Start with the AudioStreamPlayback::mix example and the existing experimental-threads behavior described in the issue. Trace where Godot invokes non-main-thread methods and where GodotClass and godot_api glue would need to carry state. Done would mean a settled, implementable design for Clone + Send + 'static state, including export restrictions and applicability to other cross-thread calls.

Written by the indexing model from the issue text.

Assessment

Tech stack
godot, rust
Domain
game-dev
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.