Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am writing an audio engine that must be multi-platform (win/linux). Basically, a CSound represents a sound file, and CSoundInstance represents one playing of a CSound. Obviously, playing sound is very different on Windows than it is on linux.

A CSoundInstance needs access to some data from its corresponding CSound. The data is heavily implementation-dependent and cannot really be abstracted.

Here are the API headers for the two classes. I omitted operator= overloads / unique_ptr use etc.

API/Sound.hpp

class CSound
{
public: 
    CSound(...file...);
    ~CSound();
private: 
    CSoundImpl *impl;
};

API/SoundInstance.hpp

class CSoundInstance
{
public:
    CSoundInstance(CSound *); // going to need access to source voice buffer
    ~CSoundInstance();
    Play();
private:
    CSoundInstanceImpl *impl;
};

I guess it is clear where this is going. You would first instanciate a CSound, then acquire a CSoundInstance and Play it.

I Intend to have a specific XXImpl class for each system and platform. Under Windows, I use XAudio2, which needs several members in CSoundImpl (buffer + waveform descriptor), as well as CSoundInstanceImpl (voice + fx + sends + callbacks).


Here are the problems that I had using the pimpl idiom:

To play a sound, a CSoundInstanceImpl ultimately needs a reference to a CSoundImpl, on Windows and linux, but I cannot write this constructor:

CSoundInstance::CSoundInstance(CSound *sound)
    : impl(sound->impl)
{
    ...
}

Not only is sound->impl private, but it would also mess up unique_ptr use.

Another thing I tried is not using the constructor CSoundInstance(CSound *) at all, and adding a factory method with the signature CSoundInstance CSound::CreateInstance(), like this:

CSoundInstance CSound::CreateInstance()
{
    return this->impl->CreateInstance();
}

CSoundInstance CSoundImpl::CreateInstance()
{
    // here I have access to the CSoundImpl instance, that I need: this
    CSoundInstance instance(this);
}

But that would imply that I have this constructor:

CSoundInstance::CSoundInstance(CSoundImpl *soundImpl)
    : impl(new CSoundInstanceImpl(soundImpl))
{
    ...
}

Using a pointer to an implementation in the constructor of a non-implementation API class doesn't look right to me.

Maybe pimpl is not the right thing for this? Is there a way how I could tackle this problem better?

share|improve this question
1  
to me this sounds like a job for inheritance. You're asking for 2 different ways to do the same thing. So you would still use CSound, and CSoundImpl but those would be your parent classes. Make children classes of that, one for linux, and one for Windows. Then just have some simple logic to choose the correct child class, but use the parent class' methods to play/record your sounds. –  Robert Snyder Feb 3 '13 at 21:41
    
@RobertSnyder That sounds reasonable. I've heard from a colleague about using pimpl as an alternative to interfaces in C++ (unfortunately, my area of expertise is C#, and low level C), so I just tried it and didn't really question that. You should probably make an answer from your comment though. –  dialer Feb 3 '13 at 22:06
    
i can do that. Funny that you say that C# is your strength as that is my strength as well. C++ is new to me. I've not heard of pimpl so I don't want to discard it without knowing more about it, or it's principle. –  Robert Snyder Feb 3 '13 at 22:13

1 Answer 1

up vote 1 down vote accepted

As you said, if you need something in a pimpl class from another class, then something is wrong. If you can't abstract away the differences, then you can't hide the differences and shouldn't use this idiom.

Don't you think it would be simpler to put Play() in CSound? Each version (Linux/Windows) would have its own pimpl and there would be no need to try to find an abstraction or to try to access private data. Otherwise it gets a bit messy, since your Linux/Windows implementations of CSoundInstances will need to accept only the correct implementation: I don't know how easy it is to do this at compile time.

share|improve this answer
    
Your first sentence sums it up basically. I tried to use pimpl inappropriately. - But no, you really can't put Play() in CSound. CSound contains all static audio data ("the samples", up to several MB), and CSoundInstance contains playback specific parameters like effects, volume, panning, voice sends etc. Both are required for playback, and the same sound file can obviously be played multiple times simultaneously, with different parameters. –  dialer Feb 5 '13 at 18:32
    
Couldn't the play() method return the playing sound? –  Amy Blankenship Feb 6 '13 at 0:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.