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

typified game.h and .cpp #78405

Merged
merged 2 commits into from
Dec 14, 2024
Merged

Conversation

PatrikLundell
Copy link
Contributor

Summary

None

Purpose of change

Replace usage of untyped coordinates with typed ones. This time focusing on game.h and game.cpp with a tangent to map and the creature class chain due to overload and default parameter clashes.

Describe the solution

Search for usage of untyped coordinates in game.h, replacing the operations found with either a typed one or an overloading typed one if dependents couldn't be migrated with a limited effort, update the usages, and remove any orphaned untyped operations. Do the same for game.cpp.

Describe alternatives you've considered

Testing

There should be no functional changes, and thus nothing in particular to test:

  • Loaded a save, walked up a ramp, jumped into a car, drove through a couple of hay bales, ran over a zombie corpse with inventory, ran into a couple of turkeys but failed to run one over, smashed into a stationary vehicle.
    Only thing noted was that the third hay bale smashing damaged the vehicle. That's probably not out of the possible.

Additional context

Don't try to do .xy() of a point variable (thinking it was a tripoint). The compiler punished me for this by failing to compile, referencing to coordinates.h and another unrelated header file, but not to the actual file where the error was made (although that file was logged as the one being compiled when the error was reported, and also, fortunately, the only one changed since the last successful compilation). Had to search for .xy() usages in the file and look for changed lines to find it. Would not recommend to try this.

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership Info / User Interface Game - player communication, menus, etc. Bionics CBM (Compact Bionic Modules) Map / Mapgen Overmap, Mapgen, Map extras, Map display Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies 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. Melee Melee weapons, tactics, techniques, reach attack Mechanics: Weather Rain, snow, portal storms and non-temperature environment Mechanics: Enchantments / Spells Enchantments and spells EOC: Effects On Condition Anything concerning Effects On Condition labels Dec 7, 2024
@github-actions github-actions bot requested a review from KorGgenT December 7, 2024 15:08
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Dec 7, 2024
@Procyonae
Copy link
Contributor

I can't remember if I've asked before but would you be against me doing some typifying? I'd probably start with the vehicle files to try to stay out of your way

@PatrikLundell
Copy link
Contributor Author

I don't MIND, but these PRs tend to spread their tentacles all over the place, frequently resulting in merge conflicts with other PRs. This is the reason I don't have more than one open at a time.

When it comes to vehicle.h/cpp I did those recently...

Thus, if you do take it on, be prepared to handle merge conflicts, as the one merged first runs a decent risk of conflicting with just about any other PR (which is also why it's risky to merge these PRs concurrently with each other or even other PRs: it's happened a fair number of times that the result doesn't compile.).

Someone (I'm horrible with names) changed coordinate constants recently, and wasn't too happy with having to keep resolving conflicts while waiting for the PR to be merged (and I ended up with a resolving task once that one was finally merged).

@Procyonae
Copy link
Contributor

When it comes to vehicle.h/cpp I did those recently...

Whoops so you did >_>

Thus, if you do take it on, be prepared to handle merge conflicts, as the one merged first runs a decent risk of conflicting with just about any other PR (which is also why it's risky to merge these PRs concurrently with each other or even other PRs: it's happened a fair number of times that the result doesn't compile.).

Yeah I'd be quite happy dealing with that I've done a number of PRs that hit large portions of the codebase I just don't want to be making things harder for you rather than easier by causing you more merge conflicts, ig the simultaneous change -> doesn't compile thing would be pretty hard to avoid too, you've just been doing amazing work on this for so long and I feel bad not helping out '^^

@PatrikLundell
Copy link
Contributor Author

If you still want to go ahead, please do. I'm only mildly annoyed by sorting out conflicts on my side, and that has to be done at times anyway.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 13, 2024
@Night-Pryanik Night-Pryanik merged commit faa60a6 into CleverRaven:master Dec 14, 2024
21 of 25 checks passed
@PatrikLundell PatrikLundell deleted the typify branch December 14, 2024 07:59
@akrieger
Copy link
Member

akrieger commented Dec 15, 2024

I think this broke the android build (which we don't run at PR time unfortunately) https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12330625228/job/34418034460#step:26:4358

@PatrikLundell
Copy link
Contributor Author

How?

The errors complains about something in a file this PR hasn't modified (and it's something I'm definitely not qualified to modify), so it wouldn't come from this PR but from master (and thus, I presume, some other PR, possible merged at the same time, but not necessarily).
Given the name of the file, I would suspect some UI stuff to be involved, so I'd recommend looking for when the file was changed.
Github says dialog_helpers.h was changed 3 weeks ago (well before this PR) by "math: standardize error severity", with I guess is the commit comment rather than the PR name.

@Procyonae
Copy link
Contributor

I think you just read it wrong, I think it just wants

sdltiles.cpp L1899

std::string get_quick_shortcut_name( const std::string &category )
{
    if( category == "DEFAULTMODE" &&
-        g->check_zone( zone_type_id( "NO_AUTO_PICKUP" ), get_player_character().pos() ) &&
+        g->check_zone( zone_type_id( "NO_AUTO_PICKUP" ), get_player_character().pos_bub() ) &&

It's just in a #if defined(__ANDROID__) which is why you and the CI didn't catch it. Idk if there are any others that also need changing though

@PatrikLundell
Copy link
Contributor Author

Thanks @Procyonae, now I get it. It wasn't what was changed, but rather what wasn't!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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` Code: Tests Measurement, self-control, statistics, balancing. 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. json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display Mechanics: Enchantments / Spells Enchantments and spells Mechanics: Weather Rain, snow, portal storms and non-temperature environment Melee Melee weapons, tactics, techniques, reach attack Monsters Monsters both friendly and unfriendly. Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants