leftiness / hex_math

MIT License
4 stars 1 forks source link

Steps along line should only be manhattan distance 1 #60

Closed leftiness closed 8 years ago

leftiness commented 8 years ago

See this test:

use std::collections::HashSet;

use hex_math::{line, Point};

let point: Point = Point::new(1, 2, 5);
let other: Point = Point::new(3, 4, 10);
let set: HashSet<Point> = line(&point, &other);

assert!(set.contains(&Point::new(1, 2, 5)));
assert!(set.contains(&Point::new(2, 2, 6)));
assert!(set.contains(&Point::new(2, 3, 8)));
assert!(set.contains(&Point::new(3, 3, 9)));
assert!(set.contains(&Point::new(3, 4, 10)));
assert_eq!(set.len(), 5);

Wrong. The manhattan distance from (1, 2, 5) to (2, 2, 6) is 2. There should not be a step with a distance of 2.

This bug causes other problems as well. I wrote the code to make ray aware of walls (#41), and I think it might need different tests if this bug didn't exist. The wall finding code is based on the idea that each step is manhattan distance 1 away from the previous step.

leftiness commented 8 years ago

asdf.

I know how to solve this now. It's just that my solution is derp.

The thing is that you need to check at each step if you're moving along the T axis. If you moved up a bunch (for example), then you need to add all the points between the 2d step with no height difference and the full step with the height difference.

But the easy way to do that is a for loop within a for loop. Like I'd basically copy paste the 20 lines in that util::line() for loop. Big meh.

Maybe I can refactor it to use a function for taking a step from A to B, including all of the intermediate steps at different heights. It would also need to return whether or not it hit a wall while adding those points at different heights. If it did, then I need to stop the main line drawing function, too.

leftiness commented 8 years ago

I wrote this note about the todo in line.rs saying that the logic is wrong:

     The distance above is usually a distance_2d(). That distance is used
     in calculating step size.

     Here, when I'm getting the upper bound for the outer for loop, it
     also works despite being a distance_2d because the outer for loop
     is the horizontal step which doesn't consider the vertical height
     difference.

     However, when checking below if steps == range, it doesn't work. It's
     comparing the true number of steps to the distance_2d/range. In the
     case of a line with range, this will work. In the case of a line
     between two points, it cuts the line short.

     To solve this, I could say match range_option, if Some(range), if
     steps == range, break. Else if none, don't break. I don't really like
     that though.

Then there's also the matter of the line function and file getting really big.