new SampleHandle API to allow for streamed samples#129
Conversation
CorvusPrudens
left a comment
CorvusPrudens
left a comment
There was a problem hiding this comment.
I did some thinking about whether we even need the sample field in SamplerNode. Let me know what you think!
| impl Diff for SamplerNode { | ||
| fn diff<E: EventQueue>(&self, baseline: &Self, path: PathBuilder, event_queue: &mut E) { | ||
| if self.sample != baseline.sample { | ||
| match spawn_sample_handle_event(&self.sample) { | ||
| Ok(event) => event_queue.push(event), | ||
| Err(_e) => { | ||
| // TODO: log error | ||
| } | ||
| } | ||
| } | ||
|
|
||
| self.volume | ||
| .diff(&baseline.volume, path.with(0), event_queue); | ||
| self.play.diff(&baseline.play, path.with(1), event_queue); | ||
| self.play_from | ||
| .diff(&baseline.play_from, path.with(2), event_queue); | ||
| self.repeat_mode | ||
| .diff(&baseline.repeat_mode, path.with(3), event_queue); | ||
| self.speed.diff(&baseline.speed, path.with(4), event_queue); | ||
| self.mono_to_stereo | ||
| .diff(&baseline.mono_to_stereo, path.with(5), event_queue); | ||
| self.crossfade_on_seek | ||
| .diff(&baseline.crossfade_on_seek, path.with(6), event_queue); | ||
| self.min_gain | ||
| .diff(&baseline.min_gain, path.with(7), event_queue); | ||
| } | ||
| } |
There was a problem hiding this comment.
If there isn't any special utility in implementing Diff manually for SamplerNode, you could derive it by wrapping Option<SampleHandle> in a newtype and implementing Diff manually for it.
There was a problem hiding this comment.
Actually the macro was struggling with the Option<SampleHandle> (It wasn't happy that it wasn't Sync.)
But this won't matter if we decide to just remove the sample as a parameter.
| #[cfg_attr(feature = "bevy_reflect", reflect(ignore))] | ||
| #[cfg_attr(feature = "serde", serde(skip))] | ||
| pub sample: Option<ArcGc<dyn SampleResource>>, | ||
| pub sample: Option<SampleHandle>, |
There was a problem hiding this comment.
What if we simply don't include the sample as a field of SamplerNode? At this point, we're twisting the code up a bit in order to abstract over in-memory versus streaming sources. But presumably, we could avoid this by removing this field and directly sending the SampleInner in user code. That would give user-space total control over how SampleInner is created, which could avoid any need for channels, interior mutability, or other workarounds for spawning the stream-feeding workers within calls to Diff.
This would also turn SamplerNode back into proper POD. In bevy_seedling, we already abstract over the SamplerNode, so most users don't see or interact with it as a normal node. That means removing this field would not represent a breaking change for Bevy users. However, I understand that this may be disruptive for other users.
There was a problem hiding this comment.
Actually, you might have a point. This is probably trying too hard to have a purely data-driven API. Considering that the graph itself doesn't use a data-driven API, it would probably be better just to keep nodes simple.
| self.volume | ||
| .diff(&baseline.volume, path.with(0), event_queue); |
There was a problem hiding this comment.
Side note -- I think these might be out of sync now with the methods like SamplerNode::sync_volume_event. Those appear to be using the old indices still.
2 participants
This adds a new
SampleHandleAPI for the sampler node which adds support for!Syncsample resources that are streamed from disk/over a network.The one thing I'm not so sure about is that
StreamedSample::spawn_stream()doesn't take any arguments (because theDifftrait method which this method is called in doesn't take any arguments), meaning that users will have to do fancy things like message channels or statics to spawn any threads or futures for the stream. We could maybe consider adding an extra argument to theDifftrait to handle creating new threads/futures, though I'm not sure what that should look like.Note the streaming logic has not been implemented in the sampler node processor yet. This is just to get the groundwork done to make the integration process much smoother when we do decide to add streamed sample resources.
I also ended up having to do some manual diffing/patching to get this to work. The macros didn't like that a field was trying to send a non-cloneable, non-sync value through the event queue.
In the process of trying to do the diffing/patching, I found a use to have a
()parameter as a sort of "spacer" for the parameter indices. I ended up not using it, but I left theDiffandPatchimplementations for()in the PR anyway.