Dtyjlui

Edit: I got asked a lot why I need to have the LightSource base class or why do I keep one vector of all the light sources, so here's my explanation:

In many points in the code I need to interact with all the lights regardless of their type. For example, I manually implemented a "shader" which looped over every face and got a direction to the light source without knowing whether it's point or parallel. Here's a code example:

const glm::vec4 Shader::calculatePhongReflection(const glm::vec4& normal,const glm::vec4& worldPoint, const glm::vec4& toCamera) const

{

    glm::vec4 ambientPart = calculateAmbientPart();

    glm::vec4 lightSum(0);

    for each (LightSource* light in scene.GetLightsVector())

    {

        glm::vec4 diffusePartSum = calculateDiffusePart (normal,worldPoint,light);

        glm::vec4 spectralPartSum = calculateSpecularPart(normal,worldPoint,toCamera,light);

        lightSum += diffusePartSum + spectralPartSum;

    }

    return ambientPart + lightSum;

}

In calculateDiffusePart and calculateSpecularPart I have this line:

glm::vec4 directionToLight = glm::normalize(lightSource->GetDirectionToLightSource(worldPoint));

So there's a benefit to maintaining one vector that holds all the lights. I don't need to iterate over two vectors (which will inevitably grow in number as I add different types of light) in this part of the code which really doesn't need to know the details of the light source. If I don't keep the base class I'd have to edit many different parts of the code every time I want to add a new type of light when all I actually need is the direction to the light (or in my current implementation which uses OpenGL, the location of the light source).

Original:

I'm working on a Computer Graphics project and I found a way to implement different types of light sources which I'm not sure is ideal.

There are two types of light sources: parallel, and point.

Point light source is moving around the mesh, and Parallel light source "can be imagined as a point light source lying in infinity". In other words a parallel light source has a direction which is constant throughout the mesh while a point light source is illuminating in all directions, but has a position which effects the direction of the light compared to the surface it's lighting.

I'm having trouble figuring out the right abstraction for all these types. Here's my solution:
I have one abstract base class called LightSource:

class LightSource: public IMovable,public IRotatable

{

protected:

    glm::vec4 color;

public:

...

    // Virtual Setters

    virtual void SetDirection(const glm::vec4& _direction) =0;

    virtual void SetLocation (const glm::vec4& _location)  =0;



    // Virtual Getters

    virtual const glm::vec4* GetDirection() const =0;

    virtual const glm::vec4* GetLocation()  const =0;



    // Inherited via IMovable

    virtual void Move(const glm::vec3 direction) override =0;



    // Inherited via IRotatable

    virtual void RotateX(const float angle) override =0;

    virtual void RotateY(const float angle) override =0;

    virtual void RotateZ(const float angle) override =0;

};

and two derived classes:

class PointLightSource : public LightSource

{

private:

...

public:

    // Constructors

...

    // Base class 

    virtual const glm::vec4 * GetDirection() const override { return nullptr; };

    virtual const glm::vec4 * GetLocation() const override { return &location; };



    // Base class setters

    virtual void SetDirection(const glm::vec4 & _direction) override { return; }

    virtual void SetLocation(const glm::vec4 & _location)   override { location = _location; }



    // Inherited via LightSource

    virtual void Move(const glm::vec3 direction) override;

    virtual void RotateX(const float angle) override {} // Point light source emits light everywhere

    virtual void RotateY(const float angle) override {} 

    virtual void RotateZ(const float angle) override {} 

...

};

and:

class ParallelLightSource : public LightSource

{

private:

...

public:

    ...

    // Setters

    virtual void SetDirection(const glm::vec4 & _direction) override { direction = glm::normalize(_direction); }

    virtual void SetLocation(const glm::vec4 & _location) override { return; }



    // Getters

    virtual const glm::vec4 * GetDirection() const override { return &direction; }

    virtual const glm::vec4 * GetLocation() const override { return nullptr; };



    // Inherited via LightSource

    virtual void RotateX(const float angle) override;

    virtual void RotateY(const float angle) override;

    virtual void RotateZ(const float angle) override;



    // Can't move a parallel light source

    virtual void Move(const glm::vec3 direction) override {}

    ...

};

LightSource virtually implements two interfaces. one is called IMovable and it's used for every object that moves around the mesh (whether it's a model, a camera, or a light source).
another interface is called IRotatable, and it's used for objects that can rotate. Not every movable object is rotatable, and point light source is a good example of that, so I did two interfaces.

Essentially what I want is to be able to move point light source but not rotate them, and rotate parallel light sources without moving them, and do all that without checking for their actual type due to polymorphism. The problem is that every LightSource is both IMovable and IRotatable, so I can't distinguish between the derived types.

My awkward solution was to have two virtual functions: GetLocation and GetDirection that return pointers to vectors. the ParallelLightSource will return nullptr on GetLocation, and PointLightSource will return nullptr on GetDirection.

In the menus part of the code, I receive a vector of LightSource pointers and I want to display their appropriate menus. There are (as you guessed) two types of menus that are relevant here, one for IMovable objects and one for IRotatable objects.
I call the GetLocation & GetDirection functions and skip the appropriate controls if the pointer I got is a nullptr.

const glm::vec4* lightLocation =  activeLight->GetLocation();

const glm::vec4* lightDirection = activeLight->GetDirection();



...

if (lightLocation != nullptr)

{

    moveObjectControls(activeLight,"Light");

}

if (lightDirection != nullptr)

{

    newDirection = *lightDirection;

    xyzSliders(newDirection, "Direction", worldRadius);

}

Passing around vector pointers rather than vectors by value is problematic. For example, now I actually do need ParallelLightSource to implement GetLocation and return an actual value. I'd have to either have a location class member which will always be equal to -direction * someLargeNumber, or refactor and have GetLocation and GetDirection return values, which will create another mess in the menus because I don't want to see controls that "move" a parallel light source or "rotate" a point light source because it doesn't make any sense, so I'd have to check for the derived types somehow which breaks polymorphism.

One solution I thought of is having booleans to indicate whether I want this light to show the IMovable/IRotatble menus, but that doesn't feel right because I don't think it's the light source responsibility to know how it is presented in the menus.

Another solution would be to hold one vector for parallel light sources and one for point light sources, but that doesn't sound smart because it's not very scalable and every time I'll implement a new type of light source I'll have to change the menus.

I'm quite new at programming (at least in terms of OOP), so I wonder what's the ideal solution to a situation like this. Let me know if you have other comments as well.

Why is LightSource inheriting from IMovable and IRotatable if not every LightSource is movable or rotatable? LightSource should only contain functions that relate to light sources, for instance intensity or color. The position and the direction are not properties of a light source here. Instead I'd make ParallelLightSource inherit from IRotatable and PointLightSource inherit from IMovable.

From your minimal example I don't see any need for the base class LightSource but you may have left this stuff out to illustrate your problem. If you do not need it, remove it.

I understand you want to edit your sources using menus, but I have no idea how your application organizes the data here.
If you really need the base class LightSource you may use RTTI to check whether your object is a IMovable or an IRotatable when creating the menu (dynamic_cast<IMovable*>(ptr) checks if ptr is a subclass of IMovable).

Overriding RotateX, RotateY and RotateZ to prevent rotation of your point light source is going to cause you grief when you add another type of light source: the spot light. A spot light is a directional point light source, and would be both IMovable and IRotatable. But if you try to derive it from PointLightSource, you’d end up having to add back in the the functionality you took out by overriding.

You probably want to remove IMovable and IRotatable from LightSource, and only add them to derived types that have those attributes.

What is a non-movable, non-rotatable light source you ask? How about AmbientLight?