-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
base: master
Are you sure you want to change the base?
Conversation
86ac9e2
to
81d2e09
Compare
Other places to remove magic 60s: basecamp::validate_sort_points() basecamp::distribute_food() basecamp::form_storage_zones() plot_options::query_seed() (migrate constexpr |
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:
|
those are some really good notes. i'll try to poke at some of this tomorrow, tonight isn't looking good for more development. |
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. |
81d2e09
to
65f0abf
Compare
@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? |
in eoc-related cpp/h bits sounds fine, just a little TODO note somewhere so it's least on record. |
There was a problem hiding this 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
9c6f272
to
3f50837
Compare
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. |
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 |
hope i didn't mess up anything badly |
@@ -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 ); |
There was a problem hiding this comment.
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 🐶
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 ); |
npc::within_boundaries_of_camp has some very sneaky hardcoded references to the current reality bubble size. https://github.com/CleverRaven/Cataclysm-DDA/blob/master/src/npc.cpp#L2503-L2504 Same deal with overmapbuffer::find_camp, actually! |
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 useMAX_VIEW_DISTANCE
, and converted the open air transparency value to use a constexpr based onMAX_VIEW_DISTANCE
attempted to update some related comments to increase clarity in light of the structural alterations.
migrated
ACTIVITY_SEARCH_DISTANCE
usages to rely onMAX_VIEW_DISTANCE
instead.added
json.h
tocommon_types.h
to satisfy a critical dependency (tests/overmap_test.cpp
couldn't compile due tojson.h
no longer being provided via thecity.h
->coordinate.h
chain). added a todo to break out deserialize, since so many things are gettingjsoh.h
that might not actually need itDescribe 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
totests/overmap_test.cpp
, but checked with kevin and he confirmed adding it tocommon_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