3
\$\begingroup\$

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.

\$\endgroup\$
7
  • 4
    \$\begingroup\$ We prefer complete code here on Code Review (not ... 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\$ Jan 29, 2019 at 8:52
  • 2
    \$\begingroup\$ You appear to have stripped out quite a lot of code, making it harder to see what you're actually doing, why and where. Please, next time, show us how your code is structured and don't leave out as much as you did. We need to see the code in it's native habitat, not as snippets. \$\endgroup\$
    – Mast
    Jan 29, 2019 at 8:54
  • \$\begingroup\$ @TobySpeight thanks for your comment. My project is quite large and my question is about a very specific part of it. Do you rather I include more relevant parts or the code in it's entirety even though many parts are irrelevant to the question? \$\endgroup\$
    – asaf92
    Jan 29, 2019 at 10:32
  • 2
    \$\begingroup\$ That's a good question, and I think there's some answers over on MetaCR as to how to get a good reviewable subset from a larger program. It helps if your code structure is reasonably modular to begin with; then you should be able to present the relevant classes with their unit tests (or at least a small 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\$ Jan 29, 2019 at 10:37
  • \$\begingroup\$ @PanthersFan92 Is the entire project more or less than 60k characters? \$\endgroup\$
    – Mast
    Jan 29, 2019 at 19:40

2 Answers 2

6
\$\begingroup\$

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).

\$\endgroup\$
6
  • \$\begingroup\$ I need the base class because I have a vector of all the lights, so it's a vector of pointers to LightSource objects \$\endgroup\$
    – asaf92
    Jan 28, 2019 at 21:42
  • \$\begingroup\$ Dynamic casting sounds interesting. I think it might solve it. Thanks! \$\endgroup\$
    – asaf92
    Jan 28, 2019 at 21:44
  • 7
    \$\begingroup\$ @PanthersFan92 "I have a vector of all the lights". I think that's your problem. You've painted yourself into this corner and now you're looking for a workaroud, not a proper solution. A proper solution can very well mean having two vectors, one for Point and one for Parallel. I would advise against using the dynamic casting which would keep you entrenched into this false-commonality situation. Use the type system to your advantage. Do not write code that actively works around it. \$\endgroup\$
    – screwnut
    Jan 28, 2019 at 23:18
  • 2
    \$\begingroup\$ @PanthersFan92 Just another thing to note is dynamic casting is more expensive than a cleaner solution since it is all done at runtime. I'd advise you avoid dynamic casts as much as you can since it forces unnecessary runtime overhead. \$\endgroup\$
    – mascoj
    Jan 29, 2019 at 1:17
  • \$\begingroup\$ I agree that splitting the vector of Lightsources into separate containers is far better than using runtime checking. From the example one cannot tell if this is feasible or if it would mean a greater refactoring. So if possible, @PanthersFan92, follow screwnuts advice and use two vectors. \$\endgroup\$
    – Cornholio
    Jan 29, 2019 at 7:36
4
\$\begingroup\$

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?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Your point about ambient light is true, but I choose to exclude ambient lighting from this inheritance hierarchy because other than having a color it doesn't really share behaviors or properties that other light sources has. It doesn't have a location, it doesn't have a direction, and there's only one ambient light. \$\endgroup\$
    – asaf92
    Jan 29, 2019 at 10:51
  • \$\begingroup\$ @PanthersFan92 Its not exactly true. A singleton ambient light is not all that useful, and is better implemented in the shading model. On the otherhand a ambient light that has a position but no location for shading other than falloff is useful indeed. \$\endgroup\$
    – joojaa
    Jan 29, 2019 at 13:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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