Cataclysm-DDA icon indicating copy to clipboard operation
Cataclysm-DDA copied to clipboard

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

Open esotericist opened this issue 2 years ago • 10 comments

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 avatar Nov 13 '23 03:11 esotericist

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 ?)

RenechCDDA avatar Nov 13 '23 20:11 RenechCDDA

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

RenechCDDA avatar Nov 13 '23 21:11 RenechCDDA

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

esotericist avatar Nov 14 '23 00:11 esotericist

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[bot] avatar Dec 14 '23 02:12 github-actions[bot]

@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?

esotericist avatar Apr 25 '24 07:04 esotericist

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

RenechCDDA avatar Apr 25 '24 12:04 RenechCDDA

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 avatar May 21 '24 12:05 Brambor

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

Brambor avatar Sep 09 '24 16:09 Brambor

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

esotericist avatar Sep 09 '24 21:09 esotericist

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

GuardianDll avatar Sep 10 '24 07:09 GuardianDll

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!

RenechCDDA avatar Nov 17 '24 17:11 RenechCDDA