Update and fix various line finding functions

[Lumi-Virtual]

Summary

Bugfixes "Fixes to various line finding functions"

Purpose of change

Fixes #11422, fixes #61471, and probably other similar things. Also to clean up a lot of code that uses line functions since they're kinda all over the place right now and in some places the wrong functions are used there's then more code to make them work.

Describe the solution

I have a comment further down explaining how all this works, but it's a bit complicated so essentially:

• The bresenham() function (and by extension everything that uses it) now gives a centered line by default, and has been given full 3d support.
• The find_clear_path() replacement, which I've renamed to find_line_to(), now correctly tries every relevant offset line, and either returns the center-most line that reached the target, or the line that got the furthest if none of them made it all the way.
• There's also a lot of times when map.sees() or line_to() is used but then the line is checked Again for specific conditions at each point, so I have remade some of the functions to let you specify whatever test conditions or code you want to run at each tile so that we only iterate over the line once. This also means that when we do something like find_line_to(), we can find the valid line given the test we want to apply to each tile, and not just whatever the first line the default test is for that function is returns.

Describe alternatives you've considered

There's a lot of different ways this could be done, and especially the way the replacement functions I'm making could be handled, which is why I'm making this draft!

Testing

Lots of throwing rocks in various situations. I also tested a few of the replacement reworks I made, such as the melee attack code that checks for a line spears can pass through if were using a spear, etc. So far everything works but given that I'm moving chunks of code to inside the interact function there could be stuff I missed.

Additional context

This is very much a work in progress, so please say what features and ideas of this sound useful and which don't! I'm also fairly new to C++ so I'm almost Certainly doing things wrong and would love to know how to do things right.

[Lumi-Virtual]

alright so iv refactored all the uses of line_to() to use its new function format. iv also added line_through() which basically just includes the starting tile. also also both of these are temporarily named line_*_2() just to keep track of which things needed to be refactored, and ill rename everything back to line_*() once im sure iv gone over everything. i also made some small bugfixes and structural tweaks to some of the code i was refactoring so hopefully that hasnt caused any problems. iv only done a little testing with stuff i was familiar with

feedback on changes would be nice so that i dont wast time on stuff, and if theres any other related work or changes that would be useful please let me know ^-^

and if anyone happens to be willing to test any of this id really appreciate it <3

[kevingranade]

To be blunt this is a super scary area of the code to be overhauling. I'm very encouraged by how generally conservative about changing the algorithms themselves you seem to be, so that's great.

It would put my mind at ease a great deal if you can get some tests of the specific things (monster-player visibility stuff) that you're specifically addressing working. It should be a relatively simple matter of enumerating a bunch of scenes where the player and monster are peeking at each other around obstacles and make assertions about whether they can see each other.

[Lumi-Virtual]

completely understandable, these functions do seem to be used in quite a few places that get run Frequently >w> ill try and figure out some useful tests then ^-^ i was also considering making some tests to cover the additional functionality of line_to() since i noticed a few edge cases that other implementations could easily miss.

also if it would be useful, i could throw together a visual diagram of what find_clear_path() ( which iv renamed to find_line_to() ), and by extension line_to() / bresenham(), are actually doing. i was using one frequently while writing and debugging the whole thing and it greatly helped me understand exactly what was going on, and understand why the old functions didnt quite work as intended

[Lumi-Virtual]

It would put my mind at ease a great deal if you can get some tests of the specific things (monster-player visibility stuff) that you're specifically addressing working. It should be a relatively simple matter of enumerating a bunch of scenes where the player and monster are peeking at each other around obstacles and make assertions about whether they can see each other.

@kevingranade (hope its alright to mention for a question like this)
would you still want these specific test cases if im going to create tests that are more generic but cover those cases ?
for example, the way iv implemented fine_line_to(), it should check every possible (uniquely offset) line between two points regardless of which one is the start and which one is the target (until it finds a valid line of course), so im planning on making a test that checks for that symmetry over a bunch of random starts and targets. if that test passes it should guarantee that monster-player visibility/targetability will be symmetrical too

[harakka]

if it would be useful, i could throw together a visual diagram of what find_clear_path() ( which iv renamed to find_line_to() ), and by extension line_to() / bresenham(), are actually doing. i was using one frequently while writing and debugging the whole thing and it greatly helped me understand exactly what was going on, and understand why the old functions didnt quite work as intended

That would be extremely welcome.

[Lumi-Virtual]

(i might have gone a little overboard with the explanations >w> but better to have more info than less i suppose)

essentially the way the bresenham function works is this:
find the side lengths of the right triangle our line makes (just the differences of each axis from the start to the target), and start moving along the longest one (which i call the major axis). we also make a counter that starts at 0.
for each tile along the major axis, we check to see if the counter is > 0. if it is, then we also move sideways by 1 tile (along the minor axis) and subtract 1 from the counter so that it "resets" (subtracting 1 just makes the code simpler, since we only care about when the counter crosses an integer value, but we still want to keep the fractional part when it does)
the interesting part happens here, where after each move we add the slope of the line to the counter. since the slope is just the relationship between how far we need to move in one axis to move one unit in the other, this guarantees we always end up at our target, and means we explore each tile in the line just once.

some nice properties of this function are that youre only ever doing addition and subtraction, and that you can scale everything up so that you dont even have to work with floating point numbers at all, just integers. since we want whole numbers, we can just use the numerator and denominator of the slope, essentially multiplying everything by the denominator. conveniently those values are almost always equal to the minor and major axes respectively, so we can just use them instead of calculating a slope first. (in our case we also multiply those two values by 2 just so that we can get a clean half of one of them later)

heres then, the diagram iv been using to visualize this whole process !

one pixel is one unit, and the checkerboard pattern represents the actual tiles we are moving though. we start at the leftmost purple pixel, and each purple pixel after it represents where we end up after each step in the loop. the tiles containing the purple pixels, which iv highlighted, are the ones we explore during the function and make up the line that gets returned (in addition to the target tile of course).

so, what about offsets then ?
first, if you notice, the bottom and top edges of each row of tiles represents where our counter is equal to and just above our threshold of 0, respectively. also notice that the first purple pixel is just below an edge, meaning it has a value of 0 like we would expect. so if we changed our starting offset by adding or subtracting to it, we would expect to see that purple pixel be shifted up or down along the minor axis, and you can see how shifting the line up or down might give slightly different paths. you can also change the threshold instead, which also shifts the line in the same way, since increasing the threshold means our starting purple pixel must be further away from the tile edge if our starting offset is still 0.

you might notice that the line we get with an offset and threshold of 0 doesnt look particularly centered. this is how the original implementation of the bresenham and line_to functions worked, and why they didnt give good lines. the find_line_to function (previously find_clear_path) actually almost remedies that issue, since it calculates the starting offset needed to center the line. the problem though is that it only gives you that line if it failed to find any clear path, which almost never actually happens, and by default it still checks the 0 offset 0 threshold case first and returns that if its clear, which it usually is.

the main change iv made there is that iv put the calculation for the centered starting offset inside the bresenham function, so that the line is centered by default. technically iv actually changed the threshold instead of the offset, but the effect is the same, as i mentioned earlier. this still lets you change the offset if you want to, but now whenever the line_to function is used the line is just centered by default with no hassle.

heres a diagram of what that looks like

a much nicer line :3 though you might notice the middle tile looks wonky, but thats because that tile and the tile above it which iv given a dithered texture are both correct center tiles and only one of them can be chosen. if youre just using line_to normally it will always choose the lower of the two by default, which i think is quite reasonable.

now with all of that in mind, the way iv implemented find_line_to looks like this

i have our original line as solid purple now behind all the others just to show where the center line was. each line is just a different offset, and if you look carefully and compare any two adjacent lines, youll see their path differs by only one tile, so this is the minimum number of lines required to explore every possible path. since the major axis is even, youll notice there are also an even number of lines, and the middle two are shifted away from our original center line by one pixel each. the way iv written it, it will check lines in symmetric pairs, starting with those middle two (cyan and green), then check the next outer two (blue and yellow) etc. so that by default if the path is clear you get the same center line that line_to would give, and otherwise you get the line thats as close to center as possible. if no line is valid, it returns the line that got closest to the target, instead of defaulting to the center line.

good void this was far more words than i was expecting to write >w> but i hope this helps explains things at least somewhat

[esotericist]

i'm a bit late, but i'm pretty sure what kevin was asking for here was additions to the test suite so that we have continuous integration validating that things are behaving as they are expected to behave.

[Lumi-Virtual]

adding to the test suite was my impression as well, i just wasnt sure what kinds of tests were desired and which parts they should be testing. my current plan that iv put some time into already but havnt pushed yet is just a whole bunch of tests in line_test.cpp that covers each piece of functionality of the line functions in a somewhat abstract mathematical way. what kevin described was a specific like, real use test case, and i wasnt sure if those were desired over a more direct set of tests

[kevingranade]

It would be totally fine if it were abstract and didn't involve actual monsters and players, i.e. "does drawing a line from point A to point B with an intervening obstacle at point C result in the expected line" sort of thing.

[Night-Pryanik]

Closing as stale. If you wish to continue working on this, ping me to reopen.