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

class Sound does not implement copy ctor properly #137

Closed
maciejewiczow opened this issue Aug 11, 2021 · 5 comments · Fixed by #138
Closed

class Sound does not implement copy ctor properly #137

maciejewiczow opened this issue Aug 11, 2021 · 5 comments · Fixed by #138

Comments

@maciejewiczow
Copy link
Contributor

Copying a Sound object does not copy the rAudioBuffer of AudioStream from the parent class. Multiple copies of one sound instance getting destroyed causes then a double-free error on the buffer (since they all share the same buffer).

@maciejewiczow
Copy link
Contributor Author

After digging around in raylib for a while I realised that copying a Sound struct is not that easy, so the copy ctor/assigmnent should be marked as deleted.

@RobLoach
Copy link
Owner

RobLoach commented Aug 12, 2021

Perhaps something like this?

    Sound(const ::Sound& sound) {
        set(sound);
    }

    Sound(const raylib::Sound& sound) {
        set(sound);
        sound.stream.buffer = NULL;
    }

    inline void Unload() {
        if (stream.buffer != NULL) {
           ::UnloadSound(*this);
        }
    }

@maciejewiczow
Copy link
Contributor Author

maciejewiczow commented Aug 12, 2021

That would not work, because in the constructor you're trying to modify the other sound object which is marked as const. Imo the best solution is to delete the copy operators, because raylib does not provide a mechanism to copy a sound struct (as far as I know).
Something like this

    Sound(const Sound&) = delete;
    Sound& operator=(const Sound&) = delete;

    Sound(Sound&& other) {
        set(other);

        other.sampleCount = 0;
        other.stream = { 0 };
    };

    Sound& operator=(Sound&& other) {
        if (this == &other)
            return *this;

        Unload();
        set(other);
        other.sampleCount = 0;
        other.stream = { 0 };

        return *this;
    };

    ~Sound() {
        Unload();
    }

I also added proper move ctor/assigmnent, because the default one also caused memory errors.
I can make a PR if you want

@RobLoach
Copy link
Owner

RobLoach commented Aug 12, 2021

Oh, that's pretty cool. A test of it would be awesome too. Something simple would be enough... Could adopt a similar pattern for the other classess too.

@maciejewiczow
Copy link
Contributor Author

I think that #102 is a symptom of the same problem that I encountered here surfacing in the other classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants