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.
...
placeholders). It's much easier to review code when we're able to compile and test it ourselves! I see you already have some answers, so it's too late to fix this question, but please re-read How to Ask before posting your next review request. Thanks! \$\endgroup\$main()
to exercise it). If you have lots of other functionality mixed in, it might suggest possible improvements to the design (it's probably already hampering the unit tests, I guess). \$\endgroup\$