Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setPlayingOffset should check the passed value #966

Closed
eXpl0it3r opened this issue Sep 16, 2015 · 8 comments
Closed

setPlayingOffset should check the passed value #966

eXpl0it3r opened this issue Sep 16, 2015 · 8 comments
Assignees
Projects
Milestone

Comments

@eXpl0it3r
Copy link
Member

As reported on the forum, the value passed to setPlayingOffset() doesn't get checked and instead gets simply applied. If the offset is longer than the sound, the sound will start from the beginning, but the counter will keep counting from the non-existent offset.

@Bromeon
Copy link
Member

Bromeon commented Sep 16, 2015

Note: The semantics may change with #629. If that pull request is merged, wrap-arounds will make sense, and we could consider allowing modulo semantics in setPlayingOffset().

@Foaly
Copy link
Contributor

Foaly commented Jan 17, 2016

Working on #1041 I noticed that if you set an offest bigger than the length of the sound for OGG and FLAC the file will simply play from the beginning. The WAV reader correctly stops the stream if the offset is too big. None of the sf::SoundFileReader*::seek() handle this properly though. The WAV reader just works because the underlying sf::FileInputStream::seek() has this behaviour. Note that this contradicts the documentation of seek() in sf::InputSoundFile and sf::SoundFileReader

I know this is subject to change, but I just wanted to updated that this problem still exists.

@BionicFelps
Copy link

Just hit something similar (using version 2.3.2), applying an offset stored earlier from getPlayingOffset(), it restarts from the beginning, note that this is an attempt to essentially make my own pause functionality so that I don't have to keep the object alive while the music is paused... When I play it uses setPlayingOffset(); I've checked in debugging, the value of the sf::Time object is very close to the 25 seconds that I slept the thread for in my test (25703764 microseconds (this was while debugging so there was a couple seconds of me hitting the step over button, but should be well within my several minute music sample)).

I have similar code for the buffered sound object, but I haven't tested it yet...

verified the code path and value of my stored offset with the debugger, so I know it's not me (although it is a somewhat complex code base)

Hmm... I'll try setting the offset after play()

That works as expected, so it's just not getting the duration information from loadFromFile(), it only does so after it starts playing...

Buffered sounds work as expected.

@eXpl0it3r
Copy link
Member Author

Just hit something similar (using version 2.3.2), applying an offset stored earlier from getPlayingOffset(), it restarts from the beginning

Issues like you describe have already been fixed. Check out the master branch.

@eXpl0it3r eXpl0it3r self-assigned this Apr 25, 2016
@eXpl0it3r
Copy link
Member Author

So what exactly should the behavior be if the offset is too long? Should it start playing from the beginning or should it seek to the end and stop?

@Bromeon
Copy link
Member

Bromeon commented May 8, 2016

For the current version, I wouldn't apply a wrap-around. For offsets longer than the duration, we could either stop, or -- more aggressively -- have an assertion. Are there cases where this behavior is not a user error (e.g. rounding errors)?

Because this behavior will likely change as I stated above, I wouldn't document it as a feature, it merely fixes a bug with currently unvalidated arguments.

@eXpl0it3r eXpl0it3r modified the milestones: 2.5, 2.4 May 18, 2016
@eXpl0it3r
Copy link
Member Author

Having dug a bit deeper, this is something we'll have to address at the bottom, that is at the file stream readers. But I think, it makes more sense to define the behavior once and it for all the supported formats instead of patching random pieces of code. As such I removed the 2.4 tag.

@eXpl0it3r
Copy link
Member Author

Fixed by #1162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.5.0
  
Merged / Superseded
Development

No branches or pull requests

5 participants