[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] modules: reparieren von Pong
Am 19.02.2021 um 16:43 hat Nico Mayer geschrieben:
> + Mit dem Programm Pong kann man jetzt wirklich Pong spielen!
Yay!
Aber es fehlt eine Signed-off-by-Zeile, mit der du das DCO [1]
"unterschreibst". Das ist nötig, damit wir den Patch nehmen können.
[1] https://developercertificate.org/
Schau dir außerdem mal doc/codestil.txt an. Die meisten Sachen, die ich
unten anmerke, stehen dort schon drin.
> ---
> src/modules/c/pong/main.c | 362 ++++++++++++++++++++++++++++++++------
> 1 file changed, 304 insertions(+), 58 deletions(-)
>
> diff --git a/src/modules/c/pong/main.c b/src/modules/c/pong/main.c
> index 0773dc21..9d2c42fb 100644
> --- a/src/modules/c/pong/main.c
> +++ b/src/modules/c/pong/main.c
> @@ -33,15 +33,304 @@
> #include <syscall.h>
> #include <types.h>
> #include <unistd.h>
> +#include <kbd.h>
> +#include <init.h>
>
> -int main(int argc, char** argv)
> +
> +//----------------------------------------------------------------------------------
> +// Types and Structures Definition
Kommentare sollten deutsch sein.
> +//----------------------------------------------------------------------------------
> +
> +
> +
> +
Die Verteilung von Leerzeilen ist ein bisschen unsystematisch. Normal
ist nur eine Leerzeile. Um zwischen Abschnitten zu trennen, sind auch
mal zwei okay, aber vier unterhalb einer Überschrift, die auch schon
abtrennt sind ein bisschen viel.
> +typedef struct Vector2 {
> + int x;
> + int y;
> +} Vector2;
> +
> +typedef struct Player {
> + Vector2 position;
> + Vector2 size;
> + int life;
lives?
> +} Player;
> +
> +typedef struct Ball {
> + Vector2 position;
> + Vector2 speed;
> + Vector2 size;
> + bool active;
> +} Ball;
> +
> +typedef struct Key {
> + bool down;
> + bool pressed;
> +} Key;
> +
> +typedef struct Keyboard {
> + pid_t kbc_pid;
> + Key arrow_up;
> + Key arrow_down;
> + Key start;
> + Key pause;
Pause hast du glaube ich vergessen zu implementieren. ;-)
Ich finde, eine Taste zum Beenden wäre auch noch ganz schick.
> +} Keyboard;
> +
> +//------------------------------------------------------------------------------------
> +// Global Variables Declaration
> +//------------------------------------------------------------------------------------
> +
> +static const int screen_width = 640;
> +static const int screen_height = 480;
> +static const int time_per_frame = 20*1000; /* 20ms */
20 * 1000 mit Leerzeichen.
> +static const int player_max_life = 5;
> +
> +static bool game_over = false;
> +static bool pause = false;
> +
> +static Player player = {0};
> +static Ball ball = {0};
> +static Keyboard keyboard = {0};
> +
> +static video_bitmap_t *front;
video_bitmap_t* front (Stern beim Typen)
> +
> +//------------------------------------------------------------------------------------
> +// Module Functions Declaration (local)
> +//------------------------------------------------------------------------------------
> +
> +static void rpc_kbd_callback(pid_t pid, uint32_t cid, size_t data_size, void* data);
> +
> +static bool init_screen(void);
> +static bool init_keyboard(void);
> +static bool init_game(void);
> +
> +
> +static void update_player(void);
> +static void update_ball(void);
> +static void run(void);
> +static void render(void);
> +
> +static void update_key(Key* key, bool down);
> +static bool is_key_pressed(Key *key);
Key* key;
Ich glaube, die meisten von diesen Forward-Deklarationen braucht es
nicht, weil du sie sowieso erst definierst und dann benutzt (oder das
tun könntest).
> +//------------------------------------------------------------------------------------
> +// Funktion Definitions
> +//------------------------------------------------------------------------------------
> +
> +
> +static void update_key(Key* key, bool down)
> +{
> + if (!down && key->down)
> + {
Die geschweifte Klammer gehört auf die Zeile vom if. (Überall sonst
auch.)
> + key->pressed = true;
> + }
> +
> + key->down = down;
> +}
> +
> +static bool is_key_pressed(Key *key)
Key* key
> +{
> + if (key->pressed)
> + {
> + key->pressed = false;
> + return true;
> + }
> +
> + return false;
> +}
Hm, eine is_*-Funktion, die den Zustand ändert, ist überraschend. Aber
ich hab jetzt auch keinen wesentlich besseren Namensvorschlag.
> +static void run()
> +{
> + uint64_t time_last_update = get_tick_count();
> + uint64_t delta_time = 0;
> +
> + while (true)
> + {
> + delta_time += get_tick_count() - time_last_update;
> + time_last_update = get_tick_count();
Streng genommen wäre es richtiger, get_tick_count() nur einmal
aufzurufen und diesen Wert in beiden Zeilen zu benutzen.
> +
> + while (delta_time >= time_per_frame)
> + {
> + delta_time -= time_per_frame;
> + update_player();
> + update_ball();
> + }
> + render();
> + }
> +}
Diese Funktion würde ich ganz nach unten schieben (also als letztes vor
main), weil sie alles andere benutzt, aber nur von main benutzt wird.
> +static void update_player()
> +{
> + if (!pause && !game_over)
> + {
> + if (player.life <= 0)
> + {
> + game_over = true;
> + }
> +
> + if (keyboard.arrow_up.down)
> + {
> + player.position.y -= 5;
> + }
> +
> + if (keyboard.arrow_down.down)
> + {
> + player.position.y += 5;
> + }
> +
> + if ((player.position.y + player.size.y) >= screen_height)
> + {
> + player.position.y = screen_height - player.size.y;
> + }
> + else if (player.position.y <= 0)
> + {
> + player.position.y = 0;
> + }
> + }
> +}
> +
> +static void update_ball()
> +{
> +
> + if (!pause && !game_over)
> + {
> + if (is_key_pressed(&keyboard.start) && !ball.active)
> + {
> + ball.speed = (Vector2){5,0};
> + ball.active = true;
> + }
> +
> + if (ball.active)
> + {
> + ball.position.x += ball.speed.x;
> + ball.position.y += ball.speed.y;
> + }
> +
> + if ((ball.position.y + ball.size.y) >= screen_height ||
> + (ball.position.y) <= 0)
Klammern um eine einzelne Variable sind ziemlich überflüssig.
> + {
> + ball.speed.y = -ball.speed.y;
> + }
> +
> + if ((ball.position.x + ball.size.x) >= screen_width)
> + {
> + ball.speed.x = -ball.speed.x;
> + }
> +
> + // gooaaal!
> + if (ball.position.x <= 0)
> + {
> + ball.position = (Vector2){screen_width/2, screen_height/2};
> + ball.speed = (Vector2){0, 0};
> + ball.active = false;
> + player.life--;
> + }
> +
> + // collision with player
> + if (player.position.x - ball.size.x <= ball.position.x &&
> + player.position.x + player.size.x >= ball.position.x &&
position.x/y ist die linke obere Ecke, nehme ich an.
Die zweite Zeile macht Sinn: Der Schläger ragt in den Ball hinein, d.h.
der rechte Rand vom Schläger (payer.pos.x + player.size.x) ist weiter
rechts als der linke Rand vom Ball (ball.position.x).
Aber was tut die erste? Ist die für den hypothetischen Fall, dass der
Ball von links auf den Spieler zukommt?
> + player.position.y - ball.size.y <= ball.position.y &&
> + player.position.y + player.size.y >= ball.position.y)
> + {
> +
> + if (ball.speed.x < 0)
> + {
> + ball.speed.x = -ball.speed.x;
> + ball.speed.y = (ball.position.x - (player.position.y + player.size.x/2))/30;
Das sieht nach Magie aus (und nach fehlenden Leerzeichen um den
Divisionsoperator).
Ist das Absicht, dass du hier x- und y-Positionen mischst? Das könnte
aber das manchmal eher komische Abprallverhalten erklären, dass ich beim
Probieren gesehen habe.
> + }
> + }
> + }
> +}
> +
> +static void render(void)
> +{
> + /* Male background */
Wieso ist denn der Hintergrund männlich? :-P
> + libvideo_change_color(0,0,0,0);
> + libvideo_draw_rectangle(0,0,screen_width, screen_height);
Leerzeichen nach dem Komma.
> +
> + /* Male Brett */
> + libvideo_change_color(0, 255, 0, 0);
> + libvideo_draw_rectangle(player.position.x, player.position.y, player.size.x, player.size.y);
Das sind mehr als 80 Zeichen. Das gleiche gilt für einige anderen Zeilen
auch.
> +
> + /* Male Ball */
> + libvideo_change_color(0, 0xab, 0xcd, 0xef);
> + libvideo_draw_rectangle(ball.position.x, ball.position.y, ball.size.x, ball.size.y);
> +
> +
Eine Leerzeile zuviel.
> + libvideo_do_command_buffer();
> + libvideo_flip_buffers(front);
> +}
> +
> +
> +static bool init_game(void)
> +{
> +
Die hier ist auch eher unnötig.
> + /* Initialize player */
> + player.position = (Vector2){screen_width*1/10, screen_height/2};
> + player.size = (Vector2){20, screen_height/5};
> + player.life = player_max_life;
> +
> + /* Initialize ball */
> + ball.position = (Vector2){screen_width/2, screen_height/2};
> + ball.speed = (Vector2){0, 0};
> + ball.size = (Vector2){20, 20};
> + ball.active = false;
> +
> +
Genau wie diese.
> + return true;
> +}
> +
> +static bool init_keyboard(void)
> +{
> + // RPC-Handler fuer die Callbacks von KBD einrichten
> + register_message_handler(KBD_RPC_CALLBACK, rpc_kbd_callback);
> +
> + keyboard.kbc_pid = init_service_get("kbc");
> + if (!keyboard.kbc_pid) {
> + return false;
> + }
> +
> + // Callback registrtrieren bei KBC
> + return rpc_get_dword(keyboard.kbc_pid, KBD_RPC_REGISTER_CALLBACK, 0, NULL);
> +
Hier haben sich ein paar Leerzeichen in einer unnötigen Zeile
versammelt. Ich glaube, die sind von den Kommas und Operatoren oben
abgehauen.
> +}
> +
> +static void rpc_kbd_callback(pid_t pid, uint32_t cid, size_t data_size,
> + void* data)
> +{
> + uint8_t* kbd_data = data;
> +
> +
> + if ((pid != keyboard.kbc_pid) || (data_size != 2)) {
> + return;
> + }
> +
> + p();
> +
> + if (kbd_data[0] == KEYCODE_ARROW_UP)
> + {
> + update_key(&keyboard.arrow_up, !kbd_data[1]);
> + }
> + else if (kbd_data[0] == KEYCODE_ARROW_DOWN)
> + {
> + update_key(&keyboard.arrow_down, !kbd_data[1]);
> + }
> + else if (kbd_data[0] == KEYCODE_ALTGR)
> + {
> + update_key(&keyboard.start, !kbd_data[1]);
> + }
> +
> + v();
> +}
> +
> +
> +static bool init_screen()
> {
> stdout = 0;
> driver_context_t* main_context;
> main_context = libvideo_create_driver_context("vesa");
> if (!main_context) {
> fprintf(stderr, "Konnte Grafiktreiberkontext nicht erstellen\n");
> - return EXIT_FAILURE;
> + return false;
> }
> libvideo_use_driver_context(main_context);
> libvideo_get_command_buffer(1024);
> @@ -85,75 +374,32 @@ int main(int argc, char** argv)
>
> printf("Farbe gesetzt.\n");
>
> - if (libvideo_change_display_resolution(0, 640, 480, 24) != 0) {
> + if (libvideo_change_display_resolution(0, screen_width, screen_height, 24) != 0) {
> printf("Modus setzen fehlgeschlagen\n");
> - return -1;
> + return false;
> }
>
> printf("Modus gesetzt.\n");
>
> - video_bitmap_t *front;
> +
> front = libvideo_get_frontbuffer_bitmap(0);
> int buffers = libvideo_request_backbuffers(front, 2);
>
> printf("Got frontbuffer. %d Backbuffer.\n", buffers);
>
> - int angle = 0;
> - int speed = 100;
> -
> - int x = 1024/2;
> - int y = 768/2;
> -
> - int dx, dy;
> -
> - int frames = 0;
> + libvideo_change_target(front);
> +
Nochmal eine Leerzeichenversammlung.
> + return true;
> +}
Es sind zwar viele Kommentare geworden, aber eigentlich sind es nur ein
Haufen Kleinigkeiten. Die neue Variante macht definitiv mehr Spaß als
davor. ;-)
Ich weiß nicht, wie viel CPU-Zeit du tatsächlich brauchst und ob da noch
Luft ist, aber mir ist noch aufgefallen, dass mein QEMU 100% CPU frisst,
wenn pong läuft. Vielleicht hilft es was, wenn man einen Timer benutzt
und wait_for_rpc() aufruft (wie in der Implementierung für sleep(), nur
ein bisschen genauer als mit ganzen Sekunden), falls es noch gar nicht
Zeit für den nächsten Frame war? Ist aber nicht so wichtig, nur ein
Vorschlag, falls du dir das anschauen willst.
Kevin