Fake 3D effect in SFML - follow up

Question

Based on my previous question, I have implemented all the recommendations received.

Here is a summary of the improvements:

• Use meaningful names as much as possible.
• Removed multiple declarations in one line.
• Removed the dynamic_casts and applied a new class hierarchy design.
• Added a Game class to improve code modularity.

I would like to know whether or not I have implemented the class hierarchy design for Colors in a suitable way.

#include <SFML/Graphics.hpp>
#include <vector>
#include <memory>
#include <cmath>
#include <stdexcept>
#include <iostream>
namespace
{
 float increase(float start, float increment, float max)
 {
 auto result = start + increment;
 while (result >= max)
 result -= max;
 while (result < 0)
 result += max;
 return result;
 }
 float limit(float value, float min, float max)
 {
 return std::max(min, std::min(value, max));
 }
}
class Polygon : public sf::Drawable, public sf::Transformable, sf::NonCopyable
{
public:
 Polygon() : mVertices(sf::Quads, 4u) {}
 void setVertices(float x1, float y1, float x2, float y2, float x3, float y3, float x4, float y4, sf::Color color)
 {
 mVertices[0].position = sf::Vector2f(x1, y1);
 mVertices[1].position = sf::Vector2f(x2, y2);
 mVertices[2].position = sf::Vector2f(x3, y3);
 mVertices[3].position = sf::Vector2f(x4, y4);
 mVertices[0].color = mVertices[1].color = mVertices[2].color = mVertices[3].color = color;
 }
private:
 void draw(sf::RenderTarget& target, sf::RenderStates states) const
 {
 states.transform *= getTransform();
 target.draw(mVertices, states);
 }
 sf::VertexArray mVertices;
};
struct Point
{
 struct Screen
 {
 float x{};
 float y{};
 float width{};
 }screen{};
 sf::Vector3f world{};
 sf::Vector3f camera{};
 void project(float cameraX, float cameraY, float cameraZ, float cameraDepth, float width, float height, float roadWidth)
 {
 camera.x = world.x - cameraX;
 camera.y = world.y - cameraY;
 camera.z = world.z - cameraZ;
 auto scale = cameraDepth / camera.z;
 screen.x = width / 2.f + scale * camera.x * width / 2.f;
 screen.y = height / 2.f - scale * camera.y * height / 2.f;
 screen.width = scale * roadWidth * width / 2.f;
 }
};
class Colors
{
public:
 Colors() : mSelf(this) {}
 virtual ~Colors() = default;
 virtual sf::Color getRoad() { return{}; }
 virtual sf::Color getGrass() { return{}; }
 virtual sf::Color getRumble() { return{}; }
 virtual sf::Color getLane() { return{}; }
 void setColor(Colors& c) { mSelf = &c; }
 Colors& getColor() const { return *mSelf; }
private:
 Colors* mSelf;
};
class Light : public Colors
{
 sf::Color getRoad() {
 return{ 100, 100, 100 };
 }
 sf::Color getGrass() {
 return{ 16, 170, 16 };
 }
 sf::Color getRumble() {
 return{ 85, 85 , 85 };
 }
 sf::Color getLane() {
 return{ sf::Color::White };
 }
}light;
class Dark : public Colors
{
 sf::Color getRoad() {
 return{ 100, 100, 100 };
 }
 sf::Color getGrass() {
 return{ 0, 154, 0 };
 }
 sf::Color getRumble() {
 return{ 187,187, 187 };
 }
 sf::Color getLane() {
 return{ 100, 100, 100 };
 }
}dark;
class Segment : public sf::Drawable, public sf::Transformable, sf::NonCopyable
{
public:
 Segment()
 : mPoint1()
 , mPoint2()
 , mRumbleSide1()
 , mRumbleSide2()
 , mLanes1()
 , mLanes2()
 , mMainRoad()
 , mLandscape()
 , mColors()
 , mIndex()
 {}
 Point& getPoint1() { return mPoint1; }
 Point& getPoint2() { return mPoint2; }
 void setSegmentColors(Colors& it) { mColors.setColor(it); }
 void setIndex(std::size_t i) { mIndex = i; }
 std::size_t getIndex() const { return mIndex; }
 void setGrounds(float width)
 {
 auto lanes = 3u;
 // Landscape
 mLandscape.setSize({ width, mPoint1.screen.y - mPoint2.screen.y });
 mLandscape.setPosition(0, mPoint2.screen.y);
 mLandscape.setFillColor(mColors.getColor().getGrass());
 // Rumble sides
 auto rumbleWidth1 = rumbleWidth(mPoint1.screen.width, lanes);
 auto rumbleWidth2 = rumbleWidth(mPoint2.screen.width, lanes);
 mRumbleSide1.setVertices(mPoint1.screen.x - mPoint1.screen.width - rumbleWidth1, mPoint1.screen.y,
 mPoint1.screen.x - mPoint1.screen.width, mPoint1.screen.y,
 mPoint2.screen.x - mPoint2.screen.width, mPoint2.screen.y,
 mPoint2.screen.x - mPoint2.screen.width - rumbleWidth2, mPoint2.screen.y, mColors.getColor().getRumble());
 mRumbleSide2.setVertices(mPoint1.screen.x + mPoint1.screen.width + rumbleWidth1, mPoint1.screen.y,
 mPoint1.screen.x + mPoint1.screen.width, mPoint1.screen.y,
 mPoint2.screen.x + mPoint2.screen.width, mPoint2.screen.y,
 mPoint2.screen.x + mPoint2.screen.width + rumbleWidth2, mPoint2.screen.y, mColors.getColor().getRumble());
 // Main Road
 mMainRoad.setVertices(mPoint1.screen.x - mPoint1.screen.width, mPoint1.screen.y,
 mPoint1.screen.x + mPoint1.screen.width, mPoint1.screen.y,
 mPoint2.screen.x + mPoint2.screen.width, mPoint2.screen.y,
 mPoint2.screen.x - mPoint2.screen.width, mPoint2.screen.y, mColors.getColor().getRoad());
 // Lanes
 auto laneMarkerWidth1 = laneMarkerWidth(mPoint1.screen.width, lanes);
 auto laneMarkerWidth2 = laneMarkerWidth(mPoint2.screen.width, lanes);
 auto lanew1 = mPoint1.screen.width * 2 / lanes;
 auto lanew2 = mPoint2.screen.width * 2 / lanes;
 auto lanex1 = mPoint1.screen.x - mPoint1.screen.width + lanew1;
 auto lanex2 = mPoint2.screen.x - mPoint2.screen.width + lanew2;
 for (auto lane = 1u; lane < lanes; lanex1 += lanew1 + 1, lanex2 += lanew2 + 1, lane++)
 {
 if (lane == 1)
 mLanes1.setVertices(lanex1 - laneMarkerWidth1 / 2, mPoint1.screen.y,
 lanex1 + laneMarkerWidth1 / 2, mPoint1.screen.y,
 lanex2 + laneMarkerWidth2 / 2, mPoint2.screen.y,
 lanex2 - laneMarkerWidth2 / 2, mPoint2.screen.y, mColors.getColor().getLane());
 else
 mLanes2.setVertices(lanex1 - laneMarkerWidth1 / 2, mPoint1.screen.y,
 lanex1 + laneMarkerWidth1 / 2, mPoint1.screen.y,
 lanex2 + laneMarkerWidth2 / 2, mPoint2.screen.y,
 lanex2 - laneMarkerWidth2 / 2, mPoint2.screen.y, mColors.getColor().getLane());
 }
 }
private:
 float rumbleWidth(float projectedRoadWidth, std::size_t lanes)
 {
 return projectedRoadWidth / std::max(6u, 2 * lanes);
 }
 float laneMarkerWidth(float projectedRoadWidth, std::size_t lanes)
 {
 return projectedRoadWidth / std::max(32u, 8 * lanes);
 }
 void draw(sf::RenderTarget& target, sf::RenderStates states) const
 {
 states.transform *= getTransform();
 target.draw(mLandscape, states);
 target.draw(mRumbleSide1, states);
 target.draw(mRumbleSide2, states);
 target.draw(mMainRoad, states);
 target.draw(mLanes1, states);
 target.draw(mLanes2, states);
 }
private:
 Point mPoint1;
 Point mPoint2;
 Polygon mRumbleSide1;
 Polygon mRumbleSide2;
 Polygon mLanes1;
 Polygon mLanes2;
 Polygon mMainRoad;
 sf::RectangleShape mLandscape;
 Colors mColors;
 std::size_t mIndex;
};
class Game
{
public:
 Game()
 : mWindow(sf::VideoMode(640, 480), "test")
 , mSegmentLength(200.f)
 , mPlayerX(0.f)
 , mCameraDepth(1 / std::atan((100 / 2.f)))
 , mCameraHeight(1000.f)
 , mPosition(0.f)
 , mTrackLength(0.f)
 , mSegments()
 , mSpeed(0.f)
 {
 addRoad();
 }
 void run()
 {
 sf::Clock clock;
 auto timeSinceLastUpdate = sf::Time::Zero;
 while (mWindow.isOpen())
 {
 auto elapsedTime = clock.restart();
 timeSinceLastUpdate += elapsedTime;
 while (timeSinceLastUpdate > TimePerFrame)
 {
 timeSinceLastUpdate -= TimePerFrame;
 processEvents();
 update(TimePerFrame);
 }
 render();
 }
 }
private:
 void processEvents()
 {
 sf::Event event;
 while (mWindow.pollEvent(event))
 {
 if (event.type == sf::Event::Closed)
 mWindow.close();
 }
 }
 void update(sf::Time timePerFrame)
 {
 auto dt = timePerFrame.asSeconds();
 auto maxSpeed = mSegmentLength / dt;
 auto accel = maxSpeed / 5.f;
 auto breaking = -maxSpeed;
 auto decel = -accel;
 auto offRoadDecel = -maxSpeed / 2.f;
 auto offRoadLimit = maxSpeed / 4.f;
 const auto& playerSegment = *mSegments[static_cast<std::size_t>(std::floor(mPosition / mSegmentLength)) % mSegments.size()];
 auto speedPercent = mSpeed / maxSpeed;
 auto dx = dt * speedPercent;
 if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))
 mPlayerX -= dx;
 if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right))
 mPlayerX += dx;
 if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
 mSpeed += accel * dt;
 else
 mSpeed += decel * dt;
 if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))
 mSpeed += breaking * dt;
 if (((mPlayerX < -1.f) || (mPlayerX > 1.f)) && (mSpeed > offRoadLimit))
 mSpeed += offRoadDecel * dt;
 mPlayerX = limit(mPlayerX, -2.f, 2.f);
 mSpeed = limit(mSpeed, 0, maxSpeed);
 mPosition = increase(mPosition, dt * mSpeed, mTrackLength);
 }
 void render()
 {
 auto width = 640.f;
 auto height = 480.f;
 auto roadWidth = 2000.f;
 auto maxy = height;
 const auto& baseSegment = *mSegments[static_cast<std::size_t>(std::floor(mPosition / mSegmentLength)) % mSegments.size()];
 mWindow.clear();
 for (auto n = 0u; n < mSegments.size(); n++)
 {
 auto& segment = *mSegments[(baseSegment.getIndex() + n) % mSegments.size()];
 bool looped = segment.getIndex() < baseSegment.getIndex();
 auto camX = mPlayerX * roadWidth;
 auto camY = mCameraHeight;
 auto camZ = mPosition - (looped ? mTrackLength : 0.f);
 auto& point1 = segment.getPoint1();
 auto& point2 = segment.getPoint2();
 point1.project(camX, camY, camZ, mCameraDepth, width, height, roadWidth);
 point2.project(camX, camY, camZ, mCameraDepth, width, height, roadWidth);
 if ((point1.camera.z <= mCameraDepth) || (point2.screen.y >= maxy))
 continue;
 segment.setGrounds(width);
 mWindow.draw(segment);
 maxy = point2.screen.y;
 }
 mWindow.display();
 }
 void addRoad()
 {
 auto rumbleLength = 3u;
 mSegments.reserve(500);
 for (auto i = 0u; i < 500; ++i)
 {
 auto segment = std::make_unique<Segment>();
 segment->setIndex(i);
 segment->getPoint1().world.z = i * mSegmentLength;
 segment->getPoint2().world.z = (i + 1) * mSegmentLength;
 if (static_cast<std::size_t>(std::floor(i / rumbleLength)) % 2)
 segment->setSegmentColors(light);
 else
 segment->setSegmentColors(dark);
 mSegments.push_back(std::move(segment));
 }
 mTrackLength = mSegments.size() * mSegmentLength;
 }
private:
 sf::RenderWindow mWindow;
 float mSegmentLength;
 float mPlayerX;
 float mCameraDepth;
 float mCameraHeight;
 float mPosition;
 float mTrackLength;
 std::vector<std::unique_ptr<Segment>> mSegments;
 const static sf::Time TimePerFrame;
 float mSpeed;
};
const sf::Time Game::TimePerFrame = sf::seconds(1 / 60.f);
int main()
{
 try
 {
 Game game;
 game.run();
 }
 catch (std::exception& e)
 {
 std::cout << "\nEXCEPTION: " << e.what() << std::endl;
 }
}

• glampert: It was a big improvement from the last one, great job!
• MORTAL: @glampert .. i did what you told me to do. :)
• glampert: Hahaha, yes, I'm biased, but I'm sure others will also agree ;)

Answer 1

Like I commented, It looks pretty clean overall, so I'll only mention a couple things this time. You might consider waiting a little bit before selecting this, in case other reviewers show up.

• mSelf in Colors is strange. I didn't quite understand its purpose and what you are trying to achieve with it that can't be accomplished with the this pointer. I think you might have done that because of the getColor/setColor methods? That's not a very conventional approach. The usual would be for Segment to have a Colors pointer, then just swap that pointer when you need to setSegmentColors.

  void setSegmentColors(const Colors * c) { mColors = c; }
  

• The methods of Colors could probably be pure virtual (= 0;). I don't suppose you'd need to declare a Colors instance by value, one the mSelf oddity is fixed.

• When you implement a virtual base class, be sure to annotate the methods in child classes with override.

• Also take a look into the final specifier. You can add it to classes that are not meant to be inherited from and to leaf nodes in a class hierarchy.

• All the virtual get* methods in Colors should be const. They are not mutating any member data.

• The globals light and dark, even though not carrying any member state as of now, should also probably be constants. I'm very picky about const because mutable shared state is a major source of bugs. Knowing a thing is const greatly reduces the number of places you have to look into if you find an inconsistent state somewhere.

• No need to call the default constructors of all members of Segment in its constructor. This is C++11, so if you'd like to emphasise that they are default initialized, you can just use the {} syntax on declaration, e.g.: Point mPoint1{};. If you do that, you can get rid of Segment's constructor.

• rumbleWidth() and laneMarkerWidth() could either be joined with the other helpers inside the unnamed namespace or could be declared as static, since those are pure functions that are not relying on any member state of Segment.