0
\$\begingroup\$

Right now I have an awkward setup, but don't really know what the cleaner solution would look like. The parameter struct is defining the action keys for a simple fly camera. Right now it looks like this:

struct FlyCameraKeyParams {
    virtual int flyForwardKey() const = 0;
    virtual int flyBackwardKey() const = 0;
    // ... and others ...
};

struct FlyCameraDefaultKeyParams : public FlyCameraKeyParams {
    virtual int flyForwardKey() const override { return 'W'; }
    virtual int flyBackwardKey() const override { return 'S'; }
    // ...
};

The question is how to most conveniently allow this to be specified. I don't think it makes much sense to allow switching the control keys mid execution, so I'm leaning toward a template. But that feels unnecessary. Currently, though, I have this ridiculous pointer bool approach:

class FlyCamera {
public:
    FlyCamera(/* position, gaze, etc params */,
              FlyCameraKeyParams *keyParams = nullptr,
              bool deleteKeyParams = true) {
        if (!keyParams) {
            mKeyParams = new FlyCameraDefaultKeyParams();
            mDeleteKeyParams = true;
        }
        else {
            mDeleteKeyParams = deleteKeyParams;
        }
    }

    virtual ~FlyCamera() {
        if (mKeyParams && mDeleteKeyParams) delete mKeyParams;
    }

protected:
    FlyCameraKeyParams *mKeyParams = nullptr;
    bool mDeleteKeyParams = true;
};

In theory the user could just issue a new CustomKeyParams() in the construction of the FlyCamera and forget about it, and if they really need to own the memory then they could by setting the boolean at the end to false. But there's no reason for this, so should I just do:

template <class KeyParams = FlyCameraDefaultKeyParams>
class FlyCamera {
public:
    static_assert(std::is_base_of<FlyCameraKeyParams, KeyParams>::value,
                  "Template parameter KeyParams must inherit from FlyCameraKeyParams");

    FlyCamera(/* no key params arg here */) {
        mKeyParams = new KeyParams();
    }

    virtual ~FlyCamera() {
        if (mKeyParams) delete mKeyParams;
    }

protected:
    FlyCameraKeyParams *mKeyParams = nullptr;
};

I want to allow users to e.g. implement using the actual arrow keys if they don't want to use WASD. Are there any cleaner alternatives that come to mind?

\$\endgroup\$
1
\$\begingroup\$

It sounds like your primary problem right now is memory management — you're worried about how to use dynamic memory allocation without leaks or double-frees. C++ has a solution for that!

class FlyCamera {
public:
    FlyCamera(/* position, gaze, etc params */,
              std::unique_ptr<FlyCameraKeyParams> keyParams = nullptr) {
        if (keyParams == nullptr) {
            keyParams = std::make_unique<FlyCameraDefaultKeyParams>();
        }
        mKeyParams = std::move(keyParams);  // take ownership
    }

private:
    std::unique_ptr<FlyCameraKeyParams> mKeyParams = nullptr;
};

The destructor of unique_ptr<T> takes care of calling delete on the owned pointer (which will call the proper derived-class destructor as long as you've made the base-class destructor virtual).


However, for this particular problem, I don't see why you want a whole polymorphic class for FlyCameraKeyParams. Have you considered flattening things out into a more "C-like" style?

The name of the class should tip you off: I understand a desire for generic KeyBindings or something; but if your FlyCameraKeyParams are literally nothing more than Params to the FlyCamera, then you should make them params to the FlyCamera. Parameterizing the behavior of a concrete class is generally done by setting member variables.

Bonus: FlyCameraDefaultKeyParams describes the defaults of the key params of FlyCamera. So with C++11 member-variable-default-initializer syntax, we flatten that class away as well.

class FlyCamera {
public:
    FlyCamera(/* position, gaze, etc params */) { ... }

    void setFlyForwardKey(int k) { mFlyForwardKey = k; }
    void setFlyBackwardKey(int k) { mFlyBackwardKey = k; }

private:
    int mFlyForwardKey = 'W'; // Self-documenting defaults!
    int mFlyBackwardKey = 'S';
    // ... 
};

Notice in passing that I removed your virtual destructor from FlyCamera and made the protected members private, just to discourage people from inheriting from it. If you really need to allow inheritance, go ahead and put the virtual stuff back.

\$\endgroup\$
  • 1
    \$\begingroup\$ Excellent answer all around, thank you! Yes, std::unique_ptr would definitely make sense. However, I think I agree more with going back to flattening it back out. This was actually the original implementation and I got rid of it because it felt cluttered, but in reality the "decluttering" just makes it more obtuse \$\endgroup\$ – svenevs Nov 11 '17 at 21:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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