ReiMerc/zombo-shooter
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

Cowd wewuw #1

New Issue
Open
opened 2025-10-27 10:02:29 +00:00 by sfja.skp · 12 comments
sfja.skp commented 2025-10-27 10:02:29 +00:00
Copy Link
No description provided.
sfja.skp commented 2025-10-27 10:07:33 +00:00
Author
Copy Link

src/main.cpp Line 8 in d46ee05126
Game game = Game();

Her ville

Game game;
// (med args)
Game game(...);
// eller
auto game = Game();

være at foretrække, da Game game = Game(); i C++-sprog betyder lav en Game og kopier/flyt den ind i Game game, hvor at det ovenfor betyder lave en Game game. I C++ er det ikke muligt at lave en uninitialized variable, udover primitives, så i Game game; er game initialized.

https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/main.cpp#L8 Her ville ```c++ Game game; // (med args) Game game(...); // eller auto game = Game(); ``` være at foretrække, da `Game game = Game();` i C++-sprog betyder lav en `Game` og kopier/flyt den ind i `Game game`, hvor at det ovenfor betyder lave en `Game game`. I C++ er det ikke muligt at lave en uninitialized variable, udover primitives, så i `Game game;` er `game` initialized.
sfja.skp commented 2025-10-27 10:10:56 +00:00
Author
Copy Link

src/Game.hpp Lines 9 to 30 in d46ee05126
class Game
{
private:
GameRenderer renderer;
Player player;
Map map;
std::thread update_thread;
bool stopping = false;
void update();
public:
Game();
~Game();
void run();
void draw() const;
};

src/Game.cpp Lines 16 to 96 in d46ee05126
void Game::update()
{
while (!stopping) {
player.update();
renderer.redraw();
map.check_bounds(player.x, player.y);
std::this_thread::sleep_for(16666us);
}
}
void Game::draw() const
{
renderer.clear_screen(0x80, 0x40, 0xFF, 0xFF);
map.draw(player.x, player.y);
player.draw();
renderer.flush();
}
void Game::run()
{
update_thread = std::thread(&Game::update, this);
while (true) {
SDL_Event e;
SDL_WaitEvent(&e);
if (e.type == SDL_QUIT) {
break;
}
if (e.type == SDL_KEYDOWN) {
switch (e.key.keysym.sym) {
case SDLK_RIGHT:
case SDLK_d:
player.x_vel = player.speed;
break;
case SDLK_LEFT:
case SDLK_a:
player.x_vel = -player.speed;
break;
case SDLK_DOWN:
case SDLK_s:
player.y_vel = player.speed;
break;
case SDLK_UP:
case SDLK_w:
player.y_vel = -player.speed;
break;
}
}
if (e.type == SDL_KEYUP) {
switch (e.key.keysym.sym) {
case SDLK_RIGHT:
case SDLK_d:
case SDLK_LEFT:
case SDLK_a:
player.x_vel = 0;
break;
case SDLK_DOWN:
case SDLK_s:
case SDLK_UP:
case SDLK_w:
player.y_vel = 0;
break;
}
}
int mouse_x, mouse_y;
SDL_GetMouseState(&mouse_x, &mouse_y);
player.angle = std::atan2(mouse_y - renderer.screen_height / 2, mouse_x - renderer.screen_width / 2);
draw();
}
}

Dette kode er ikke thread safe. Game::update() og Game::run() kan køre samtidig og lave undefined behavior. Jeg ville bruge std::mutex og std::lock_guard.

https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Game.hpp#L9-L30 https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Game.cpp#L16-L96 Dette kode er ikke thread safe. `Game::update()` og `Game::run()` kan køre samtidig og lave undefined behavior. Jeg ville bruge [`std::mutex`](https://en.cppreference.com/w/cpp/thread/mutex.html) og [`std::lock_guard`](https://en.cppreference.com/w/cpp/thread/lock_guard.html).
sfja.skp commented 2025-10-27 10:15:42 +00:00
Author
Copy Link

src/Game.hpp Lines 22 to 24 in d46ee05126
Game();
~Game();

src/Game.cpp Lines 8 to 14 in d46ee05126
Game::Game() : renderer("Zombo Shooter", 800, 450), player(&renderer), map(&renderer, 40) {}
Game::~Game()
{
stopping = true;
update_thread.join();
}

Per convention plejer man at definere små constructors of destructors i header'en. Så har man dem samme sted som default-værdierne, og man kan derved nemt se, hvordan en klasse bliver initialiseret. Samtidig gør at det kompileren altid kan inline constructor'en, som kan hjælpe med const initializering.

https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Game.hpp#L22-L24 https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Game.cpp#L8-L14 Per convention plejer man at definere små constructors of destructors i header'en. Så har man dem samme sted som default-værdierne, og man kan derved nemt se, hvordan en klasse bliver initialiseret. Samtidig gør at det kompileren altid kan inline constructor'en, som kan hjælpe med const initializering.
sfja.skp commented 2025-10-27 10:18:29 +00:00
Author
Copy Link

src/Game.hpp Line 16 in d46ee05126
std::thread update_thread;

src/Game.cpp Line 13 in d46ee05126
update_thread.join();

Du kan bruge en std::jthread istedet. Den requester join og joiner automatisk, når dens destructor bliver kaldet. På denne måde kan du slippe af med Game::~Game(). Man foretrækker oftest at undgå eksplicitte destructors, hvis man ikke behøver at free ressourcer selv.

https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Game.hpp#L16 https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Game.cpp#L13 Du kan bruge en [`std::jthread`](https://en.cppreference.com/w/cpp/thread/jthread.html) istedet. Den requester join og joiner automatisk, når dens destructor bliver kaldet. På denne måde kan du slippe af med `Game::~Game()`. Man foretrækker oftest at undgå eksplicitte destructors, hvis man ikke behøver at free ressourcer selv.
sfja.skp commented 2025-10-27 10:23:49 +00:00
Author
Copy Link

src/GameRenderer.cpp Lines 10 to 11 in d46ee05126
std::cerr << "Unable to initialize SDL" << std::endl;
exit(EXIT_FAILURE);

src/GameRenderer.cpp Lines 24 to 25 in d46ee05126
std::cerr << "Could not create window" << std::endl;
exit(EXIT_FAILURE);

src/GameRenderer.cpp Lines 31 to 32 in d46ee05126
std::cerr << "Could not create renderer" << std::endl;
exit(EXIT_FAILURE);

I 'best practice' C++ ville man istedet 1) throw exceptions her, så man kan fange dem senere, 2) delegate operationer der kan fejle til en anden funktion, eller 3) initialisere en 'invalid' GameRender, som man fx kan tjekke med en GameRender::operator bool()-funktion.

Som eksempel på exception kunne det være:

#include <exception>
#include <format>

class InitSdlFail : public std::exception {
public:
   InitSdlFail(std::string message) : message(message) {}

   const char* what() const noexcept override
   {
      return message.c_str();
   }

private:
   std::string message;
};

    if (!window) {
        throw InitSdlFail("Could not create window"); // note: ingen 'new'
	}
https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/GameRenderer.cpp#L10-L11 https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/GameRenderer.cpp#L24-L25 https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/GameRenderer.cpp#L31-L32 I 'best practice' C++ ville man istedet 1) throw exceptions her, så man kan fange dem senere, 2) delegate operationer der kan fejle til en anden funktion, eller 3) initialisere en 'invalid' `GameRender`, som man fx kan tjekke med en `GameRender::operator bool()`-funktion. Som eksempel på exception kunne det være: ```c++ #include <exception> #include <format> class InitSdlFail : public std::exception { public: InitSdlFail(std::string message) : message(message) {} const char* what() const noexcept override { return message.c_str(); } private: std::string message; }; ``` ```c++ if (!window) { throw InitSdlFail("Could not create window"); // note: ingen 'new' } ```
sfja.skp commented 2025-10-27 10:34:14 +00:00
Author
Copy Link

src/Map.hpp Line 24 in d46ee05126
Map(GameRenderer *renderer, int tile_size);

src/Map.cpp Lines 4 to 5 in d46ee05126
Map::Map(GameRenderer *renderer, int tile_size) :
renderer(renderer),

Undgå at tage 'reference'-pointers som pointers i constructors. Pass istedet by reference, og tag pointeren i initializeren.

class Map
{
private:
    GameRenderer *renderer; // stadig pointer
    // ...
public:
    Map(GameRenderer &renderer, int tile_size); // reference her
    // ...
};

Map::Map(GameRenderer &renderer, int tile_size) : // reference her
    renderer(&renderer), // dette tager addressen af variablen som referencen refererer til

På denne måde undgår man, at man kan pass nullptr i constructuren. Dog noter, at man ikke bør have references som class-members.

Dette gælder også Player::Player(GameRenderer*).

https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Map.hpp#L24 https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Map.cpp#L4-L5 Undgå at tage 'reference'-pointers som pointers i constructors. Pass istedet by reference, og tag pointeren i initializeren. ```c++ class Map { private: GameRenderer *renderer; // stadig pointer // ... public: Map(GameRenderer &renderer, int tile_size); // reference her // ... }; ``` ```c++ Map::Map(GameRenderer &renderer, int tile_size) : // reference her renderer(&renderer), // dette tager addressen af variablen som referencen refererer til ``` På denne måde undgår man, at man kan pass `nullptr` i constructuren. Dog noter, at man ikke bør have references som class-members. Dette gælder også `Player::Player(GameRenderer*)`.
sfja.skp commented 2025-10-27 10:37:10 +00:00
Author
Copy Link

src/Map.hpp Line 21 in d46ee05126
std::vector<Tile> generate_tiles(std::vector<Tile> prev_tiles) const;

src/Map.cpp Line 31 in d46ee05126
std::vector<Tile> Map::generate_tiles(std::vector<Tile> prev_tiles) const

Det ligner, at prev_tiles her burde passes som en const std::vector<...>& og ikke std::vector<...>. Ellers bliver der lavet en kopi af vector'en, hver gang man kalder generate_tiles.

https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Map.hpp#L21 https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Map.cpp#L31 Det ligner, at `prev_tiles` her burde passes som en `const std::vector<...>&` og ikke `std::vector<...>`. Ellers bliver der lavet en kopi af vector'en, hver gang man kalder `generate_tiles`.
sfja.skp commented 2025-10-27 10:39:32 +00:00
Author
Copy Link

src/Map.cpp Lines 105 to 108 in d46ee05126
switch (tiles[tile_x][tile_y]) {
case grass: renderer->draw_sprite(grass_sprite, screen_x, screen_y); break;
case path: renderer->draw_sprite(path_sprite, screen_x, screen_y); break;
}

Jeg ville nok lave et Tile-objekt eller noget lignende, som selv ved om den er grass eller path, hvorpå man bare kalder tile->draw(renderer). Switching er typisk et anti-pattern i sådanne context.

https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Map.cpp#L105-L108 Jeg ville nok lave et `Tile`-objekt eller noget lignende, som selv ved om den er `grass` eller `path`, hvorpå man bare kalder `tile->draw(renderer)`. Switching er typisk et anti-pattern i sådanne context.
sfja.skp commented 2025-10-27 10:44:57 +00:00
Author
Copy Link

src/Player.hpp Line 21 in d46ee05126
const double speed = 1.5;

Undgå const data-member. Det kan fucke med dine move-constructors. Lav den istedet static (og constexpr)

   static constexpr double speed = 1.5;
https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Player.hpp#L21 Undgå `const` data-member. Det kan fucke med dine move-constructors. Lav den istedet `static` (og `constexpr`) ```c++ static constexpr double speed = 1.5; ```
sfja.skp commented 2025-10-27 10:48:13 +00:00
Author
Copy Link

src/Tile.hpp Lines 4 to 8 in d46ee05126
enum Tile
{
grass,
path,
};

src/Map.cpp Lines 106 to 108 in d46ee05126
case grass: renderer->draw_sprite(grass_sprite, screen_x, screen_y); break;
case path: renderer->draw_sprite(path_sprite, screen_x, screen_y); break;
}

Brug enum class istedet.

enum class Tile
{
    grass,
    path,
};

                case Tile::grass: renderer->draw_sprite(grass_sprite, screen_x, screen_y); break;
                case Tile::path:  renderer->draw_sprite(path_sprite,  screen_x, screen_y); break;
https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Tile.hpp#L4-L8 https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Map.cpp#L106-L108 Brug `enum class` istedet. ```c++ enum class Tile { grass, path, }; ``` ```c++ case Tile::grass: renderer->draw_sprite(grass_sprite, screen_x, screen_y); break; case Tile::path: renderer->draw_sprite(path_sprite, screen_x, screen_y); break; ```
sfja.skp commented 2025-10-27 10:55:28 +00:00
Author
Copy Link

src/GameRenderer.hpp Line 5 in d46ee05126
#include <SDL2/SDL.h>

Dette er mest personligt: Undgå at expose SDL-headers i din headers. SDL er et C-library, der ikke bruger C++-namespaces, så din global namespaces bliver bloated med alt muligt.

Det er flere måder at undgå det på, men du kan fx gøre sådan her:

using OpaqueSdlRenderer = void;
using OpaqueSdlWindow = void;

class GameRenderer
{
private:
    OpaqueSdlRenderer *renderer;
    OpaqueSdlWindow *window;
// ...
};

Dette gælder også:

src/Sprite.hpp Line 4 in d46ee05126
#include <SDL2/SDL_render.h>

https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/GameRenderer.hpp#L5 Dette er mest personligt: Undgå at expose SDL-headers i din headers. SDL er et C-library, der ikke bruger C++-namespaces, så din global namespaces bliver bloated med alt muligt. Det er flere måder at undgå det på, men du kan fx gøre sådan her: ```c++ using OpaqueSdlRenderer = void; using OpaqueSdlWindow = void; class GameRenderer { private: OpaqueSdlRenderer *renderer; OpaqueSdlWindow *window; // ... }; ``` Dette gælder også: https://git.reim.ar/ReiMerc/zombo-shooter/src/commit/d46ee05126c8971ca659d1cade03a5a3a5aebbb4/src/Sprite.hpp#L4
sfja.skp commented 2025-10-27 10:59:12 +00:00
Author
Copy Link

https://git.reim.ar/ReiMerc/zombo-shooter/src/branch/master/Makefile#L15

Jeg ved ikke hvilke CFLAGS dit system har by default, men jeg ville tilføje en CXX_FLAGS-variabel til debugging med følgende:

CXX_FLAGS = \
	-std=c++23 \
	-Og \
	-Wall -Wextra -Wpedantic -Wconversion \
	-pedantic -pedantic-errors \
    -fsanitize=address,undefined

Og til release:

CXX_FLAGS = \
	-std=c++23 \
	-O2 \
	-Wall -Wextra -Wpedantic -Wconversion \
	-pedantic -pedantic-errors \
    -Werror \
https://git.reim.ar/ReiMerc/zombo-shooter/src/branch/master/Makefile#L15 Jeg ved ikke hvilke CFLAGS dit system har by default, men jeg ville tilføje en `CXX_FLAGS`-variabel til debugging med følgende: ```make CXX_FLAGS = \ -std=c++23 \ -Og \ -Wall -Wextra -Wpedantic -Wconversion \ -pedantic -pedantic-errors \ -fsanitize=address,undefined ``` Og til release: ```make CXX_FLAGS = \ -std=c++23 \ -O2 \ -Wall -Wextra -Wpedantic -Wconversion \ -pedantic -pedantic-errors \ -Werror \ ```
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: ReiMerc/zombo-shooter#1
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?