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

Refactor repeated map.ter .furn .*_at calls #77109

Merged

Conversation

sparr
Copy link
Member

@sparr sparr commented Oct 18, 2024

Summary

None

Purpose of change

A lot of places in the code call the same lookup functions for map data repeatedly. This makes some of those sections hard to read if the parameters are not short. It also results in lack of optimization in some cases

Describe the solution

Repeated calls to the map methods in question are replaced with one call and mostly const mostly references. A lot of existing calls that get copied to local vars are are converted to const references.

A dozen mapgen functions with identical has_vehicle_collision have been refactors to inherit from a new class that provides that method.

Describe alternatives you've considered

Caching more results in the map classes is an option but would be much more complex.

Testing

Automated tests pass. I've played for a while with the changes, and continue to do so.

Additional context

I skipped a few places where the repeated calls appear in the middle of a series of conditions so it would be more complex to break them out without changing the order of execution.

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership 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 [C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. Scenarios New Scenarios, balancing, bugs with scenarios Player Faction Base / Camp All about the player faction base/camp/site astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Oct 18, 2024
Copy link
Contributor

@PatrikLundell PatrikLundell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of cramming assignments into "if" statements, because it's messy, ugly, and confusing to read. I do, however, think breaking out constants prior to the "if" statements is readable and makes sense (when used more than once, of course, which is not an issue here).
If it isn't considered to make sense to break out constants before the tests (e.g. because it's a long if/else chain) I would drop the constant altogether, unless it makes sense to define it after the "if" test for usage further down. The gains are minuscule anyway.

Saw at least one place where assignment of constant resulted in removal of preexisting ugly assignment inside "if" statement. Good!

Note that I have no official capacity, so my opinions can easily be overridden.

@sparr
Copy link
Member Author

sparr commented Oct 18, 2024

@PatrikLundell I understand your concern, and share it to a lesser degree. I did decline to implement the increasingly atrocious syntax necessary to account for cases like these without performance costs:

if( foo || here.ter( p ) == x || here.ter( p ) == y ) {} else {}
if () {} else if( foo || here.ter( p ) == x || here.ter( p ) == y ) {} else {}
if () {} else if( foo || here.ter( p ) == x || bar || here.ter( p ) == y ) {} else {}

I'd be fine with getting rid of the constants-in-conditions, and I can make educated guesses whether it's more efficient to define the constant in advance (when it might not be needed at all) or make the call twice in the conditions.

@sparr sparr changed the title WIP Refactor repeated map.ter .furn .*_at calls Refactor repeated map.ter .furn .*_at calls Oct 28, 2024
@sparr sparr marked this pull request as ready for review October 28, 2024 10:26
@github-actions github-actions bot added Info / User Interface Game - player communication, menus, etc. Monsters Monsters both friendly and unfriendly. labels Nov 2, 2024
@github-actions github-actions bot requested a review from KorGgenT November 2, 2024 15:44
@sparr sparr force-pushed the reduce_ter_furn_check_duplication branch from e792b85 to 42bae77 Compare November 2, 2024 16:37
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 2, 2024
@sparr sparr force-pushed the reduce_ter_furn_check_duplication branch from 42bae77 to 964eb6a Compare November 2, 2024 23:14
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 3, 2024
@Maleclypse Maleclypse merged commit d14f093 into CleverRaven:master Nov 3, 2024
20 of 26 checks passed
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` Crafting / Construction / Recipes Includes: Uncrafting / Disassembling 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 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.

3 participants