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

Fix OpenAL context management #602

Merged
merged 2 commits into from Jan 1, 2015

Conversation

Projects
None yet
7 participants
@binary1248
Member

binary1248 commented May 18, 2014

I Introduced AlResource to manage contexts the same way it is done for GlResources. The Listener properties are now part of the context itself and thus moved to the rest of the AudioDevice code so they can be delay applied if the user chooses to set them before a resource is constructed and reapplied if there is temporarily no context. sf::Listener just proxies everything to the AudioDevice which stores the values itself. This way no changes to the public API have to be made.

Because the AL context lifetime is tied to the lifetime of the resources that require it, unless the user decides to have global audio resources, the context should be destroyed before main() returns. This is a fix for #30.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 18, 2014

Member

A lot of code in AudioDevice is duplicated. Is there no way to avoid this?

Member

LaurentGomila commented May 18, 2014

A lot of code in AudioDevice is duplicated. Is there no way to avoid this?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 18, 2014

Member

Unless you want to drop exception safety... no 😄

Member

binary1248 commented May 18, 2014

Unless you want to drop exception safety... no 😄

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 18, 2014

Member

What about this:

void commonStuff()
{
    ...
}

void AudioDevice::stuff()
{
    if (!audioDevice)
    {
        AudioDevice device;
        commonStuff();
    }
    else
    {
        commonStuff();
    }
}

But I don't like to split functions this way, so maybe an auto/whatever ptr would be better.

Member

LaurentGomila commented May 18, 2014

What about this:

void commonStuff()
{
    ...
}

void AudioDevice::stuff()
{
    if (!audioDevice)
    {
        AudioDevice device;
        commonStuff();
    }
    else
    {
        commonStuff();
    }
}

But I don't like to split functions this way, so maybe an auto/whatever ptr would be better.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 18, 2014

Member

Replaced the duplicated code with auto_ptr<AudioDevice> in latest version.

Member

binary1248 commented May 18, 2014

Replaced the duplicated code with auto_ptr<AudioDevice> in latest version.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 22, 2014

Member

Do we really need std::auto_ptr? Not sure about compatibility with older compilers and as far as I know it's deprecated with C++11 anyway.

Member

MarioLiebisch commented May 22, 2014

Do we really need std::auto_ptr? Not sure about compatibility with older compilers and as far as I know it's deprecated with C++11 anyway.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 22, 2014

Member

The advantage of std::auto_ptr is that it can be replaced with std::unique_ptr when SFML starts to use C++11. And of course, RAII is generally preferable to raw new and delete.

Member

Bromeon commented May 22, 2014

The advantage of std::auto_ptr is that it can be replaced with std::unique_ptr when SFML starts to use C++11. And of course, RAII is generally preferable to raw new and delete.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 22, 2014

Member

I'm aware of that, I'm just not sure it's available everywhere.

Member

MarioLiebisch commented May 22, 2014

I'm aware of that, I'm just not sure it's available everywhere.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 22, 2014

Member

I'm aware of that, I'm just not sure it's available everywhere.

std::auto_ptr has been in the C++ standard since 1998. Compilers that compile C++98 must support it.

Member

Bromeon commented May 22, 2014

I'm aware of that, I'm just not sure it's available everywhere.

std::auto_ptr has been in the C++ standard since 1998. Compilers that compile C++98 must support it.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 22, 2014

Member

Okay, that sounds reasonable. :)

Member

MarioLiebisch commented May 22, 2014

Okay, that sounds reasonable. :)

@mantognini mantognini added this to the 2.x milestone May 26, 2014

@Bromeon Bromeon added the s:accepted label Jun 6, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

No negative review, so I guess this can be merged?

Member

TankOs commented Jun 11, 2014

No negative review, so I guess this can be merged?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 11, 2014

Member

Looks good, yes, but I wouldn't include it in SFML 2.2. Such a modification should be tested as much as possible by users to make sure it doesn't break anything.

Member

LaurentGomila commented Jun 11, 2014

Looks good, yes, but I wouldn't include it in SFML 2.2. Such a modification should be tested as much as possible by users to make sure it doesn't break anything.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

Sounds reasonable. :)

Member

TankOs commented Jun 11, 2014

Sounds reasonable. :)

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

Merged in develop branch, bugfix branch can be deleted.

Member

TankOs commented Jun 11, 2014

Merged in develop branch, bugfix branch can be deleted.

@TankOs TankOs closed this Jun 11, 2014

@TankOs TankOs reopened this Jun 12, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 12, 2014

Member

Will keep this open until milestone is hit...

Member

TankOs commented Jun 12, 2014

Will keep this open until milestone is hit...

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 5, 2014

Member

Looks good, yes, but I wouldn't include it in SFML 2.2. Such a modification should be tested as much as possible by users to make sure it doesn't break anything.

Given the amount of branches pilling up next to the list of issues and seeing the age of the issue, I don't think it's a good idea to just postpone the merge. If you're lucky one or two more people will actually bother compiling this branch and run it other than that, it most likely will never get tested well enough.

By changing the way we develop things, after SFML 2.2 we're no longer stuck on the 2.x branch, but can deploy hotfixes for critical issues. And with a hopefully reduced release cycle, we'll be able to push out enhancements way quicker. Thus the best way to actually test this, is to put it into the wild (or maybe something like an unstable branch).

Member

eXpl0it3r commented Jul 5, 2014

Looks good, yes, but I wouldn't include it in SFML 2.2. Such a modification should be tested as much as possible by users to make sure it doesn't break anything.

Given the amount of branches pilling up next to the list of issues and seeing the age of the issue, I don't think it's a good idea to just postpone the merge. If you're lucky one or two more people will actually bother compiling this branch and run it other than that, it most likely will never get tested well enough.

By changing the way we develop things, after SFML 2.2 we're no longer stuck on the 2.x branch, but can deploy hotfixes for critical issues. And with a hopefully reduced release cycle, we'll be able to push out enhancements way quicker. Thus the best way to actually test this, is to put it into the wild (or maybe something like an unstable branch).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jul 5, 2014

Member

What's wrong with merging it after 2.2 is released, so that it can be tested as long as we work on 2.3? Still much better than releasing a broken version and having to release a patch version in a hurry, in my opinion.

Member

LaurentGomila commented Jul 5, 2014

What's wrong with merging it after 2.2 is released, so that it can be tested as long as we work on 2.3? Still much better than releasing a broken version and having to release a patch version in a hurry, in my opinion.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jul 5, 2014

Member

I would also wait, it's better if SFML 2.2. contains less features, but is more stable. The number of branches piling up alone isn't reason enough to merge ;)

Furthermore, this context-related functionality is very subtle when it comes to errors depending on the used variable scope, different drivers and other factors. It will really need some testing, preferably an automated one. Maybe this can be a case for the unit tests once we start integrating them.

Member

Bromeon commented Jul 5, 2014

I would also wait, it's better if SFML 2.2. contains less features, but is more stable. The number of branches piling up alone isn't reason enough to merge ;)

Furthermore, this context-related functionality is very subtle when it comes to errors depending on the used variable scope, different drivers and other factors. It will really need some testing, preferably an automated one. Maybe this can be a case for the unit tests once we start integrating them.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 20, 2014

Member

Is this still blocked by 2.2? I don't mind, but considering this doesn't add or remove features and fixes one of the longest lived bugs, it might be worth considering for 2.2 as well.

Member

binary1248 commented Aug 20, 2014

Is this still blocked by 2.2? I don't mind, but considering this doesn't add or remove features and fixes one of the longest lived bugs, it might be worth considering for 2.2 as well.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 21, 2014

Member

It needs testing. And it won't be tested until it is merged into master, which is always postponed because we think SFML 2.2 is about to be released :D

Member

LaurentGomila commented Aug 21, 2014

It needs testing. And it won't be tested until it is merged into master, which is always postponed because we think SFML 2.2 is about to be released :D

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 25, 2014

Member

So.... what now? 😛

I think we can agree that 2.2 won't be done as soon as we anticipated 😉.

Member

binary1248 commented Sep 25, 2014

So.... what now? 😛

I think we can agree that 2.2 won't be done as soon as we anticipated 😉.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 25, 2014

Member

That's right, but I don't think we're in a hurry for merging this modification, are we?

Member

LaurentGomila commented Sep 25, 2014

That's right, but I don't think we're in a hurry for merging this modification, are we?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 25, 2014

Member

Well.... if there really is nothing else left to merge in a month and 2.2 is still waiting for certain things to be done... 😁.

Member

binary1248 commented Sep 25, 2014

Well.... if there really is nothing else left to merge in a month and 2.2 is still waiting for certain things to be done... 😁.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 2, 2014

Member

Positive test from me side. However I'd like to have a test that I can run that covers all changes. The sound and sound_capture examples are not enough.

Member

TankOs commented Oct 2, 2014

Positive test from me side. However I'd like to have a test that I can run that covers all changes. The sound and sound_capture examples are not enough.

@eXpl0it3r eXpl0it3r removed this from the 2.x milestone Nov 13, 2014

@binary1248 binary1248 removed this from the 2.x milestone Nov 13, 2014

@eXpl0it3r eXpl0it3r added this to the 2.3 milestone Dec 20, 2014

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 20, 2014

Member

Needs a rebase and further testing.

Member

eXpl0it3r commented Dec 20, 2014

Needs a rebase and further testing.

Made OpenAL context management more intelligent, in analogy to OpenGL…
… context management. OpenAL contexts now only exist as long as AlResources require them and are destroyed when they are no longer required. Fixes #30.
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Dec 21, 2014

Member

Rebased.

Member

binary1248 commented Dec 21, 2014

Rebased.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 22, 2014

Member

After some people give the green light, I'll be merging this branch.

Member

eXpl0it3r commented Dec 22, 2014

After some people give the green light, I'll be merging this branch.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 23, 2014

Member

Apart from the two minor comments that I posted, it looks good to me.

Member

LaurentGomila commented Dec 23, 2014

Apart from the two minor comments that I posted, it looks good to me.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Dec 27, 2014

Member

Issues fixed in latest commit.

Member

binary1248 commented Dec 27, 2014

Issues fixed in latest commit.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 27, 2014

Member

👍 for me

Member

LaurentGomila commented Dec 27, 2014

👍 for me

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 27, 2014

Member

Tested very quickly, works like a charm.

Member

mantognini commented Dec 27, 2014

Tested very quickly, works like a charm.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 28, 2014

Member

A bit strange... why not reset()?
If the idea is just to have a local variable, why not simply declare one (without auto_ptr)?

Member

Bromeon commented on src/SFML/Audio/AudioDevice.cpp in 0ad401c Dec 28, 2014

A bit strange... why not reset()?
If the idea is just to have a local variable, why not simply declare one (without auto_ptr)?

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Dec 28, 2014

Member

Check the newer version of the commit, there are comments that explain everything 😉.

Member

binary1248 replied Dec 28, 2014

Check the newer version of the commit, there are comments that explain everything 😉.

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 28, 2014

Member

I saw them but the app didn't let me comment on the diff, only on separate commits. I still don't see why you don't use auto_ptr::reset() ;)

Member

Bromeon replied Dec 28, 2014

I saw them but the app didn't let me comment on the diff, only on separate commits. I still don't see why you don't use auto_ptr::reset() ;)

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Dec 28, 2014

Member

So... what you are saying is that it should be a .reset() instead of an assignment? 😛

Member

binary1248 replied Dec 28, 2014

So... what you are saying is that it should be a .reset() instead of an assignment? 😛

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 28, 2014

Member

+1, that would be slightly less verbose.

Member

LaurentGomila replied Dec 28, 2014

+1, that would be slightly less verbose.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Dec 28, 2014

Member

Updated commit with .reset().

Member

binary1248 commented Dec 28, 2014

Updated commit with .reset().

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 28, 2014

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Dec 28, 2014

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r merged commit c4e450c into master Jan 1, 2015

@eXpl0it3r eXpl0it3r deleted the bugfix/al_context branch Jan 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment