Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace point constants with static class members #77730

Merged
merged 9 commits into from
Nov 26, 2024

Conversation

sparr
Copy link
Member

@sparr sparr commented Nov 10, 2024

Summary

Infrastructure "Replace point constants with static class members"

Purpose of change

Currently whenever someone needs one of the point constants (zero, min, max, invalid, north, north_east, etc) for a coordinate type that hasn't been needed before, they add it to coordinate_constants.h. This file has accumulated over 100 such constants, in a hodge podge with no clear pattern to which constants exist and which constants don't and when it's appropriate to add a new one. This can make the codebase confusing to someone who sees tripoint_abs_omt_foo and wonders why tripoint_rel_ms_foo doesn't exist. This also leads to scenarios where sometimes the constants are used and sometimes they are created from scratch, e.g. tripoint_bub_ms( tripoint_zero ).

Describe the solution

I have moved the constants into static members of the point and tripoint classes, then exposed them in coord_point_ob. Now every tripoint_foo_bar type has ::zero, ::min, ::max, etc. The relative types have ::north, ::north_east, etc. The relative 3d types have ::above and ::below.

I also added a helper function to check whether a [tri]point equals the ::invalid constant, which was the original goal of this effort and led me down the rabbit hole of making point and tripoint behave more compatibly.

I've attempted to update every usage, including various places that were using _min but should have been using _invalid (which didn't exist in some cases) and are now using ::invalid.

I updated the tests that use relative constants with absolute coordinate types, to instead create an absolute coordinate from a relative underlying point.

I described these constants in POINTS_COORDINATES.md.

Describe alternatives you've considered

I attempted to make the individual members conditional using SFINAE and std::enable_if_t but this led down a trail of hurdles in templating and working around compiler bugs.
I tried defining the constants inside the coord class definition which worked in clang and gcc but is rejected by MSVC.

Testing

Automated tests pass. I am playing with this change.

Additional context

I'm not sure how best to break this up into multiple PRs. I could put the old constants back, change just a few files to the new constants, PR that, then continue until they are all changed?

This PR should be squash merged; the commits are just my development travails.

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership Info / User Interface Game - player communication, menus, etc. Missions Quests and missions Bionics CBM (Compact Bionic Modules) Map / Mapgen Overmap, Mapgen, Map extras, Map display Vehicles Vehicles, parts, mechanics & interactions Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Code: Tests Measurement, self-control, statistics, balancing. [C++] Changes (can be) made in C++. Previously named `Code` Monsters Monsters both friendly and unfriendly. Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. Items: Ammo / Guns Ammunition for all kinds of weapons and these weapons themselves Melee Melee weapons, tactics, techniques, reach attack Scenarios New Scenarios, balancing, bugs with scenarios Character / World Generation Issues and enhancements concerning stages of creating a character or a world Player Faction Base / Camp All about the player faction base/camp/site Mechanics: Weather Rain, snow, portal storms and non-temperature environment Items: Archery Bows, crossbows, arrows, bolts Items: Containers Things that hold other things Code: Tooling Tooling that is not part of the main game but is part of the repo. Mechanics: Enchantments / Spells Enchantments and spells Items: Armor / Clothing Armor and clothing Appliance/Power Grid Anything to do with appliances and power grid EOC: Effects On Condition Anything concerning Effects On Condition astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Nov 10, 2024
@Procyonae
Copy link
Contributor

Adding these to POINTS_COORDINATES.md wouldn't hurt

@sparr sparr force-pushed the point_constant_static_members branch 2 times, most recently from f0022f7 to e508928 Compare November 10, 2024 16:48
@github-actions github-actions bot added the <Documentation> Design documents, internal info, guides and help. label Nov 10, 2024
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Nov 11, 2024
@sparr sparr force-pushed the point_constant_static_members branch 3 times, most recently from b82b871 to a013acf Compare November 15, 2024 00:06
@sparr sparr marked this pull request as ready for review November 15, 2024 00:07
@sparr sparr force-pushed the point_constant_static_members branch 3 times, most recently from bc4905d to 764daf8 Compare November 15, 2024 21:29
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 15, 2024
@Night-Pryanik
Copy link

@sparr could you please resolve conflicts?

@sparr
Copy link
Member Author

sparr commented Nov 24, 2024

@Night-Pryanik I've done a few rounds of conflict resolution on my two big open PRs in order to get tests to pass, but then they sit for a week without review, so I kinda burned out on it. I'd be happy to do it one more time, but I can't keep it up.

@Night-Pryanik
Copy link

I'm trying my best to review all prs that were sitting for a long time and merge them if possible. I understand it's frustrating to do a big work and got no feedback for it for weeks. In any case I appreciate the work you've done here.

@PatrikLundell
Copy link
Contributor

Unfortunately, that's what happens to large PRs. When they get large reviewers shy away from them. It certainly doesn't help that they rarely pass all tests, since there's usually at least one test that fails for unrelated reasons most times for most PRs.
I guess the moral of the story is to cut down PRs to a manageable size or be prepared to wait for weeks or months while fixing merge conflicts as a reviewer finally musters the courage and stamina to tackle the PR.
Note that I'm not blaming you: this is a common situation for me as I've been involved in a number of series of PRs of rather substantial size, and so is very familiar with the situation. As you can guess, I've ended up opting for the large and slow approach much of the time.

@NetSysFire
Copy link
Member

I'd also maybe use a different PR summary and not use None. Something like Infrastructure "Replace point constants with static class members". This is too big to just use None imo.

@sparr
Copy link
Member Author

sparr commented Nov 24, 2024

Updated the summary. Compiling and running tests locally now to check my conflict resolutions.

@sparr sparr force-pushed the point_constant_static_members branch from 764daf8 to 6f2033d Compare November 26, 2024 15:14
@github-actions github-actions bot added the Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style label Nov 26, 2024
@sparr
Copy link
Member Author

sparr commented Nov 26, 2024

Conflicts resolved, tests passed locally.

@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Nov 26, 2024
@Night-Pryanik Night-Pryanik merged commit 4589bf8 into CleverRaven:master Nov 26, 2024
18 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Appliance/Power Grid Anything to do with appliances and power grid astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Bionics CBM (Compact Bionic Modules) [C++] Changes (can be) made in C++. Previously named `Code` Character / World Generation Issues and enhancements concerning stages of creating a character or a world Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tests Measurement, self-control, statistics, balancing. Code: Tooling Tooling that is not part of the main game but is part of the repo. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling <Documentation> Design documents, internal info, guides and help. EOC: Effects On Condition Anything concerning Effects On Condition Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. Info / User Interface Game - player communication, menus, etc. Items: Ammo / Guns Ammunition for all kinds of weapons and these weapons themselves Items: Archery Bows, crossbows, arrows, bolts Items: Armor / Clothing Armor and clothing Items: Containers Things that hold other things json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display [Markdown] Markdown issues and PRs Mechanics: Enchantments / Spells Enchantments and spells Mechanics: Weather Rain, snow, portal storms and non-temperature environment Melee Melee weapons, tactics, techniques, reach attack Missions Quests and missions Monsters Monsters both friendly and unfriendly. NPC / Factions NPCs, AI, Speech, Factions, Ownership Player Faction Base / Camp All about the player faction base/camp/site Scenarios New Scenarios, balancing, bugs with scenarios Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants