Refactor Rect struct - Githubissues

rmtew / incursion-roguelike

The legendary computer game Incursion: Hall of the Goblin King!

https://incursion-roguelike.net/

Other

9 stars 1 forks source link

Refactor Rect struct #5

Closed HexDecimal closed 3 months ago

HexDecimal commented 3 months ago

Testing out a minor PR. Would be nice to have CI before doing anything too big. Related to #4

Always initialize Rect variables
Prefer member initializer lists
Return full Rect from methods since returning a reference to an internal static variable is a disaster waiting to happen.
Mark relevant methods and parameters as const

More could be done later. min/max could be replaced by std variants. Leading underscores should probably be changed.

HexDecimal commented 3 months ago

I've noticed that some of Rect's methods use inclusive ranges Rect::Within while other methods use exclusive ranges Rect::Volume. I wonder if there is a bug here since this indicates off-by-one errors.

PlaceWithin is more strange the more I look at it. PlaceWithinSafely has completely different code to do just what PlaceWithin is doing but with a padding size of 2 units. Both functions look like they'll break if the input Rect is ever too small.

rmtew commented 3 months ago

You will need to analyse the usage and verify this is safe, to make this change. I have vague memories of hoping to do a straightforward refactoring replacement of the custom string handling as one of my first changes after moving from allegro to libtcod, then I realised I could either fix gameplay bugs and polish the user experience or get dragged into refactoring all the other stuff.

I think cleaning up all this stuff would be immensely valuable for preserving Incursion for the future, if you have the time and energy. If you note all the things like this I need to double check like this when merging, I can also go over it just to make sure we preserve behaviour and don't introduce regressions.

HexDecimal commented 3 months ago

I considered rewriting PlaceWithin but I decided not to unless I could verify how it's used. I'm considering adding assert to verify that Rect's are not too small when that code is run. Git does not like the braces style and it's messing up the diffs.

My changes so far are mostly updating the syntax and not the logic. I was able to comprehend the code enough to document what it was trying to do.

Be sure to add comments to specific lines if you need them explained.

The refactoring of String can be broken down into smaller steps. I could make an attempt at refactoring it if you want me to.

rmtew commented 3 months ago

Work on what you feel makes the most difference and interests you. My own focus won't be old code or warnings, it'll be getting the game accessible to users. Web site. Maybe itch downloads. Maybe MacOS app given use of libtcod-1.7.0-x86.x86_64-MacOS.tar.gz.

HexDecimal commented 3 months ago

My own focus won't be old code or warnings, it'll be getting the game accessible to users. Web site. Maybe itch downloads. Maybe MacOS app given use of libtcod-1.7.0-x86.x86_64-MacOS.tar.gz.

I'm experienced with this as well. Mainly with setting up CMake and Vcpkg for cross-platform deployment (Windows/MacOS/Linux). I also know how to deploy directly to Itch and GitHub Releases. I can switch my focus to this if you want me to. This is likely to run into issues with Clang and GCC as the current codebase only accounts for MSVC.