CleverRaven / Cataclysm-DDA

Cataclysm - Dark Days Ahead. A turn-based survival game set in a post-apocalyptic world.
http://cataclysmdda.org
Other
9.73k stars 4.05k forks source link

Made inbounds use map parameters #73503

Closed PatrikLundell closed 1 week ago

PatrikLundell commented 2 weeks ago

Summary

None

Purpose of change

Make 'inbounds' use the map parameters rather than hard coded overloaded operations using those same values.

Describe the solution

Changed the map::inbounds operations to check against the map's own parameters rather than hard coded limits that are the same as the map's at the time the code is written. This means tinymap can just call the map operations rather than overload them.

Removed confusing hard coded open cuboid checks against OVERMAP_HEIGHT +1 and replaced these with map::inbounds checks instead. If the reality bubble is expanded in the future, there's one less bug to find (looks like somebody really wanted to find a use for the cuboid stuff).

Removed a couple of weird usages of inbounds_z, where they really were used as "has the value changed from the initial value?" checks, which just is confusing.

Describe alternatives you've considered

Change inbounds_z to return false if the Z value isn't the map's Z value when Z levels aren't supported and use that in the inbounds checks. Reverted that because there might be some flaky tinymap using code that does stuff with caches anyway when the Z level is off (and it was a mistake to commit the changed code originally).

Testing

Load a save and walk around a bit. The changes should make no functional difference.

Additional context

PatrikLundell commented 2 weeks ago

Well, that was unexpected...

Testing blows up because the test code expects access to other Z levels to return the value of the current Z level for maps that don't support Z levels, i.e. that the Z level is ignored.

Another test (presumably: can't find the possible reference to a test case among all the error output) expects to get away with setting furniture at the current Z level while writing to a different one (again, it expects the inbounds test to ignore the Z level parameter, as well as the code itself further on).

Utterly disgusting, in my opinion.

I can't say if it's only testing (which can be changed) or if this disgusting use in also practiced somehow for some bizarre reason.