[develop] Add datel cheat support from filesystem #204
Conversation
WalkthroughThis pull request introduces comprehensive cheat code support for the N64 emulator menu system. The changes include adding a new source file Changes
Sequence DiagramsequenceDiagram
participant User
participant ROMLoadMenu
participant CheatLoader
participant ROMInfo
User->>ROMLoadMenu: Select ROM
ROMLoadMenu->>ROMInfo: Check cheat settings
User->>ROMLoadMenu: Enable/Disable Cheats
ROMLoadMenu->>CheatLoader: Load Cheats
CheatLoader-->>ROMLoadMenu: Cheats Loaded/Error
ROMLoadMenu->>ROMInfo: Save Cheat Setting
Possibly related PRs
Suggested Labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I would also like to implement the following before a merge in the main branch:
|
src/menu/views/load_rom.c
Outdated
| cheat_load_err_t cheat_err = load_cheats(menu); | ||
| if (cheat_err != CHEAT_LOAD_OK) { | ||
| menu_show_error(menu, cheat_load_convert_error_message(cheat_err)); | ||
| return; |
There was a problem hiding this comment.
If cheats are not loaded, are you sure you want to return? that will stop the ROM from loading?!
There was a problem hiding this comment.
Would it be possible to display the error, but proceed with boot after a timeout ?
There was a problem hiding this comment.
I can also just set cheat_list to NULL if there is an error, but I wanted to provide the user with feedback in case something happened
There was a problem hiding this comment.
We need to think about that senario in a bit more depth then.
There was a problem hiding this comment.
An idea is to parse the cheats not at game launch, but rather when a user enables cheats for their game. This way, feedback can be provided way earlier in the timeline without hindering at the game boot process
|
@XLuma would you take issue if I make some further codestyle improvements (mainly space vs tabs) and just push them to the branch? it will require you to pull after... |
Go ahead ! |
Done. The other main thing that I can see (for code style) is wrapping I have not done it as it is too easy to get the flow incorrect. |
|
Some games that use the Expansion pak will not work with cheats. Ideally, we prompt the user before enabling cheats for any game that "Recommends" or "Requires" the expansion pak |
Technically that is what "enable code"s are for, so that needs to be part of the suggestion. |
|
@XLuma I have pushed codestyle fixes. Please check I have not broke it (especially |
| @@ -857,6 +857,8 @@ static rom_err_t save_override (path_t *path, const char *id, int value, int def | |||
|
|
|||
| if (value == default_value) { | |||
| mini_err = mini_delete_value(rom_info_ini, "custom_boot", id); | |||
| } else if (strncmp(id, "cheat_codes", strlen("cheat_codes"))) { | |||
| mini_err = mini_set_bool(rom_info_ini, NULL, id, value); | |||
There was a problem hiding this comment.
something funky going on here... need to check
There was a problem hiding this comment.
Current thinking is that this block
if (value == default_value) {
mini_err = mini_delete_value(rom_info_ini, "custom_boot", id);
} else if (strncmp(id, "cheat_codes", strlen("cheat_codes"))) {
mini_err = mini_set_bool(rom_info_ini, NULL, id, value);
} else {
mini_err = mini_set_int(rom_info_ini, "custom_boot", id, value);
}
is in the wrong place and should be seperate from the ROM info file.
Though I have not looked into where it should actually be.
There was a problem hiding this comment.
So i wrote this before I had this next idea:
Ideally, the .ini file stores an array or something like that) of cheat titles/indexes (whatever ends up working best), so that rather than having an on-off cheat, the menu will parse previously enabled cheats.
This approach would ensure errors would ahve been shown to the user at some point, and thus only working/enabled cheats are included
| @@ -15,6 +15,7 @@ static const char *image_extensions[] = { "png", "jpg", "gif", NULL }; | |||
| static const char *music_extensions[] = { "mp3", "wav", "ogg", "wma", "flac", NULL }; | |||
| static const char *controller_pak_extensions[] = { "mpk", "pak", NULL }; | |||
| static const char *emulator_extensions[] = { "nes", "smc", "gb", "gbc", "sms", "gg", "chf", NULL }; | |||
| static const char *cheat_extensions[] = {"cht", NULL}; | |||
There was a problem hiding this comment.
Are we happy with this extension name?!
There was a problem hiding this comment.
This allows users to kinda drag-and-drop files from other sources(PJ64 for ex) with only some reformatting. Totally open to other extensions though
networkfusion
changed the title
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/menu/cheat_load.c (2)
89-115: Replace customft_splitwith a standard functionThe custom string splitting function
ft_splitcan be replaced with standard or library functions for better reliability and maintainability.Consider using
strtokor a well-tested library function to split strings.
31-57: Simplify and optimize thefind_strfunctionThe
find_strfunction can be simplified to improve readability and performance.Consider simplifying the function as follows:
-static int find_str (char const *s, char c) { - int i; - int nb_str; - - i = 0; - nb_str = 0; - if (!s[0]) { - return (0); - } - while (s[i] && s[i] == c) { - i++; - } - while (s[i]) { - if (s[i] == c) { - nb_str++; - while (s[i] && s[i] == c) { - i++; - } - continue; - } - i++; - } - if (s[i - 1] != c) { - nb_str++; - } - return (nb_str); +static int count_substrings(const char *s, char c) { + int count = 0; + int in_substring = 0; + + while (*s) { + if (*s != c && !in_substring) { + in_substring = 1; + count++; + } else if (*s == c) { + in_substring = 0; + } + s++; + } + return count; }Don't forget to update calls to use
count_substringsinstead offind_str.docs/13_datel_cheats.md (3)
19-22: Add descriptions for the supported code types.The code type descriptions are missing. Please provide clear explanations for each supported code type (80, D0, Fx).
16-18: Complete the expansion pak caveats section.The caveats section about cheats and expansion paks is incomplete.
34-42: Add language specifiers to code blocks.The code blocks should specify the language for proper syntax highlighting.
Apply these changes:
-``` +```txt # Super mario 64 infinite lives 8033B21D 0064 ...-``` +```txt # Example cheat codes for the game "Majoras Mask USA" # Enable code ...Also applies to: 45-54
🧰 Tools
🪛 Markdownlint (0.37.0)
34-34: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Makefile(1 hunks)docs/13_datel_cheats.md(1 hunks)src/menu/cart_load.c(1 hunks)src/menu/cheat_load.c(1 hunks)src/menu/cheat_load.h(1 hunks)src/menu/rom_info.c(2 hunks)src/menu/rom_info.h(1 hunks)src/menu/views/file_info.c(2 hunks)src/menu/views/load_rom.c(3 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/13_datel_cheats.md
[formatting] ~24-~24: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...on here) The codes XX are not supported, because... - e.g. they rely on the button. ###...
(COMMA_BEFORE_BECAUSE)
🪛 Markdownlint (0.37.0)
docs/13_datel_cheats.md
34-34: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
46-46: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 cppcheck (2.10-2)
src/menu/cheat_load.c
[error] 152-152: Memory leak
(memleak)
[error] 158-158: Memory leak
(memleak)
🔇 Additional comments (12)
src/menu/cheat_load.c (2)
111-111: Ensurestrlcpyis portable or provide an alternativeThe function
strlcpyis not part of the C standard library and may not be available on all platforms.Check if
strlcpyis available in your development environment. If not, consider usingstrncpyor implementingstrlcpymanually.Would you like assistance in providing a portable implementation of
strlcpy?
183-184: Validate the return values ofstrtoul
strtoulmay fail or return zero for invalid inputs, which could cause unexpected behavior.Ensure to check for errors after calling
strtouland handle invalid inputs appropriately.uint32_t cheatValue1 = strtoul(splitCheat[0], NULL, 16); uint32_t cheatValue2 = strtoul(splitCheat[1], NULL, 16); +if (cheatValue1 == 0 && splitCheat[0][0] != '0') { + // Handle invalid cheat code + continue; +} +if (cheatValue2 == 0 && splitCheat[1][0] != '0') { + // Handle invalid cheat value + continue; +}src/menu/cheat_load.h (1)
26-26: Add a newline at the end of the filePOSIX standards recommend that files end with a newline character.
src/menu/views/file_info.c (1)
18-18: Confirm the cheat file extension 'cht'Ensure that the extension 'cht' for cheat files aligns with project conventions and does not conflict with other file types.
src/menu/rom_info.h (1)
250-251: LGTM!The function declaration follows the established naming convention and has appropriate parameters for managing cheat settings.
src/menu/views/load_rom.c (3)
170-185: LGTM! Error handling looks good.The function properly handles memory management and displays errors without blocking the ROM loading process, addressing the concerns raised in previous reviews.
225-229: LGTM! Menu integration is clean.The cheat options menu is properly integrated into the existing context menu system.
235-235: LGTM! Menu item placement is appropriate.The "Set Cheats" option is logically placed among other ROM settings.
src/menu/rom_info.c (1)
967-970: LGTM! Clean implementation.The function implementation is clean and follows the established pattern of other similar functions.
Makefile (1)
46-46: LGTM! Source file addition is properly structured.The new source file
menu/cheat_load.cis correctly added to the SRCS list and follows the alphabetical ordering convention within the menu section.src/menu/cart_load.c (2)
9-9: LGTM! Header inclusion is properly placed.The
cheat_load.hheader is correctly positioned after system headers and before local headers.
Line range hint
1-200: Verify integration of cheat loading functionality.While the header is included, there appears to be no integration of the cheat loading functionality in the ROM loading process. Based on the PR objectives, we should verify if cheat loading needs to be integrated here, particularly in the
cart_load_n64_rom_and_savefunction where ROM loading occurs.Let's verify the integration requirements:
| path_t *path = path_clone(menu->load.rom_path); | ||
|
|
||
| // Parse cheats from file | ||
| path_ext_replace(path, "cht"); | ||
| if((cheatsFile = fopen(path_get(path), "rb")) == NULL) { | ||
| path_free(path); | ||
| return CHEAT_LOAD_OK; // no file is not an error. | ||
| } | ||
|
|
||
| if (fstat(fileno(cheatsFile), &st)){ | ||
| path_free(path); | ||
| return CHEAT_LOAD_ERR_SIZE_FAILED; | ||
| } | ||
|
|
||
| cheatsLength = st.st_size; | ||
| if (cheatsLength <= 0) { | ||
| path_free(path); | ||
| return CHEAT_LOAD_ERR_CHEAT_EMPTY; | ||
| } | ||
| if (cheatsLength > KiB(128)) { | ||
| path_free(path); | ||
| return CHEAT_LOAD_ERR_CHEAT_TOO_LARGE; | ||
| } | ||
|
|
||
| char *cheatsContent = NULL; | ||
| if((cheatsContent = malloc((cheatsLength + 1) * sizeof(char))) == NULL) { | ||
| path_free(path); | ||
| return CHEAT_LOAD_ERR_MALLOC_FAILED; | ||
| } | ||
| if(fread(cheatsContent, cheatsLength, 1, cheatsFile) != 1) { | ||
| path_free(path); | ||
| return CHEAT_LOAD_ERR_READ_FAILED; | ||
| } | ||
|
|
||
| cheatsContent[cheatsLength] = '\0'; | ||
| if(fclose(cheatsFile) != 0){ | ||
| path_free(path); | ||
| return CHEAT_LOAD_ERR_CLOSE_FAILED; | ||
| } | ||
| cheatsFile = NULL; | ||
|
|
||
| char **tab = ft_split(cheatsContent, '\n'); | ||
| size_t lines = 1; | ||
| for (size_t i = 0; tab[i] != NULL; i++) { | ||
| lines++; | ||
| } | ||
|
|
||
| free(cheatsContent); | ||
|
|
||
| uint32_t *cheats = (uint32_t*)malloc(((lines * sizeof(uint32_t)) * 2) + 2); | ||
| memset(cheats, 0, ((lines * sizeof(uint32_t)) * 2) + 2); | ||
| size_t cheatIndex = 0; | ||
| for(size_t i = 0; tab[i] != NULL; i++) { | ||
| // ignore titles | ||
| if (tab[i][0] == '#' || tab[i][0] == '$') { | ||
| continue; | ||
| } | ||
| // ignore empty, too small or too big lines | ||
| if (strlen(tab[i]) < 12 || strlen(tab[i]) > 15) { | ||
| continue; | ||
| } | ||
| char **splitCheat = ft_split(tab[i], ' '); | ||
| uint32_t cheatValue1 = strtoul(splitCheat[0], NULL, 16); | ||
| uint32_t cheatValue2 = strtoul(splitCheat[1], NULL, 16); | ||
| cheats[cheatIndex] = cheatValue1; | ||
| cheats[cheatIndex + 1] = cheatValue2; | ||
| free_tab(splitCheat); | ||
| cheatIndex += 2; | ||
| } | ||
| free_tab(tab); | ||
|
|
||
| cheats[cheatIndex] = 0; | ||
| cheats[cheatIndex + 1] = 0; | ||
| menu->boot_params->cheat_list = cheats; | ||
|
|
There was a problem hiding this comment.
Ensure all allocated resources are freed in error paths
In the function load_cheats, some error paths do not free allocated resources like path, cheatsContent, and cheats.
Review all return statements and make sure that allocated resources are properly freed to prevent memory leaks.
Would you like me to help refactor the function to ensure all resources are correctly managed?
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 152-152: Memory leak
(memleak)
[error] 158-158: Memory leak
(memleak)
| if(fclose(cheatsFile) != 0){ | ||
| path_free(path); | ||
| return CHEAT_LOAD_ERR_CLOSE_FAILED; | ||
| } |
There was a problem hiding this comment.
Fix memory leak when fclose fails
If fclose fails, cheatsContent is not freed before returning, leading to a memory leak.
Apply this diff to fix the issue:
if(fclose(cheatsFile) != 0){
path_free(path);
+ free(cheatsContent);
return CHEAT_LOAD_ERR_CLOSE_FAILED;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(fclose(cheatsFile) != 0){ | |
| path_free(path); | |
| return CHEAT_LOAD_ERR_CLOSE_FAILED; | |
| } | |
| if(fclose(cheatsFile) != 0){ | |
| path_free(path); | |
| free(cheatsContent); | |
| return CHEAT_LOAD_ERR_CLOSE_FAILED; | |
| } |
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 158-158: Memory leak
(memleak)
| if(fread(cheatsContent, cheatsLength, 1, cheatsFile) != 1) { | ||
| path_free(path); | ||
| return CHEAT_LOAD_ERR_READ_FAILED; | ||
| } |
There was a problem hiding this comment.
Fix memory leak when fread fails
If fread fails, cheatsContent is not freed before returning, leading to a memory leak.
Apply this diff to fix the issue:
if(fread(cheatsContent, cheatsLength, 1, cheatsFile) != 1) {
path_free(path);
+ free(cheatsContent);
return CHEAT_LOAD_ERR_READ_FAILED;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(fread(cheatsContent, cheatsLength, 1, cheatsFile) != 1) { | |
| path_free(path); | |
| return CHEAT_LOAD_ERR_READ_FAILED; | |
| } | |
| if(fread(cheatsContent, cheatsLength, 1, cheatsFile) != 1) { | |
| path_free(path); | |
| free(cheatsContent); | |
| return CHEAT_LOAD_ERR_READ_FAILED; | |
| } |
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 152-152: Memory leak
(memleak)
| uint32_t *cheats = (uint32_t*)malloc(((lines * sizeof(uint32_t)) * 2) + 2); | ||
| memset(cheats, 0, ((lines * sizeof(uint32_t)) * 2) + 2); |
There was a problem hiding this comment.
Adjust the allocation size for cheats array
The calculation for the size of the cheats array may lead to insufficient memory allocation.
Update the allocation size to accommodate all cheat entries and the terminating zeros:
-uint32_t *cheats = (uint32_t*)malloc(((lines * sizeof(uint32_t)) * 2) + 2);
-memset(cheats, 0, ((lines * sizeof(uint32_t)) * 2) + 2);
+uint32_t *cheats = (uint32_t *)malloc(((lines + 1) * 2 * sizeof(uint32_t)));
+memset(cheats, 0, ((lines + 1) * 2 * sizeof(uint32_t)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint32_t *cheats = (uint32_t*)malloc(((lines * sizeof(uint32_t)) * 2) + 2); | |
| memset(cheats, 0, ((lines * sizeof(uint32_t)) * 2) + 2); | |
| uint32_t *cheats = (uint32_t *)malloc(((lines + 1) * 2 * sizeof(uint32_t))); | |
| memset(cheats, 0, ((lines + 1) * 2 * sizeof(uint32_t))); |
| for (size_t i = 0; tab[i] != NULL; i++) { | ||
| lines++; | ||
| } |
There was a problem hiding this comment.
Correct the initialization of lines
Initializing lines to 1 may lead to an incorrect count of cheat entries.
Initialize lines to 0 to accurately count the number of lines:
-size_t lines = 1;
+size_t lines = 0;
for (size_t i = 0; tab[i] != NULL; i++) {
lines++;
}Committable suggestion skipped: line range outside the PR's diff.
| } else if (strncmp(id, "cheat_codes", strlen("cheat_codes"))) { | ||
| mini_err = mini_set_bool(rom_info_ini, NULL, id, value); |
There was a problem hiding this comment.
Fix the string comparison logic.
The strncmp condition is incorrect. It returns 0 for equal strings, so the current logic is inverted.
Apply this diff to fix the logic:
- } else if (strncmp(id, "cheat_codes", strlen("cheat_codes"))) {
+ } else if (strncmp(id, "cheat_codes", strlen("cheat_codes")) == 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (strncmp(id, "cheat_codes", strlen("cheat_codes"))) { | |
| mini_err = mini_set_bool(rom_info_ini, NULL, id, value); | |
| } else if (strncmp(id, "cheat_codes", strlen("cheat_codes")) == 0) { | |
| mini_err = mini_set_bool(rom_info_ini, NULL, id, value); |
|
@XLuma the coderabbit review is AI (imperfect) some suggestions might be helpful, but they are only suggestions, and not mandatory. |
Description
This pull request adds a parser for cheat code support in the frontend. If a file named the same as the selected rom with the extension .cht is found, it will attempt to parse the file for cheat codes and place them in
menu->boot_params->cheat_listper the cheat backend API.Cheat files should be formatted this way:
The parser ignores lines that start with a '#', are under 12 characters or over 15 characters. Every other line should be valid cheat code inputs with the code on the left, and the value on the right separated by a space.
Motivation and Context
Adds some initial cheat support in the frontend to allow users to modify their games more easily, and take advantage of the backend API.
How Has This Been Tested?
Tested on real hardware with a Summercart64 and Super Mario 64.
Screenshots
Types of changes
Checklist:
Signed-off-by: XLuma
Summary by CodeRabbit
New Features
.chtcheat filesDocumentation
Bug Fixes