Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Bug in sf::Sound copy semantics?  (Read 4613 times)

0 Members and 1 Guest are viewing this topic.

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Bug in sf::Sound copy semantics?
« on: June 08, 2009, 08:10:56 pm »
Recently, I had a bug in my application concerning sf::Sound. I stored sf::Sounds in a container. When playing, they didn't show the right 3D-sound attributes. After debugging quite a while, I found out the sf::Sound copy constructor wouldn't copy the properties Attenuation and MinDistance.

In my opinion, this leads to unexpected copy semantics. I guess one would reckon those properties to be copied with the sf::Sound, too. At least I did so. Isn't this behaviour confusing and not coherent with other SFML classes like Sprite (in some way, sf::Sound is the audio equivalent of a sprite) which copy the whole object's state?

I think fixing this issue wouldn't be a big effort. You could expand the sf::Sound copy constructor with the following lines:
Code: [Select]
SetAttenuation(Copy.GetAttenuation());
SetMinDistance(Copy.GetMinDistance());

By the way, is there a reason why you duplicate code and write
Code: [Select]
ALCheck(alSourcei (mySource, AL_BUFFER,   myBuffer ? myBuffer->myBuffer : 0));
ALCheck(alSourcei (mySource, AL_LOOPING,  Copy.GetLoop()));
ALCheck(alSourcef (mySource, AL_PITCH,    Copy.GetPitch()));
ALCheck(alSourcef (mySource, AL_GAIN,     Copy.GetVolume() * 0.01f));
ALCheck(alSource3f(mySource, AL_POSITION, Copy.GetPosition().x, Copy.GetPosition().y, Copy.GetPosition().z));
instead of
Code: [Select]
SetBuffer(Copy.GetBuffer());
SetLoop(Copy.GetLoop());
SetPitch(Copy.GetPitch());
SetVolume(Copy.GetVolume());
SetPosition(Copy.GetPosition());
? This would be easier to maintain, too. The same applies to the other constructors which could call the set functions as well.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32498
    • View Profile
    • SFML's website
    • Email
Bug in sf::Sound copy semantics?
« Reply #1 on: June 08, 2009, 08:30:40 pm »
Quote
In my opinion, this leads to unexpected copy semantics. I guess one would reckon those properties to be copied with the sf::Sound, too. At least I did so. Isn't this behaviour confusing and not coherent with other SFML classes like Sprite (in some way, sf::Sound is the audio equivalent of a sprite) which copy the whole object's state?

Well, this is just a bug ;)

Quote
By the way, is there a reason why you duplicate code

Absolutely not.

Thanks for your feedback :)
Laurent Gomila - SFML developer

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Bug in sf::Sound copy semantics?
« Reply #2 on: June 08, 2009, 08:48:29 pm »
Quote from: "Laurent"
Thanks for your feedback :)
No problem, I like discussions and suggestions related to the library. :)

Sorry for posting in the wrong forum. In future, should I post potencial bug reports in the corresponding package subforum?
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Bug in sf::Sound copy semantics?
« Reply #3 on: June 08, 2009, 10:06:05 pm »
These were not the only missing attributes. You forgot some others, too - here is the complete list. I hope the copy constructor is implemented right like this (for example, I don't know about the status (playing/paused/stopped), how this is internally managed by OpenAL). But a suggestion:
Code: [Select]
Sound::Sound(const Sound& Copy) :
AudioResource(Copy)
{
    ALCheck(alGenSources(1, &mySource));

    // assign buffer if available
    if (Copy.GetBuffer())
    {
        SetBuffer(*Copy.GetBuffer());
    }

    // copy other attributes
    SetLoop(Copy.GetLoop());
    SetPitch(Copy.GetPitch());
    SetVolume(Copy.GetVolume());
    SetPlayingOffset(Copy.GetPlayingOffset());
    SetPosition(Copy.GetPosition());
    SetAttenuation(Copy.GetAttenuation());
    SetMinDistance(Copy.GetMinDistance());
    SetRelativeToListener(Copy.IsRelativeToListener());
}

The other constructor:
Code: [Select]
Sound::Sound(const SoundBuffer& Buffer, bool Loop, float Pitch, float Volume, const Vector3f& Position) :
myBuffer(&Buffer) // is this correct? I know too few about that...
{
    ALCheck(alGenSources(1, &mySource));

    SetBuffer(Buffer); // or is this the better way?
    SetLoop(Loop);
    SetPitch(Pitch);
    SetVolume(Volume);
    SetPosition(Position);
    // don't the other attributes have to be set?
}

Additionally, the assignment operator must be adapted.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

 

anything