Feat/events by kidp8479 · Pull Request #6 · d2codex/cub3d

kidp8479 · 2025-12-08T22:01:04Z

https://www.notion.so/feature-events-2c2456d6aba380fe9d83e0edc1b6a0d4

- cleanup function that cleans all the data when the user presses ESC key or click the red cross on top of the windows - prepare stub functions for player movements, for now, while waiting raycasting part, they log their action into the console

it's very basic, I'll see what I can do better later

Copilot

Pull request overview

This PR implements event handling functionality for the cub3D game, enabling user interaction through keyboard input and window management. The implementation includes event routing, player movement/rotation stubs, cleanup logic, and a welcome ASCII art banner.

Key changes:

Event handling system with keyboard input routing (WASD for movement, arrow keys for rotation, ESC to exit)
Window close button handler and resource cleanup function
Stub implementations for player actions (forward, backward, strafe, rotate)

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/events/events_handler.c	Implements keypress and window close event handlers that route inputs to appropriate action functions
src/events/cleanup_exit.c	Handles cleanup of MLX resources and game data before program exit
src/events/player_actions_move.c	Provides stub implementations for player movement actions (forward, backward, strafe)
src/events/player_actions_rotate.c	Provides stub implementations for camera rotation actions (left, right)
src/main.c	Integrates event handlers with MLX loop and adds ASCII art welcome message
src/utils/ascii_art.c	Displays colorized cub3D logo and usage instructions at startup
includes/cub3d.h	Adds function declarations for event handling and utility functions
Makefile	Includes new source files in build configuration and fixes ASCII art formatting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-08T22:05:45Z

src/events/events_handler.c

+	game = (t_game *)param;
+	if (keycode == XK_Escape)
+		cleanup_exit(game);
+	if (keycode == XK_w || keycode == XK_W)


Missing else statement before the next condition check. After cleanup_exit(game) is called, the function will exit the program, but for code clarity and to prevent potential issues if cleanup_exit is modified in the future, line 19 should use else if instead of just if. This ensures the subsequent checks are only evaluated when Escape is not pressed.

Suggested change

if (keycode == XK_w || keycode == XK_W)

else if (keycode == XK_w || keycode == XK_W)

yes my bad, it was working do I did not payed attention

Copilot · 2025-12-08T22:05:46Z

src/events/events_handler.c

+	game = (t_game *)param;
+	cleanup_exit(game);
+	return (EXIT_SUCCESS);
+


[nitpick] Extra blank line at the end of the file. This should be removed to maintain consistency with the rest of the codebase.

ok, and I did a norm pass as well

Copilot · 2025-12-08T22:05:46Z

src/events/events_handler.c

+int	handle_keypress(int keycode, void *param)
+{
+	t_game *game;
+
+	game = (t_game *)param;
+	if (keycode == XK_Escape)
+		cleanup_exit(game);
+	if (keycode == XK_w || keycode == XK_W)
+		move_forward(game);
+	else if (keycode == XK_s || keycode == XK_S)
+		move_backward(game);
+	else if (keycode == XK_a || keycode == XK_A)
+		strafe_left(game);
+	else if (keycode == XK_d || keycode == XK_D)
+		strafe_right(game);
+	else if (keycode == XK_Left)
+		rotate_left(game);
+	else if (keycode == XK_Right)
+		rotate_right(game);
+	return (EXIT_SUCCESS);
+}


The event handling functions (handle_keypress, handle_close) lack test coverage. Given that the repository has unit tests for other functionality (see tests/unit/), these critical event handlers should also have corresponding tests to verify keycode routing, cleanup behavior, and proper handling of different input scenarios.

the test is the program

Copilot · 2025-12-08T22:05:46Z

src/events/cleanup_exit.c

+void	cleanup_exit(t_game *game)
+{
+	if (!game)
+		exit(EXIT_FAILURE);
+	if (game->img)
+		mlx_destroy_image(game->mlx, game->img);
+	if (game->win)
+		mlx_destroy_window(game->mlx, game->win);
+	if (game->mlx)
+	{
+		mlx_destroy_display(game->mlx);
+		free(game->mlx);
+	}
+	if (game->map.grid)
+		free_map(&game->map);
+	exit(EXIT_SUCCESS);
+}


The cleanup_exit function lacks test coverage. Given that the repository has unit tests for other functionality (see tests/unit/), this critical cleanup function should have tests to verify proper resource deallocation and exit behavior under different scenarios (e.g., when some resources are NULL, when all resources are allocated).

the test is the program

Copilot · 2025-12-08T22:05:46Z

src/main.c

 		return (EXIT_FAILURE);
+	print_ascii_art_hello();
 	print_map_grid(&game.map);
 	free_map(&game.map);


The map is freed here, but then mlx_loop() is started on line 24. The game loop will likely need access to the map data for rendering. This will cause a use-after-free bug when the game tries to access game.map during the render loop. The free_map() call should be removed from here since cleanup_exit() already handles freeing the map when the game exits.

Suggested change

free_map(&game.map);

there is a check in the cleanup function to prevent double free but yeah, let's get rid of the extra ligne in the main, we will need it for the norm anyway

- missing else word - extra line to free in main - norm

- now you don't have to spam the keys to advance, rotate, you keep the keys pressed and it will stop when you release the keys

new structure for key_bindings explained

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 18 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-10T11:24:02Z

src/main.c

-	free_map(&game.map);
+	setup_hooks(&game);
+	mlx_loop_hook(game.mlx, game_loop, &game);
+	mlx_loop(game.mlx);


The game loop is started but there is no cleanup of resources when the loop exits normally. The mlx_loop function can return, and when it does, allocated resources (map grid, MLX resources) should be freed. Consider adding cleanup_exit(&game); or free_map(&game.map); after line 23 to prevent memory leaks if the loop terminates normally.

Suggested change

mlx_loop(game.mlx);

mlx_loop(game.mlx);

cleanup_exit(&game);

oops my bad, a victim of the refactor and the helper function

I am a bit unsure if mlx_loop does actually return

Copilot · 2025-12-10T11:24:03Z

includes/cub3d.h

 	t_player	player;		// player data (position, direction, camera plane)
 	t_map		map;			// map data (grid, size, textures paths, colors)
+	t_keys		keys;		// tracks which keys are currently pressed
+		// === Mouse ===


[nitpick] Inconsistent indentation: the "// === Mouse ===" comment is indented with a tab character while other similar section comments (like "// === MLX ===" and "// === Game state ===") appear to start at the same indentation level. This should be aligned consistently with the other section comments.

Suggested change

// === Mouse ===

// === Mouse ===

Copilot · 2025-12-10T11:24:03Z

src/events/player_actions_rotate.c

+
+	(void)game;
+	frame_count++;
+	if (frame_count % 60 == 0)


[nitpick] Magic number: The value 60 appears to be used as a frame count divisor for debug logging throttling. Consider defining this as a named constant (e.g., DEBUG_LOG_FRAME_INTERVAL or DEBUG_PRINT_EVERY_N_FRAMES) to improve code clarity and maintainability.

it's explained in the notion card + it's only a test, nothing to see here

Copilot · 2025-12-10T11:24:04Z

src/events/player_actions_move.c

+
+	(void)game;
+	frame_count++;
+	if (frame_count % 60 == 0)


[nitpick] Magic number: The value 60 appears to be used as a frame count divisor for debug logging throttling. Consider defining this as a named constant (e.g., DEBUG_LOG_FRAME_INTERVAL or DEBUG_PRINT_EVERY_N_FRAMES) to improve code clarity and maintainability.

it's explained in the notion card + it's only a test, nothing to see here

Copilot · 2025-12-10T11:24:04Z

src/events/player_actions_move.c

+
+	(void)game;
+	frame_count++;
+	if (frame_count % 60 == 0)


[nitpick] Magic number: The value 60 appears to be used as a frame count divisor for debug logging throttling. Consider defining this as a named constant (e.g., DEBUG_LOG_FRAME_INTERVAL or DEBUG_PRINT_EVERY_N_FRAMES) to improve code clarity and maintainability.

it's explained in the notion card + it's only a test, nothing to see here

Copilot · 2025-12-10T11:24:06Z

src/events/mouse_handler.c

+	else if (delta_x < 0)
+	{
+		frame_count_left++;
+		if (frame_count_left % 60 == 0)


[nitpick] Magic number: The value 60 appears to be used as a frame count divisor for debug logging throttling. Consider defining this as a named constant (e.g., DEBUG_LOG_FRAME_INTERVAL or DEBUG_PRINT_EVERY_N_FRAMES) to improve code clarity and maintainability.

it's explained in the notion card + it's only a test, nothing to see here

Copilot · 2025-12-10T11:24:06Z

src/events/player_actions_rotate.c

+/**
+  * @brief Rotates camera to the right
+  *
+  * @param game Pointer to game structure
+*/


[nitpick] Documentation formatting: The closing part of the docstring has inconsistent indentation. It should be aligned with the opening /** for consistency with the other docstrings in this file.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2025-12-10T11:24:06Z

src/events/player_actions_move.c

+/**
+  * @brief Moves player forward in current direction
+  *
+  * @param game Pointer to game structure
+*/


[nitpick] Documentation formatting: The closing part of the docstring has inconsistent indentation. It should be aligned with the opening /** for consistency with the other docstrings in this file.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2025-12-10T11:24:06Z

src/events/player_actions_move.c

+/**
+  * @brief Moves player backward (opposite of current direction)
+  *
+  * @param game Pointer to game structure
+*/


[nitpick] Documentation formatting: The closing part of the docstring has inconsistent indentation. It should be aligned with the opening /** for consistency with the other docstrings in this file.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2025-12-10T11:24:07Z

src/events/player_actions_move.c

+/**
+  * @brief Strafes player to the left (perpendicular to direction)
+  *
+  * @param game Pointer to game structure
+*/


[nitpick] Documentation formatting: The closing part of the docstring has inconsistent indentation. It should be aligned with the opening /** for consistency with the other docstrings in this file.

Copilot · 2025-12-10T13:01:47Z

@kidp8479 I've opened a new pull request, #8, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2025-12-10T13:02:13Z

@kidp8479 I've opened a new pull request, #9, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2025-12-10T13:02:32Z

@kidp8479 I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2025-12-10T13:02:44Z

@kidp8479 I've opened a new pull request, #11, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2025-12-10T13:02:56Z

@kidp8479 I've opened a new pull request, #12, to work on those changes. Once the pull request is ready, I'll request review from you.

- remove const on a struct (no need) - add doxygen com

kidp8479 added 10 commits December 8, 2025 16:19

feature: add new events functions to header file

10373cd

feature: I know I can't help it: ASCII ART time

3ed4d1c

it's very basic, I'll see what I can do better later

feature: add ascii art call to the main

0037c72

feature: add new functions signature to makefile

0fb5093

Merge branch 'main' into feat/events

3707c9f

feature: update header with handlers functions

89f49a6

feature: implement events handler and player stub movements

6408707

feature: fix color in ascci art

c0157a4

feature: implement key hook, cross hook and mlx loop

b3f338b

kidp8479 requested review from Copilot and d2codex December 8, 2025 22:01

Copilot started reviewing on behalf of kidp8479 December 8, 2025 22:01 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

kidp8479 added 7 commits December 8, 2025 23:13

fix: fix issues in PR

7e48b6d

- missing else word - extra line to free in main - norm

refactor: change logic to movement reacting to key being released

3421e77

- now you don't have to spam the keys to advance, rotate, you keep the keys pressed and it will stop when you release the keys

refactor: function pointers and no more if/else forest

9c8ca36

refactor: add documentation

953f75e

new structure for key_bindings explained

feature: mouse bonus implementation

225b246

feature: update makefile with mouse handling functions

7bb1d01

Merge branch 'main' into feat/events

d7739cf

kidp8479 requested a review from Copilot December 10, 2025 11:15

Copilot started reviewing on behalf of kidp8479 December 10, 2025 11:16 View session

Copilot AI reviewed Dec 10, 2025

View reviewed changes

Copilot AI mentioned this pull request Dec 10, 2025

[WIP] Address feedback on feat/events pull request #8

Closed

4 tasks

Copilot AI mentioned this pull request Dec 10, 2025

[WIP] Address feedback on 'Feat/events' pull request #9

Closed

5 tasks

Copilot AI mentioned this pull request Dec 10, 2025

[WIP] Address feedback on Feat/events pull request #10

Closed

13 tasks

This was referenced Dec 10, 2025

[WIP] Address feedback on Feat/events pull request #11

Closed

[WIP] Address feedback on events pull request #12

Closed

kidp8479 added 2 commits December 10, 2025 14:09

fix: add missing cleanup function in main

93d2320

fix: last fix before merge

dda6de3

- remove const on a struct (no need) - add doxygen com

kidp8479 merged commit e551204 into main Dec 10, 2025

kidp8479 deleted the feat/events branch December 10, 2025 15:06

	if (keycode == XK_w \|\| keycode == XK_W)
	else if (keycode == XK_w \|\| keycode == XK_W)

Conversation

kidp8479 commented Dec 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025