Set start time to play sound in A_PlaySound

[Chris]

Because there's no guarantee that the alternate backend would do the same thing as OpenAL would. It's like when we got Vulkan - things are just .. different ... and things are also handled differently, even if ultimately the end-user never sees it.

The same thing might happen if OpenAL went through, for instance, a major refactor where the code was rewritten from scratch. Just as an example.

Basically the point I am trying to drive home is - when coding, it is important to check inputs and make sure they match what is actually supported by the library, in order to avoid mishaps in the future. Not doing input validation leads to unpredictable results, and more often than not, issues.

Oh, sure, the behavior should be defined/documented and predictable. That's actually what the question was about, whether the offset should auto-wrap or -clamp depending if the sound is looping or not, or always auto-clamp, or ignore the offset if it's out of range. OpenAL itself ignores out-of-range offsets, so you get that behavior "for free" if that's the preferred option. Any of the other methods can be implemented, though. Whichever you want, however you want.

[Nash]

More stylistically, the way you apply the default parameter value for startTime in the sound classes don't look right. It should be applied to the base declaration, what the calling code will "see". It's pretty useless on the implementation definition since callers won't be aware of it when calling the function. I think some compilers will actually warn about this.

Third time's a charm ... which one is the base declaration?

[Major Cooke]

Nash: I'm making a bunch of comments inside on where I believe Chris is talking about, with all the defaults. Notice how even my pitch parameter doesn't have defaults. That's because I encountered issues when trying to set defaults for pitch myself and, last I recall, it failed to compile even.

Simply remove those = 0.f defaults, but only where I made comments. The rest are fine.

[Chris]

Third time's a charm ... which one is the base declaration?

Sorry, I forgot to answer that. The base declaration for the sound renderer methods are in the SoundRenderer class in src/sound/backend/i_sound.h.

[Nash]

Is this any improvement?

[Chris]

Is this any improvement?

This doesn't look right:

Code: Select all

float st = (chanflags&SNDF_LOOP) ? fmod(startTime, (float)GetMSLength(sfx) / 1000.f) : clamp<float>(startTime, 0.f, (float)GetMSLength(sfx));

For non-looped sounds, it's clamping the start time to the length in milliseconds, rather than seconds. More generally, I think it would be better to always clamp the start time instead of conditionally wrapping it when looping, because a sound may have non-default loop points that one would expect such wrapping to acknowledge. I also think this can be just as easily handled outside of the backend, prior to any StartSound[3D] calls so it doesn't depend on the backend to implement this behavior.

[Nash]

Has long been added