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

migrate map-size related constants to new file, unmagic related constants #69324

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

esotericist
Copy link
Contributor

@esotericist esotericist commented Nov 13, 2023

Summary

Infrastructure "migrate map-size related constants to new file, unmagic related constants"

Purpose of change

during my adventures in attempting to increase the reality bubble size for extra experimental purposes, i came across a handful of hardcoded values that were clearly intended to be directly related to the viewing distance assumed by the game (60), when we actually have an already-extant constexpr that evaluates to this value

Describe the solution

migrated many map-size related constants to map_scale_constants.h, updated the appropriate include dependencies, convert several hardcoded values to use MAX_VIEW_DISTANCE, and converted the open air transparency value to use a constexpr based on MAX_VIEW_DISTANCE

attempted to update some related comments to increase clarity in light of the structural alterations.

migrated ACTIVITY_SEARCH_DISTANCE usages to rely on MAX_VIEW_DISTANCE instead.

added json.h to common_types.h to satisfy a critical dependency (tests/overmap_test.cpp couldn't compile due to json.h no longer being provided via the city.h -> coordinate.h chain). added a todo to break out deserialize, since so many things are getting jsoh.h that might not actually need it

Describe alternatives you've considered

leaving hardcoded and magic numbers in place, slightly confusing and complicating reality bubble related elements in the future?

some of the values i changed i'm less sure about than others; many of the volume values seem to clearly be intended "you can hear this from anywhere in the reality bubble" (due to using a volume of 60), but i'm also not sure if those reasonably should be heard anywhere in the reality bubble.

considered not migrating ACTIVITY_SEARCH_DISTANCE, but a single constant seemed to make more sense.

previously i tried adding json.h to tests/overmap_test.cpp, but checked with kevin and he confirmed adding it to common_types.h is the more correct (if unfortunate) choice for now.

Testing

built locally, made sure game worked correclty, also locally ran all of the vision tests, no regressions found.

no perceptible impact to game behavior observed; this is a simple refactoring so nothing should have changed

Additional context

i'm pretty sure a bunch of the tests are gonna shatter really hard once we actually get around to adjusting the reality bubble, due to them using hardcoded coordinates. that's future us problems, though

@esotericist esotericist added Map / Mapgen Overmap, Mapgen, Map extras, Map display [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Nov 13, 2023
@github-actions github-actions bot added Code: Tests Measurement, self-control, statistics, balancing. astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Nov 13, 2023
@RenechCDDA
Copy link
Member

RenechCDDA commented Nov 13, 2023

Other places to remove magic 60s:

basecamp::validate_sort_points()

basecamp::distribute_food()

basecamp::form_storage_zones()

plot_options::query_seed()

(migrate constexpr ACTIVITY_SEARCH_DISTANCE in clzones.h ?)

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 13, 2023
@RenechCDDA
Copy link
Member

RenechCDDA commented Nov 13, 2023

One more: The shadow EOCs reference 60 (as the reality bubble constant), so some sort of TODO note stating that EOCs need to be able to hook into them might be warranted. Not asking you to do the work of actually hooking it up here, it's out of scope.

example:

{ "math": [ "u_monsters_nearby('mon_lieutenant_shadow', 'radius': 60)", ">", "0" ] }

@esotericist esotericist marked this pull request as draft November 14, 2023 00:33
@esotericist
Copy link
Contributor Author

those are some really good notes. i'll try to poke at some of this tomorrow, tonight isn't looking good for more development.

Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not bump or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@github-actions github-actions bot added the stale Closed for lack of activity, but still valid. label Dec 14, 2023
@Maleclypse Maleclypse added (P5 - Long-term) Long-term WIP, may stay on the list for a while. and removed stale Closed for lack of activity, but still valid. labels Dec 31, 2023
@esotericist
Copy link
Contributor Author

@RenechCDDA back on the train for this. for that proposed todo, where are you suggesting that live? in the eoc json? in eoc-related cpp/h bits?

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership Info / User Interface Game - player communication, menus, etc. Vehicles Vehicles, parts, mechanics & interactions Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. Player Faction Base / Camp All about the player faction base/camp/site labels Apr 25, 2024
@RenechCDDA
Copy link
Member

in eoc-related cpp/h bits sounds fine, just a little TODO note somewhere so it's least on record.

@esotericist esotericist marked this pull request as ready for review April 25, 2024 16:11
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Auto-requesting reviews from non-collaborators: @andrei8l

also added a todo comment because this is unfortunate
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 25, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 25, 2024
@Brambor
Copy link
Contributor

Brambor commented May 21, 2024

I am for merging the constant definition so that new code can use it already.

Later, the constants can be picked up one by one and it would be "Good First Issue". Anyone can do it once the constant is defined.

@Brambor
Copy link
Contributor

Brambor commented Sep 9, 2024

Once again, I think it is beneficial to merge this. (Maybe restart the tests just to be sure, but not restarting is fine too.)

@esotericist
Copy link
Contributor Author

Once again, I think it is beneficial to merge this. (Maybe restart the tests just to be sure, but not restarting is fine too.)

i'm going to try rebasing this in the coming days, and look for any new magic numbers that have slipped through in the interim

@GuardianDll
Copy link
Member

GuardianDll commented Sep 10, 2024

hope i didn't mess up anything badly
(upd: i probably did, my apologies)

@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Sep 10, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Sep 10, 2024
@@ -308,7 +308,7 @@ plot_options::query_seed_result plot_options::query_seed()
zone_manager &mgr = zone_manager::get_manager();
map &here = get_map();
const std::unordered_set<tripoint_abs_ms> zone_src_set =
mgr.get_near( zone_type_LOOT_SEEDS, here.getglobal( player_character.pos_bub() ), 60 );
mgr.get_near( zone_type_LOOT_SEEDS, here.getglobal( player_character.pos_bub() ), MAX_VIEW_DISTANCE );
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
mgr.get_near( zone_type_LOOT_SEEDS, here.getglobal( player_character.pos_bub() ), MAX_VIEW_DISTANCE );
mgr.get_near( zone_type_LOOT_SEEDS, here.getglobal( player_character.pos_bub() ),
MAX_VIEW_DISTANCE );

@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tests Measurement, self-control, statistics, balancing. 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 NPC / Factions NPCs, AI, Speech, Factions, Ownership (P5 - Long-term) Long-term WIP, may stay on the list for a while. Player Faction Base / Camp All about the player faction base/camp/site Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants