search

afritz1 / OpenTESArena

Open-source re-implementation of The Elder Scrolls: Arena.
MIT License
988 stars 68 forks source link

mSource assertion fails in OpenALStream::init() #24

Closed afritz1 closed 8 years ago

afritz1 commented 8 years ago

assert(mSource != 0) in OpenALStream::init() fails when GameState::tick() is called the first time.

The mSongStream.reset(new OpenALStream(this, mCurrentSong.get())) function right before if (mSongStream->init(mFreeSources.front(), mMusicVolume)) in AudioManagerImpl::playMusic() seems to be the problem. It causes mSource to always be set to 0 in the OpenALStream() initialization list.

Could it just be that the mSongStream.reset() unintentionally sets mSource to 0?

Ragora commented 8 years ago

It looks like it could potentially just be the wrong variable being used there. Let me explain. According to the OpenALStream constructor, the mSource value will be set to 0: https://github.com/afritz1/OpenTESArena/blob/master/OpenTESArena/src/Media/AudioManager.cpp#L406

Cool, but the reason I say there might be wrong values in use here is the init itself using mSource: https://github.com/afritz1/OpenTESArena/blob/master/OpenTESArena/src/Media/AudioManager.cpp#L477

Why? It appears maybe it wants to assert(source != 0) because we're initializing mSource with the source value input to ::init: https://github.com/afritz1/OpenTESArena/blob/master/OpenTESArena/src/Media/AudioManager.cpp#L510

Which makes sense as to why mSource might not be initialized (well, why it might be 0).

afritz1 commented 8 years ago

I have a feeling it should say assert(source != 0). @kcat?

@Ragora, I think you should use the link of the actual commit instead of using master. It would go out of date if master was updated, right?

Ragora commented 8 years ago

Also ::reset will pretty much just reinitialize the unique_ptr with a new pointer value, destroying the old one if there is a non-NULL value stored: http://en.cppreference.com/w/cpp/memory/unique_ptr/reset

kcat commented 8 years ago

I have a feeling it should say assert(source != 0).

Actually that should've been assert(mSource == 0); to ensure the init method isn't being called again. The source parameter is valid, there's no way it can be called with 0 (if it is, there's more serious issues to worry about). I'll get a fix for that shortly.

Really annoying that cmake's RelWithDebInfo build type disables assertions, even though debug info is supposed to remain. I would've caught it earlier.