Little game showcases

[Florian3321]

I have made two little games that I want to share with you
Frogger is not yet fully finished, but maybe it helps somebody or someboy gives me feedback about the code. That would be very coool
Github-repos:
Frogger
https://github.com/florianPat/Frogger
Escaper:
https://github.com/florianPat/Escaper

[albinopapa]

You can make this a little cleaner ( not a critique, just showing a little trick )

Code: Select all

	if (direction < 0)
	{
		pos.x -= speed * dt;

		if (pos.x < 0)
		{
			direction *= -1;
			pos.x = 0;
		}
	}
	else
	{
		pos.x += speed * dt;

		if (pos.x + width > gfx.ScreenWidth)
		{
			direction *= -1;
			pos.x = gfx.ScreenWidth - width;
		}
	}

by doing this

Code: Select all

	// use the direction to determine speed and direction
	// When -1, pos.x += -(speed * dt)
	// When +1, pos.x += (speed * dt)
	pos.x += ( speed * dt * static_cast<float>(direction);
	
	// Shorthand for if/else if
	// The ternary operator?: evaluates the expression ( pos.x < 0 )
	// then either returns the value on the left of the colon (true) or right of the colon (false)
	direction = ( pos.x < 0 ) ? 1 : -1;

	// You can use min/max to clamp values
	// std::max returns the greater value
	// std::min returns the lesser value
	pos.x = std::max(0, std::min( gfx.ScreenWidth - ( width + 1 ), pos.x ) );

	// Example: 
	// minValue = 0
	// maxValue = 100
	// testValue_0 = 50
	// testValue_1 = -2
	// testValue_2 = 110
	/*
	result = std::max( minValue, std::min( maxValue, testValue_0); returns 50
	result = std::max( minValue, std::min( maxValue, testValue_1); returns 0
	result = std::max( minValue, std::min( maxValue, testValue_2); returns 100
	Std::min will return the lesser of maxValue(100) or testValue_?
	Then, std::max will test the result of std::max with the minValue and return it to result.
	*/

This is from the Lake::Float::Update method.

Something to consider

Code: Select all

Lake::Lake(float yPos, Graphics & gfx) 
	:
pos(0, yPos), 
gfx(gfx), 
boundingBox(pos, gfx.ScreenWidth, gfx.ScreenHeight / 3.0f), 
floats( numFloats ) // Initialize with numFloats
{
	/*for (int i = 0; i < numFloats; ++i)
	{
		floats.push_back(Float(yPos + i * 40, 100 /*random! #width*/, 1, gfx));
	}*/
	// With the vector initialized with the correct number of elements, you can now iterate over it using a ranged for loop.  To me it looks cleaner.
	for( auto &it : floats )
	{
		it = Float(yPos + ( i * 40), 100, 1, gfx);
	}
}

// Same with here
		/*for (auto it = floats.begin(); it != floats.end(); ++it)
		{
			it->draw();
		}*/
		for(auto &it : floats)
		{
			it.draw();
		}		

// Consider moving logic into the Update function instead of in the Draw function

Code: Select all

void Lake::draw()
{
	gfx.DrawRect(boundingBox, color);

	if (delay == numFloats)
	{
		for (auto it = floats.begin(); it != floats.end(); ++it)
		{
			it->draw();
		}
	}
	else
	{
		if( i != 0)
		{
			auto it = floats.begin(); 
			for(int z = 0; z < delay; ++z, ++it)
			{
				it->draw();
			}
		}
		/*if (i == 0)
		{
			delay++;
			i = waitTime;
		}
		else
		{
			auto it = floats.begin();
			for (int z = 0; z < delay; z++, it++)
			{
				it->draw();
			}
			i--;
		}*/
	}
}

void Lake::update(float dt)
{
	if (delay == numFloats)
	{
		for (auto it = floats.begin(); it != floats.end(); ++it)
		{
			it->update(dt);
		}
	}
	else
	{
		// Another trick using a lambda ( chili might cover these at some point )
		// Using the ternary operator?: to decide which path to take, if true
		// the lambda function is executed which increments delay and returns waitTime which
		// gets assigned to i.  If i != 0, then i - 1 is evaluated and gets assigned to i;
		// i = ( i == 0 ) ? [this](){ ++delay; return waitTime; } : i - 1;

		// This performs the same as the line above with the lambda and is probably more familiar.
		if( i == 0 )
		{
			++delay;
			i = waitTime
		}
		else
		{
			--i;
		}
		auto it = floats.begin();
		for (int z = 0; z < delay; z++, it++)
		{
			it->update(dt);
		}
	}
}

Code: Select all

std::vector<RectF> Lake::getFloatsBoundingBox()
{
	std::vector<RectF> result;

	for (auto it = floats.begin(); it != floats.end(); ++it)
	{
		result.push_back(it->getBoundingBox());
	}

	return result;
}

// Could be 
std::vector<RectF> Lake::getFloatsBoundingBox()
{
	// Initialize the vector to it's expected size
	std::vector<RectF> result(floats.size());

	// Don't be afraid to use indices when it make sense.
	// Range for loop only deals with one container	
	for( size_t i = 0; i < floats.size(); ++i )
	{
		result[i] = floats[i].getBoundingBox();
	}

	return result;
}

The only other things I would suggest is using a reference instead of a pointer in Player::Update and Player::HandlePhysik, and using more const correctness.

Other than that, your code for frogger looks clean.

Hope you can tell, I got bored and went way overboard here, so again, don't think I'm picking on you. Everything you have is fine and probably works as is. Just wanted to share some stuff that went through my mind as I looked through it.

[Florian3321]

Oh thank you very much that you looked over my code

[albinopapa]

Now that I have downloaded the code and made some of the modifications I mentioned, I'm finding a few things that I didn't when I just reviewed on GitHub.

1) There are no default constructors for Car and Lake::Float so initializing the vectors containing them causes compilation errors. To resolve, just add a default constructor Ex: Car() = default;
2) I missed the fact that in the Lake constructor, you are using the index of the loop in calculations, which doesn't happen using my suggestion. One of the problems is that you have a class member named 'i' as well, this probably isn't a good name for a class member especially if you are going to be using 'i' as an index in loops.

Here's the updated suggestion:

Code: Select all

Lake::Lake( float yPos )
	:
	pos( 0.f, yPos ),
	boundingBox( pos, 
				 static_cast< float >( Graphics::ScreenWidth ),
				 static_cast< float >( Graphics::ScreenHeight ) / 3.f ),
	floats( numFloats )
{
	// The cast is to eliminate the compiler warnings about implicit conversions
	const auto iyPos = static_cast< int >( yPos );

	// It makes sense to use this style of loop ( as you had before )
	// since you are using a counter.
	for( int count = 0; count < numFloats; ++count )
	{
		floats[ count ] = Float( iyPos + ( count * 40 ), 100, 1 );
	}
}

I also ran into an issue with variables not being initialized correctly because they were declared "out of order" in the header file.

Original

Code: Select all

	class Float
	{
		RectF boundingBox;
		Vec2 pos;
		int width;
		static constexpr int height = 40;
		int direction; //Make random
		static constexpr Color color = Colors::MakeRGB(165, 122, 11);
		static constexpr int speed = 50;

		Graphics& gfx;
	public:
		Float(int yPos, int width, int direction, Graphics& gfx);
		void update(float dt);
		void draw();
		RectF getBoundingBox();
	};

Lake::Float::Float(int yPos, int width, int direction, Graphics& gfx)
	: 
	width(width), 
	boundingBox(pos, width, height), 
	gfx(gfx), 
	direction(direction), 
	pos( ( direction < 0 ? gfx.ScreenWidth - width : 0 ), yPos )
{
}

Suggested

Code: Select all

	class Float
	{
		int width;
		int direction; //Make random
		Vec2 pos;
		RectF boundingBox;
		static constexpr int height = 40;
		static constexpr Color color = Colors::MakeRGB( 165u, 122u, 11u );
		static constexpr int speed = 50;

	public:
		Float() = default;
		Float( int yPos, int width, int direction );
		void update( float dt );
		void draw( Graphics &Gfx )const;
		RectF getBoundingBox()const;
	};

Lake::Float::Float( int yPos, int width, int direction )
	:
	width( width ),
	direction( direction ),
	pos( ( direction < 0 ) ?
		 static_cast<float>( Graphics::ScreenWidth - width ) : 0.f,
		 static_cast<float>( yPos ) ),
	boundingBox( pos,
				 static_cast<float>( width ),
				 static_cast<float>( height ) )
{
}

I'm actually kind of surprised that this compiles and runs. Storing a reference I thought makes the class uncopyable, which means it shouldn't be able to go into a vector container. Anyway, what I mean by "out of order" is the variables that you initialize in the constructor's initializer list are initialized in the order they are declared in the header file and not in the order you put them in the initializer list. This means that the boundingBox which relies on pos is being initialized before pos in the original version. Would have been the same for width, but your parameter is also called width with the same letter case, so the compiler used the passed in parameter instead of the class member.

[cameron]

References are copiable, just not how one might think. When copy/move op = is called on a class that contains a ref, It just calls the copy constructor of the object it references. So it is more like copy be value than creating a duplicate ref. When a copy constructor is called on a class that contains a ref it works like normal I believe. But I know vector calls copy op= whenever anything is removed (not at the end) and in a few other cases that I can not remember.

[albinopapa]

When copy/move op = is called on a class that contains a ref, It just calls the copy constructor of the object it references

I think you mean it calls the copy/move op= ( assignment operator ). That's kind of why I didn't think it would work since the Graphics class shouldn't be copied or copyable, but he's storing a ref to Graphics in classes that get move assigned into vectors.

[chili]

When copy/move op = is called on a class that contains a ref, It just calls the copy constructor of the object it references.

I don't think this is true. Do you have a reference for this?

This code works fine as expected: http://cpp.sh/8l5h

The copy constructor of object referenced by the member is def not being invoked. Default behavior for copy constructor is basically invoke copy operator all embedded objects, copy bits for simple data types and aggregates (including ptr/ref).

[cameron]

Sorry, my post had a typo in it. I mixed up the two things I wanted to say. I meant it would call the copy op= if copy= is called on class that contains ref. And no I don't have a specific reference it is just what I have noticed in my experience. (That's what I get for not proofreading my post I guess)

[chili]

I don't understand escape at all

Frogger looks like it has some potential! I kinda wish I had thought of it, it makes a pretty good game that you can make with just simple rectangle graphics. I was going to come here and tell you about some things that aren't working, but I just saw that it was a work in progress. Keep at it!

[albinopapa]

When copy/move op = is called on a class that contains a ref, It just calls the copy constructor of the object it references.

I don't think this is true. Do you have a reference for this?

This code works fine as expected: http://cpp.sh/8l5h

The copy constructor of object referenced by the member is def not being invoked. Default behavior for copy constructor is basically invoke copy operator all embedded objects, copy bits for simple data types and aggregates (including ptr/ref).

Is this because the default implementation would be something like ?

Code: Select all

memcpy( &b2, &b1, sizeof(B) );

By the by, this code doesn't compile in the C++ shell, but does compile and runs in VS2015 and VS2017.
http://cpp.sh/4rzs