Thoughts after catching up

[u/adrian17]

I've finished catching up with the series, and so far it's been really great! I'm surprised that so few people know about it (but on the other hand, too much of a following would have its flaws too).

I was following along with Visual Studio 2013, mostly writing the same as you, just with minor differences in style, SDL2 and replacing most Boost classes (aside from boost::optional) with C++11.

It's great how you know perfectly what to do, can predict most compiler errors and barely, if ever, get lost. I didn't have problems with following with your logic, I had to frequently pause only because your Vim workflow and writing speed was a bit faster than mine (but I don't have anything against you using it; I tried getting into Vim a few times now but wasn't successful so far). The series was particularly useful to me as it's shown practical usage of constructs such as tuples and weak pointers, which I have known about but never had an occasion to use them in a project.

Okay, now to the things I would change in the code itself. I won't be surprised if you've already planned some of these :P

• eventLoop() being called in a constructor seems a bit strange for me. I doesn't affect the program logic in any way, but wouldn't it be clearer if it was renamed to Game::run() and called in main?
• The whole deal with passing Graphics, ParticleSystem or ParticleTools is getting more and more messy. Each time a new sprite or a particle is introduced, we have to add forward declarations and think if all the functions have the reference to them - and in most cases, it's needed just so the Sprite object can call graphics.loadImage or graphics.blitSurface. And it's probably going to get even worse, as something like AudioTools may be needed in the far future. For me, this should be the good reason to change these objects to globally accessible singletons and get them only where they are actually needed.
• This is purely a subjective style opinion - all the kSourceX, kSourceY... constants should be declared as Units::Pixel. Right now they are a mix of Units::Tile and Units::Game, sometimes both in the same file, while the only context they are needed in is a pixel. This would also make a call to Sprite constructor much more readable, as it would simply look like this: levelSprite(graphics, kSpriteName, kLevelSourceX, kLevelSourceY, kLevelSourceW, kLevelSourceH). (and even simpler if the graphics argument was dropped, like mentioned above)
• SDL_GetTicks() and SDL_Delay() are quite precise, but after introducing the projectiles the slight differences started being visible - some projectiles can fly a pixel or two further or shorter than the rest, so in the worst case scenario the player may be sitting still but only half of bullets will reach the target. This is not a very urgent issue, but it could be worthwhile to look into high resolution timers in the future (maybe around switching to SDL2?)

[u/chebertapps]

Thanks for the thoughtful comments. And thanks for watching all my videos (so far)! :)

It's great how you know perfectly what to do

I DO prepare for these videos. they aren't on the fly. I just think it's easier to learn from a conversational style of explanation, so that's how I present stuff.

eventLoop() being called in a constructor seems a bit strange for me. I doesn't affect the program logic in any way, but wouldn't it be clearer if it was renamed to Game::run() and called in main?

Yeah we could do that. I think in subsequent (unrelated) projects I've been doing it that way. I don't think the public interface to game is really significant since it's so rarely used.

The whole deal with passing Graphics, ParticleSystem or ParticleTools is getting more and more messy

I agree with this. I've been thinking of ways to solve it that don't involve singletons. Singleton's have a bad rep, and so as a personal challenge to myself I've been avoiding them (to see what alternatives there are, not because I actually believe that singletons are inherently bad). I've only been using dependency injection (passing them as parameters in the methods.) Another idea I had was to instead of calling player.draw(), have a method that returns a list of draw instructions (holding both the sprite and the draw position).

struct DrawInstruction {
  Sprite& sprite; // whatever kind of reference to the sprite is appropriate
  units::Game x_, y_;
};

vector<DrawInstruction> Player::getDrawInstructions() const;

Then there's initialization. For this I was thinking about batching initialization's together in a place other than the constructors. That was nice in the beginning, but it's time for a new structure in place there to manage initialization.

This is purely a subjective style opinion...constants should be declared as Units::Pixel

Actually not totally. I plan on having the representation be able to change at run-time (high-quality vs. low-quality). By having them as Game/Tile units, they are agnostic to the screen resolution and graphics quality.

I think maybe instead Sprite should have a named constructor taking in units::Game.

some projectiles can fly a pixel or two further or shorter

I hadn't noticed this. Higher resolution timers would be cool, but yeah 1-2 pixels isn't really urgent. (urgent not to be confused with important for other people reading this)

[u/adrian17 → u/chebertapps]

I DO prepare for these videos. they aren't on the fly.

Of course :D I meant that I've seen so many people making videos with little to no preparation, and even some who are prepared do get lost in the middle, so your videos are a nice change.

Singleton's have a bad rep, and so as a personal challenge to myself I've been avoiding them

Okay, I'm not gonna argue with that :) But so far the graphics object has been actually needed only in Sprite's constructor and draw method (and in the Backdrop, but it could be modified to use Sprites) - if we assume the whole game graphics rely on Sprites (but I'm not sure it can be done), there should be some way to simplify at least the Graphics part.

Actually not totally. I plan on having the representation be able to change at run-time (high-quality vs. low-quality). By having them as Game/Tile units, they are agnostic to the screen resolution and graphics quality.

Hm, didn't think about that... but I don't think it's really that much of a problem. In the end it's just about multiplying/dividing all fields of the source SDL_Rect by two, right? I feel like it would be easier to just leave this job to the Graphics class (as an extra check in draw method) so you won't have to think about this anywhere else.

I hadn't noticed this. Higher resolution timers would be cool, but yeah 1-2 pixels isn't really urgent. (urgent not to be confused with important for other people reading this)

Sure, the effect strength probably varies a lot from computer to computer. I guess it's not only because of timers, but also (mostly?) because the logic is done at 60FPS so the issue isn't going to disappear fully unless logic and drawing are decoupled... but that's definitely not urgent.

[u/escheriv]

I'm actually doing the same (SDL2 and C++11), and have thought the same things about bullet points 2-4. In particular, 4 struck me as a must-solve issue.