Implementing different types of light sources in a Graphics project












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.










share|improve this question











$endgroup$








  • 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$
    – Toby Speight
    Jan 29 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 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$
    – PanthersFan92
    Jan 29 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$
    – Toby Speight
    Jan 29 at 10:37












  • $begingroup$
    @PanthersFan92 Is the entire project more or less than 60k characters?
    $endgroup$
    – Mast
    Jan 29 at 19:40
















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.










share|improve this question











$endgroup$








  • 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$
    – Toby Speight
    Jan 29 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 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$
    – PanthersFan92
    Jan 29 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$
    – Toby Speight
    Jan 29 at 10:37












  • $begingroup$
    @PanthersFan92 Is the entire project more or less than 60k characters?
    $endgroup$
    – Mast
    Jan 29 at 19:40














3












3








3


2



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










share|improve this question











$endgroup$




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.







c++ object-oriented inheritance polymorphism






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Jan 29 at 10:28







PanthersFan92

















asked Jan 28 at 19:45









PanthersFan92PanthersFan92

1215




1215








  • 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$
    – Toby Speight
    Jan 29 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 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$
    – PanthersFan92
    Jan 29 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$
    – Toby Speight
    Jan 29 at 10:37












  • $begingroup$
    @PanthersFan92 Is the entire project more or less than 60k characters?
    $endgroup$
    – Mast
    Jan 29 at 19:40














  • 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$
    – Toby Speight
    Jan 29 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 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$
    – PanthersFan92
    Jan 29 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$
    – Toby Speight
    Jan 29 at 10:37












  • $begingroup$
    @PanthersFan92 Is the entire project more or less than 60k characters?
    $endgroup$
    – Mast
    Jan 29 at 19:40








4




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$
– Toby Speight
Jan 29 at 8:52




$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$
– Toby Speight
Jan 29 at 8:52




2




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 at 8:54






$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 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$
– PanthersFan92
Jan 29 at 10:32




$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$
– PanthersFan92
Jan 29 at 10:32




2




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$
– Toby Speight
Jan 29 at 10:37






$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$
– Toby Speight
Jan 29 at 10:37














$begingroup$
@PanthersFan92 Is the entire project more or less than 60k characters?
$endgroup$
– Mast
Jan 29 at 19:40




$begingroup$
@PanthersFan92 Is the entire project more or less than 60k characters?
$endgroup$
– Mast
Jan 29 at 19:40










2 Answers
2






active

oldest

votes


















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






share|improve this answer









$endgroup$













  • $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$
    – PanthersFan92
    Jan 28 at 21:42










  • $begingroup$
    Dynamic casting sounds interesting. I think it might solve it. Thanks!
    $endgroup$
    – PanthersFan92
    Jan 28 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 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 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 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?






share|improve this answer









$endgroup$









  • 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$
    – PanthersFan92
    Jan 29 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 at 13:38











Your Answer





StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});


}
});














draft saved

draft discarded


















StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212417%2fimplementing-different-types-of-light-sources-in-a-graphics-project%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown

























2 Answers
2






active

oldest

votes








2 Answers
2






active

oldest

votes









active

oldest

votes






active

oldest

votes









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






share|improve this answer









$endgroup$













  • $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$
    – PanthersFan92
    Jan 28 at 21:42










  • $begingroup$
    Dynamic casting sounds interesting. I think it might solve it. Thanks!
    $endgroup$
    – PanthersFan92
    Jan 28 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 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 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 at 7:36
















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






share|improve this answer









$endgroup$













  • $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$
    – PanthersFan92
    Jan 28 at 21:42










  • $begingroup$
    Dynamic casting sounds interesting. I think it might solve it. Thanks!
    $endgroup$
    – PanthersFan92
    Jan 28 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 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 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 at 7:36














6












6








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






share|improve this answer









$endgroup$



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







share|improve this answer












share|improve this answer



share|improve this answer










answered Jan 28 at 20:27









CornholioCornholio

53117




53117












  • $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$
    – PanthersFan92
    Jan 28 at 21:42










  • $begingroup$
    Dynamic casting sounds interesting. I think it might solve it. Thanks!
    $endgroup$
    – PanthersFan92
    Jan 28 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 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 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 at 7:36


















  • $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$
    – PanthersFan92
    Jan 28 at 21:42










  • $begingroup$
    Dynamic casting sounds interesting. I think it might solve it. Thanks!
    $endgroup$
    – PanthersFan92
    Jan 28 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 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 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 at 7:36
















$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$
– PanthersFan92
Jan 28 at 21:42




$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$
– PanthersFan92
Jan 28 at 21:42












$begingroup$
Dynamic casting sounds interesting. I think it might solve it. Thanks!
$endgroup$
– PanthersFan92
Jan 28 at 21:44




$begingroup$
Dynamic casting sounds interesting. I think it might solve it. Thanks!
$endgroup$
– PanthersFan92
Jan 28 at 21:44




7




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 at 23:18




$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 at 23:18




2




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 at 1:17




$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 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 at 7:36




$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 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?






share|improve this answer









$endgroup$









  • 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$
    – PanthersFan92
    Jan 29 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 at 13:38
















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?






share|improve this answer









$endgroup$









  • 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$
    – PanthersFan92
    Jan 29 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 at 13:38














4












4








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?






share|improve this answer









$endgroup$



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?







share|improve this answer












share|improve this answer



share|improve this answer










answered Jan 28 at 20:41









AJNeufeldAJNeufeld

5,9751520




5,9751520








  • 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$
    – PanthersFan92
    Jan 29 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 at 13:38














  • 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$
    – PanthersFan92
    Jan 29 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 at 13:38








1




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$
– PanthersFan92
Jan 29 at 10:51




$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$
– PanthersFan92
Jan 29 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 at 13:38




$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 at 13:38


















draft saved

draft discarded




















































Thanks for contributing an answer to Code Review Stack Exchange!


  • Please be sure to answer the question. Provide details and share your research!

But avoid



  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.


Use MathJax to format equations. MathJax reference.


To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212417%2fimplementing-different-types-of-light-sources-in-a-graphics-project%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

Human spaceflight

Can not write log (Is /dev/pts mounted?) - openpty in Ubuntu-on-Windows?

張江高科駅