added getPan, setPan and setPanAbsolute #91
Conversation
lib/src/bindings_player_ffi.dart
Outdated
| // Note that because of the float<=>double conversion precision error | ||
| // (SoLoud lib uses floats), the returned value is not precise. Here we set | ||
| // a rounding of 5 digits | ||
| final ret = (_getPan(handle) * 100000).toInt() / 100000; |
There was a problem hiding this comment.
Frankly, I don't understand why we need to do this rounding. Is it not enough to just return the double?
There was a problem hiding this comment.
Since SoLoud lib uses floats and on Dart we use doubles, a conversion is needed. This conversion produces some aberration of the decimals as for standard IEEE 754 (hope I pointed out the right link to explain this).
So it happens this:
- from Dart we ask for a double with
getPan()that we think is a 0.8 double (usingsetPan()) - on cpp we get a 0.8 float which on Dart becomes a double.
this "converted" double becomes something like 0.800000042353 which has a 7-digit precision and could cause some troubles when testing it.
I think 5-digit precision is enough for the pan value. Of course, this can be done on cpp side instead.
There was a problem hiding this comment.
FWIW, if this is about testing, I'd just document this in the function signature ("Note that if you setPan() to 0.8 and then getPan(), you might get a slightly different number, such as 0.800000042353. This is expected since the internal audio engine uses float instead of double, and so there are rounding errors.").
Then, in unit tests, just use the closeTo() operand to deal with the tiny changes.
There was a problem hiding this comment.
Doing so we add something that the dev should remember when seeing a strange number given back. If you think this is the way, no problem to fix it.
Saying that I realized that this happens also for other functions like setVolume() - getVolume()! So, that ok :)
There was a problem hiding this comment.
Yeah, I think it's great to be aware of this and document this, but I don't think it's our place to quantize values for the developer in order to make the values "look right". I totally understand the sentiment, of course. I just think it's adding a functionality people might not necessarily want that needlessly adds more things to the package, too.
| if (pan > 1.0f) | ||
| pan = 1.0f; | ||
| if (pan < -1.0f) | ||
| pan = -1.0f; |
There was a problem hiding this comment.
Oh, I see that we're sanitizing the input here in C code. Since this is likely not a performance bottleneck, I'd rather sanitize it in Dart land, since that's closer to the user's debugger.
There was a problem hiding this comment.
You are right! Better to let the dev know what he is trying to do :)
| if (panLeft > 1.0f) panLeft = 1.0f; | ||
| if (panLeft < -1.0f) panLeft = -1.0f; | ||
| if (panRight > 1.0f) panRight = 1.0f; | ||
| if (panRight < -1.0f) panRight = -1.0f; | ||
| soloud.setPanAbsolute(handle, panLeft, panRight); |
There was a problem hiding this comment.
same here for panAbsolute: ideally sanitize (and assert) in Dart-land
I don't see the change here. Is this something that landed in another PR? I should have my iOS testing setup ready in a few minutes. Can test. |
Yes, please test PR #89. A lot of changes on cpp side there. |
Description
getPan(),setPan()andsetPanAbsolute()to fix feat: Add functions to directly get and set pan #90.Type of Change