SFML community forums

Help => Audio => Topic started by: Nexus on June 08, 2009, 08:10:56 pm

Title: Bug in sf::Sound copy semantics?
Post by: Nexus 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.

Title: Bug in sf::Sound copy semantics?
Post by: Laurent 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 :)

Title: Bug in sf::Sound copy semantics?
Post by: Nexus 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?

Title: Bug in sf::Sound copy semantics?
Post by: Nexus 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.