Banderi
commented
2 years ago Gosh, this is a LOT of stuff haha. It's gonna take me a long time to go through all this, and I'm afraid if you ask about every single compilation warning/return line out of place/etc. you'll find hundreds if not thousands, some of them from the original codebase, some left in there by debugging by me. My advice is to go right ahead and remove/correct them as you see fit, if you think they serve no purpose, otherwise ignore them as I might still use them in the future for printing stuff. The whole project is mostly in the "make it work" phase first, so things are always in flux and I did not bother with cleaning stuff up.
Regarding conventions: I have not given any thought about it yet, I just mostly go with the flow since no single convention fit perfectly for every case; but for class names, I think capital camelcase works well (ClassName) and for the actual variables I always use lowercase with underscores; things like p_ and double underscores etc. aren't harmful to have if you wish, I don't mind them, I just never do it because I'm lazy.
The setting files, user files, asset files, pak, saves, maps, scores, etc. are not fully mapped yet (likely never will be), and in the original code from Julius you can see many instances of variables like unused_f61 that denote fields that were leftover from the original game development, unused, or the purpose isn't clear yet. Some of these could even be repurposed to new things and possibly new features in the game.
I've been documenting everything that I can in the comments and my notes, but if you're curious the biggest one that you can read through is this: https://banderi.github.io/ozymandias-extra/?url=pharaoh_datasheets.ods
I think I've tried compiling with C++17 before and it failed, but I'd have to double check when I can. I've only set it to C++14 because I wanted some of the struct initialization styles that aren't available before, and maybe they don't work afterwards either. Again, I'd have to double check to be sure though! I left a comment somewhere in the code about it, though.
In all honesty I'm trying to think how to move forward from here, and how to handle PRs and such (it's also my first time) because as I said, I'm really hesitant to pour much if any more work into this one; I will start fresh, and fixing rogue warnings about return lines is really a waste of time at least from my part. But at the same time, I'm amazed that other people are finding interest in this, and I don't want to stop them from contributing! This already is a huge amount of work very quickly, I could never hope to match that myself, so I commend you, but I also likely would never be able to keep up if I need to approve every single change like all these by myself. I think maybe a good compromise would be, few PRs but big, with cursory descriptions, and so long as they don't break everything I'd be happy to do a quick test and merge?
SandroWissmann
First of all. This pull request is still a draft because two things should be done before:
Content of this pull request:
So in short I did the following:
settings.hintoclass SettingsI also run into a function
calc_boundin the settings which does exactly the same asstd::clamp(Except that std::clamp is more typesafe). So I removedcalc_boundcomplete and replaced it withstd::clamp.I realized in CMake we had only C++14 but I changed it to C++17 since it is not a big step but gives us many beenfits. Let me know if thats okay.
Some questions I also have:
Should Class names be capital or do we want to follow the old style. So is it
class Settingsor classsettings.Same question for the case. Is it
class SoundSettingorSound_settingorsound_setting. I just thing we should be consistent.I run into some wired things to discuss I will mark them here with comments.
Let me also know what you think about the style. I tried to make it as clean as possible.