migrate map-size related constants to new file, unmagic related constants
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
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 ?)
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: https://github.com/CleverRaven/Cataclysm-DDA/blob/730e6e395695d837f2c37aa5e4104db3de3d443b/data/json/monsters/zed_lieutenant.json#L381
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.
@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.
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.)
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 (upd: i probably did, my apologies)
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!