Set start time to play sound in A_PlaySound

Post a reply

Smilies
:D :) :( :o :shock: :? 8-) :lol: :x :P :oops: :cry: :evil: :twisted: :roll: :wink: :geek: :ugeek: :!: :?: :idea: :arrow: :| :mrgreen: :3: :wub: >:( :blergh:
View more smilies

BBCode is OFF
Smilies are ON

Topic review
   

Expand view Topic review: Set start time to play sound in A_PlaySound

Re: Set start time to play sound in A_PlaySound

by Nash » Tue Jun 16, 2020 4:30 am

Has long been added

Re: Set start time to play sound in A_PlaySound

by Chris » Mon Oct 21, 2019 1:11 pm

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.

Re: Set start time to play sound in A_PlaySound

by Nash » Mon Oct 21, 2019 10:13 am

Re: Set start time to play sound in A_PlaySound

by Chris » Sat Oct 19, 2019 10:09 am

Nash wrote: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.

Re: Set start time to play sound in A_PlaySound

by Major Cooke » Fri Oct 18, 2019 12:39 pm

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.

Re: Set start time to play sound in A_PlaySound

by Nash » Thu Oct 17, 2019 11:31 pm

Chris wrote: 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?

Re: Set start time to play sound in A_PlaySound

by Chris » Mon Oct 14, 2019 5:28 pm

Rachael wrote: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.

Re: Set start time to play sound in A_PlaySound

by Nash » Mon Oct 14, 2019 5:12 pm

Okay, point taken, this discussion should have never happened in the first place, you can see in my branch that I did not leave the value unchecked (it wraps arounds). :D I just had my doubts because I was given a suggestion that perhaps it should be left to the caller.

Anyway, can I get some help on this?
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.

Re: Set start time to play sound in A_PlaySound

by Graf Zahl » Mon Oct 14, 2019 4:05 pm

... and if the unvalidated input leads to exploits of whatever kind, the shit really hits the fan.
"The caller should do it" is the worst thing to do here, especially when designing a scripting interface for public use.

Re: Set start time to play sound in A_PlaySound

by Rachael » Mon Oct 14, 2019 3:42 pm

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.

Re: Set start time to play sound in A_PlaySound

by Chris » Mon Oct 14, 2019 3:00 pm

Rachael wrote:
Nash wrote:Hmmm, after some thinking, in my opinion, the wrapping should be handled by the scripter. Doesn't seem like the engine's business to be dealing with these problems. You can get a sound's length in ZScript with S_GetLength, the user has everything they need to ensure their scripts behave well.
In most cases I'd normally agree with you, but what happens if we implement an alternative sound backend to OpenAL? The results would then be unpredictable in this case
How so? If there's an alternative sound backend, it would need to be able to report the length of a sound it loaded, meaning scripts still have the same info it needs to wrap. If a new backend isn't able to report a sound's length, all scripts relying on S_GetLength would fail.

Re: Set start time to play sound in A_PlaySound

by Graf Zahl » Mon Oct 14, 2019 8:02 am

Never EVER operate on unvalidated data. If you got out of range offsets there's only two options:

1) return an error
2) clamp the value to a reasonable range or revert to the default.

If there is even some remote chance that a bogus value produces side effects it needs to be dealt with, otherwise people WILL abuse it for whatever stupid reason they may come up with.

Re: Set start time to play sound in A_PlaySound

by Nash » Mon Oct 14, 2019 6:31 am

Well okay, sure, but in this very specific context, all that's happening here is if the "time" given to A_PlaySound is beyond range, the sound simply plays at the beginning. I'm not sure that's harmful at all, or affects anything significant in the long run... at best, I'd just apply a clamp rather than deal with the wrapping. With the clamp, the sound position will at least be safely reset to 0.

Re: Set start time to play sound in A_PlaySound

by Rachael » Mon Oct 14, 2019 6:29 am

Nash wrote:Hmmm, after some thinking, in my opinion, the wrapping should be handled by the scripter. Doesn't seem like the engine's business to be dealing with these problems. You can get a sound's length in ZScript with S_GetLength, the user has everything they need to ensure their scripts behave well.
In most cases I'd normally agree with you, but what happens if we implement an alternative sound backend to OpenAL? The results would then be unpredictable in this case, and history has taught us here that if someone discovers a hack doing things one way, they will abuse it, and then when the affected code gets changed or upgraded their hack breaks.

I'm not saying you're right or wrong, here, I am just throwing out a little caveat to consider.

Re: Set start time to play sound in A_PlaySound

by Nash » Mon Oct 14, 2019 6:26 am

Hmmm, after some thinking, in my opinion, the wrapping should be handled by the scripter. Doesn't seem like the engine's business to be dealing with these problems. You can get a sound's length in ZScript with S_GetLength, the user has everything they need to ensure their scripts behave well.
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.
I'm lost here. I don't really know what I'm doing here, if I'm being completely honest. :lol: Can you be very specific? I don't know which one is the base function. Also the fact that these sound functions are overloaded all over the place made it even more confusing for someone inexperienced like me.
Other than that, as far as GZDoom itself is concerned I'm still not sure how to deal with the fact that sounds can be modded and their length/timing changed, throwing off any kind of scripted offset. At best it could just apply a relative offset to a previously retrieved one, like your radio example, but beyond that it starts falling apart. Other people who are more acquainted with modding these kinds of things would have to speak on how that is usually handled.
I think this should be handled by the modder. This isn't the only case where scripting can get data from assets - remember, you can get texture and sprite dimensions too, and this is all freely accessible in scripting. Those textures can be easily replaced and stuff starts to get unpredictable. Whatever will happen here won't be something new to the engine. In my opinion, I think it's fine to leave this issue as-is...

Top