Bounded values in 0.10+

[Roms1383]

Hi @tesselode I have a question regarding latest changes in 0.10+ with values that range from -1.0 to 1.0, for example:

• Mix
• stereo_width
• and other ones

What happens when the value is out of range ? Does it panic ? Or produce unexpected audio output ? Or simply handles it gracefully inside kira ?

Would you like a PR that revisit these values to make them stricter ?
I'm especially thinking about types and values that implement Deserialize where basically inputs can easily be incorrectly set.

[tesselode]

I honestly don't remember how out-of-bounds values are handled. I think panicking could be appropriate, but only on the calling thread, not the audio thread. Otherwise, I think values should be silently clamped to be in bounds. I'd be happy to accept a PR that does one or both of those!

[Roms1383]

Honestly, for my own little case, panicking anywhere is something that I cannot afford because the last thing gamers want is their game crashing while they play.

Automatic clamping would be acceptable, but I was actually thinking of something simpler: why not leverage Rust's type system to have invalid values unrepresentable?

We could have something like:

// ...
pub struct PanningError {
	OutOfRange,
}

// ...
pub struct Panning(f32);

impl TryFrom<f32> for Panning {
	type Error = PanningError;
	fn try_from(value: f32) -> Result<Self, Self::Error> {
		if value >= -1.0 && value <= 1.0 {
			return Ok(Self(value));
		}
		Err(PanningError::OutOfRange)
	}
}

// more methods ...

This way it'd be simply impossible to construct a faulty Panning (well, outside of unsafe).

And, since it's generally annoying to have to .try_into()? everywhere,
we could also have a simple macro to allow constructing a Panning checked at compile-time, something like:

panning!(0.1);

Let me know what you think, I'm willing to give it a try.

[Roms1383]

I definitely will! I just cannot give an accurate ETA because I'm kinda overloaded at work, but it's gonna be soon :)

[tesselode]

no worries! lmk when you come up with something!

[Roms1383]

I'm slowly starting to experiment on my branch, I'll open a PR when I get something consistent.

Indeed this morning when using fallible implementation (e.g. TryFrom) it quickly becomes cumbersome, even with a convenience macro.

I'm currently testing adding debug_assert! in key methods instead, so nobody have to pay the cost in release builds while asserting safety during development. As for the math ops, clamping indeed seems like a good idea. In this approach, what is nice is that kira API does not have to change at all.

[Roms1383]

I should have started by this 😅
I've made a simple example for panning on this branch with both valid and invalid values actually there's no panic at all, invalid values just make the sound silent.

Knowing this, clamping values / turning f32::NAN into Panning::CENTER should probably be enough.

[Roms1383]

Ok the current experiments that I ran left me mitigated.

I tested clamping values in another branch in Tweenable::interpolate, but this led to a degradation in performances.
Clamping elsewhere means clamping through constructor + From + saturating math ops.

Your current implementation is probably the best,
considering it gives the best performance when one uses valid values only,
while other consumers who want stronger guarantees can still make their own newtypes over Panning, Mix, Decibels, etc.

I still think making Serialize/Deserialize stricter is worth it, because failing on serde is acceptable.
I will eventually open a PR at least for it + run a format workflow in CI.

I haven't been through parameters like Region yet, I probably will at some point next week.