Various changes #3

martinbutt · 2025-02-09T23:36:01Z

Obviously this is a big change, but it's mostly moving around what was already there so I can use it as part of another project (PM3000). I didn't want to mess with your code too much, so I've kept things in the C-style where I could, without leaning too much into modern C++ (although I did end up using some C++17).

Here's an overview:

Split out the shareable code into pm3.hh / pm3.cc
Updated the formatting of player lists to match the tabular game format.
In all this tinkering, I've discovered that aggression has a disproportionate weight on a players abilities in the game. If you have a team where all players have an aggression of 9, they are pretty much unbeatable despite the rest of their stats. I've added a function to level the aggression of all players in all teams to 5.
Mapped the structure of the PREFS file and SAVES.dir file.
Fixed some minor bugs, e.g. directors_confidence_current and directors_confidence_start being reversed.
Added mapping for injuries/bans/etc.
Added support for PM3 Deluxe.
Changed the path that is passed in to be the game path and not the saves path.
Added CMake build scripts.
Added function to load the default gamedata.dat, clubdata.dat and playdata.dat.
Added mapping for kits style and color (home, away1, away2).
Added mapping for weekly league position

I know it's a lot, and if you want to ignore this, no worries, I'll just use my fork for PM3000.

Also, my IDE reformatted the code. If that's a blocker, let me know and I'll restore the original formatting as best I can.

eb4x

The PR would have been better if there were a bunch of smaller "atomic" commits, that fixed one issue at a time. That way we could go through the reasoning behind the changes, and I'd see the thought-process a bit clearer.

The C++17 std::filesystem is probably a better option than what we have today with aprinft. I'm not sure about the PATH_SEPARATOR, I think/ works just as well? (Maybe not if you're passing in \ from the commandline? i.e. game_path="C:\dosbox\pm3" and then appending /SAVES to that)

The functions reading the default game, club, player, meta and prefs look really interesting, but aren't getting used anywhere? (Update, I see they get used from PM3000)

Anyway, cool of you to check in. If you like the feedback and having your code reviewed, then you're welcome to submit PRs and I'll gladly take a look.

But if you have a vision for this code and what it can become (which it kinda seems like you do), you have my full blessing to just pursue PM3000. Which is looking great btw!

eb4x · 2025-02-10T12:51:06Z

pm3/pm3.cc

+char* append_trailing_slash(const char* path) {
+    char* appended_path = const_cast<char *>(path);


Why are you taking in a const char *, and then casting it to a char *? const is there to prevent you from modifying/reassigning it. It's mostly a memo to yourself that "I do not intend to modify this."

Good catch there. That's an oversight on my part. At some point the const made sense further up the stack, but where it finished up, it no longer did.

eb4x · 2025-02-10T12:54:39Z

pm3/pm3.cc

+    appended_path[len] = PATH_SEPARATOR;
+    appended_path[len + 1] = '\0'; // Null-terminate the string


I didn't see a malloc anywhere, so now you're writing beyond the buffer.

I think what you'd want to do something like,

size_t len = strlen(path); if (path[len - 1] == PATH_SEPARATOR) return strdup(path); char new_path = malloc(len + 2); strcpy(new_path, path); new_path[len] = PATH_SEPARATOR; new_path[len + 1] = '\0'; return new_path;

But, now you have to track the returned value, and remember to free() it when it's not needed anymore. There are perhaps cleaner ways of doing this.

Very good point. I've cleaned this up along with your other comments by moving over to use std::filessystem. I think the result is pretty clean.

eb4x · 2025-02-10T13:29:36Z

pm3/pm3.cc

+void load_binaries(int game_nr, const char *saves_path) {
+    char* full_path = append_trailing_slash(saves_path);
+
+    size_t gamexa_len = asprintf(&gamexa, "%sGAME%.1dA", full_path != nullptr ? full_path : "", game_nr);


You'll need a corresponding call to free() for every asprintf as it allocates memory.

eb4x · 2025-02-10T13:30:35Z

pm3/pm3.cc

+    fread(&gamea, sizeof (struct gamea), 1, fga);
+    fclose(fga);
+
+    size_t gamexb_len = asprintf(&gamexb, "%sGAME%.1dB", full_path != nullptr ? full_path : "", game_nr);


asprintf missing free

eb4x · 2025-02-10T13:30:45Z

pm3/pm3.cc

+    fread(&gameb, sizeof (struct gameb), 1, fgb);
+    fclose(fgb);
+
+    size_t gamexc_len = asprintf(&gamexc, "%sGAME%.1dC", full_path != nullptr ? full_path : "", game_nr);


asprintf missing free

eb4x · 2025-02-10T13:32:17Z

pm3/pm3.cc

+    fwrite(&saves, sizeof (struct saves), 1, fgs);
+    fclose(fgs);
+
+    size_t prefs_len = asprintf(&prefsx, "%sPREFS", full_path != nullptr ? full_path : "");


asprintf missing free

eb4x · 2025-02-10T13:35:11Z

main.cc

+    free(gamexa);
+    free(gamexb);
+    free(gamexc);


No longer relevant here.

eb4x · 2025-02-10T16:00:57Z

pm3/pm3.hh

+#define STANDARD_SAVES_PATH "SAVES"
+#define DELUXE_SAVES_PATH "saves"


I'm guessing this really boils down to DOS was case insensitive, and would be better handled with a function that looks up the contents of a directory and case-insensitive looks for a entry, then uses the first result.

char *case_insensitive_path(const char *path, const char *name) { char *full_path = NULL; DIR *dirp = opendir(path); struct dirent *entry = NULL; while ((entry = readdir(dirp)) != NULL) { if (0 != strcasecmp(entry->d_name, name)) continue; size_t res = asprintf(&full_path, "%s%c%s", path, PATH_SEPARATOR, entry->d_name); break; } closedir(dirp); return full_path; }

Maybe the C++17 std::filesystem can do something similar?

The filename case sensitivity doesn't seem to have a straightforward solution with modern libraries. As there are only two cases "SAVES" and "saves", I could change get_saves_folder to:

const char* get_saves_folder(const char *game_path) { if (std::filesystem::exists(std::filesystem::path(game_path) / STANDARD_SAVES_PATH)) { return STANDARD_SAVES_PATH; } else if (std::filesystem::exists(std::filesystem::path(game_path) / DELUXE_SAVES_PATH)) { return DELUXE_SAVES_PATH; } else { fprintf(stderr, "Saves folder not found\n"); exit(EXIT_FAILURE); }} }

However, I'm using the get_pm3_game_type() function (which the current get_saves_folder relies on) in PM3000, so I'm tempted to leave it as is?

eb4x · 2025-02-10T16:09:36Z

pm3/pm3.cc

+    gameb.club[new_club_idx].player_image = gameb.club[old_club_idx].player_image;
+
+	strncpy(gameb.club[new_club_idx].manager, gameb.club[old_club_idx].manager, 16);
+	strncpy(gameb.club[old_club_idx].manager, DEFAULT_MANAGER_NAME, 16);


By reading in the default values from game/player/club which you've already done, you can actually fix this to the original club manager instead of "J. Smith".

eb4x · 2025-02-10T16:10:22Z

pm3/pm3.hh

+
+#define HOME 0
+#define AWAY 1
+#define DEFAULT_MANAGER_NAME "J.Smith         "


See other comment about J. Smith.

eb4x · 2025-02-11T22:31:54Z

I've got a WIP splitting up your PR into individual commits at #4.

So, I really like these changes. I'm not sure about moving the pm3 files up a directory, but I like the idea of splitting them out into a header and a library, and just keept them at the root. (It just looks a bit weird when you've got pm3.git/pm3/pm3.{cc,h}, which seems like one too many pm3.) And so I'm keeping the dump/print functions in main.cc, as they're just there for figuring out the data structures, and don't really provide anything to the library per se.

The detection of standard vs deluxe seemed to me to be an issue of case sensitive filesystem, so I just wrote the case-insensitive search for the directory. If there's more to it, let me know?

I changed the load_default_* functions to return a pointer to a memory-allocated struct, as it probably doesn't make sense reading that into the game[abc] structs in pm3.cc if you're doing comparisons or what not? Then again, the extern structs might need some thought aswell?

I'll have a go at implementing the rest of the functions tomorrow and compiling it agains pm3k. Let me know what you think 😸

martinbutt · 2025-02-11T22:55:25Z

Your code reviews are massively appreciated. The whole point of this for me is to sharpen my C++ and learn about reverse engineering. PM3000 has been a lot of fun. If you want to try that out and give feedback, it would also be very welcome. Eventually, I'd love to do go deeper into the .exe file and make mods directly to the game code, instead of just manipulating the save games. I would also like to finish decoding the gadgets.dat to extract the image assets.

Looks like we were working on this at the same time. I'll add some comments above.

martinbutt · 2025-02-12T02:26:48Z

Just a heads up that if you are going to look at pm3k and have already pulled it, I've just pushed a refactor, so you'll want to pull down those changes first.

Various changes

cc399b0

eb4x requested changes Feb 10, 2025

View reviewed changes

Martin Butt added 5 commits February 11, 2025 10:14

Remove unnecessary const

014a693

Clean up handling of file paths

d52655f

Pull out magic strings

fc4efd9

Remove redundant PATH_SEPARATOR

4458ba8

Minor formatting change

fed25d2

eb4x mentioned this pull request Feb 11, 2025

Fixes from Martin Butt #4

Open

Martin Butt added 3 commits February 11, 2025 13:32

Remove redundant frees

36b990d

Fix variable names

48952f8

Restore original manager name when changing club

908301c

Must load metadata before saving it

174bf37

		char* append_trailing_slash(const char* path) {
		char* appended_path = const_cast<char *>(path);

		appended_path[len] = PATH_SEPARATOR;
		appended_path[len + 1] = '\0'; // Null-terminate the string

		#define STANDARD_SAVES_PATH "SAVES"
		#define DELUXE_SAVES_PATH "saves"

Various changes #3

Are you sure you want to change the base?

Various changes #3

Uh oh!

Conversation

martinbutt commented Feb 9, 2025

Uh oh!

eb4x left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eb4x commented Feb 11, 2025

Uh oh!

martinbutt commented Feb 11, 2025

Uh oh!

martinbutt commented Feb 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants