Generalizing lines algorithms for three dimensions, refactoring. #6560

ClockworkZombie · Mar 11, 2014

Pretty much went through the lines code and added overloads for tripoint arguments and vectors, stripped out and cleaned up some other stuff as well.

There are some dumb cleanup things in here as well, I'll pull them out later but I keep meaning to submit this stuff and never get around to it so hopefully I'll get some churn on it by posting. Specifically, itype was touched unintentionally. Any input is welcomed.

I did remove a lot of the old logic for line_to calculation as it seemed inelegant and on a modern processor with a modern compiler you're probably not going to notice much of a difference. I've not benchmarked anything though.

Lain- · Mar 12, 2014

Did I smell z-level? I don't understand a thing in this but I have hope for this.

kevingranade · Mar 12, 2014

kevingranade reviewed Mar 12, 2014

View changes

kevingranade · Mar 12, 2014

src/line.cpp

+//probably slow, so leaving two full functions for now
+//note that there is not currently a notion of "which" B-line
+//is used anymore, line_to will return the same line every time.
+//90+% of callers default to zero anyways.  Haven't yet tested this.


I'll have to double-check, but I'm pretty sure that 1% really needs it. There's a feature that checks the non-default line in case there's a clear path when aiming a gun.

Went through and checked all callers- there are two that actually use this, the function that creates rifts in the map and the function that decides where a tree drops. In every other case, the variable used for t is explicitly initialized to 0 or left default for int (also 0). iirc, shooting around corners also only works in one direction (since .9 at least).

-edit- blergh... so much for that. by-reference mod functions for single integer values without documentation. Everywhere. (>.<). Good thing I'm an optimist!

kevingranade · Mar 12, 2014

kevingranade reviewed Mar 12, 2014

View changes

kevingranade · Mar 12, 2014

src/line.cpp

 std::vector <point> line_to(const int x1, const int y1, const int x2, const int y2, int t)
 {
    std::vector<point> ret;
    // Preallocate the number of cells we need instead of allocating them piecewise.
-    ret.reserve( square_dist( x1, y1, x2, y2 ) );


I'll have to test to make sure, but I'm fairly certain this is not ok, there's a reason this is super crazy and integer-based, it's used MANY times per turn.

I compiled on my end and didn't notice any particular difference running around the map, but I hadn't tried sleeping or anything. Not sure if that would accelerate things. In my experience, modern PCs really won't show a difference on a scale noticeable in a game like this. In many cases floating -point operations are even faster, depending on the processor. I'm not sure why this isn't mergeable, but if someone wouldn't mind pulling it and pointing their profiling tool of choice at it I'd really appreciate it.

-edit- looked up where the original algorithm was coming from, turns out it was described by Bresenham back in the early 60s. At that point, yeah, there was probably a massive difference.

There's still a massive difference, CPU ALUs haven't actually changed that much, FP operations are still much slower than integer. Feel free to prove me wrong with a benchmark.

Granted, my inclination was that the affect wouldn't be noticeable - after checking it looks like this is still enough of a bottleneck during map loading/transitions that the optimization is worthwhile. I've rewritten the 3D lines algorithm to use integer logic similar to the old function.

kevingranade · Mar 12, 2014

kevingranade reviewed Mar 12, 2014

View changes

kevingranade · Mar 12, 2014

src/line.cpp

+
+int trig_dist(const tripoint loc1, const tripoint loc2)
+{
+    return int( sqrt( double( pow((loc1.x - loc2.x), 2) + pow((loc1.y - loc2.y), 2) + pow((loc1.z - loc2.z), 2))));


We tested this, x * x is faster than pow( x, 2 )

Good to know- I'll go and update this. Any info on how much faster?

Zireael07 · Mar 13, 2014

does a happy dance

Good to see work on z-levels being done!

ClockworkZombie · Mar 15, 2014

I think this should be set unless anyone objects to the changes or there are errors I didn't catch. I was able to make some minor optimizations to the 2D line_to algorithm as well. It's not much, but it's something.

kevingranade · Mar 15, 2014

kevingranade reviewed Mar 15, 2014

View changes

kevingranade · Mar 15, 2014

src/line.cpp

+    ret.reserve(numCells);
+    const int dx = x2 - x1, dy = y2 - y1;
+    // Any ideas why we're multiplying the abs distance by two here?
+    const int ax = abs(dx) * 2, ay = abs(dy) * 2;


We can hope that the compiler will sub in << 1 for * 2, so that's fine, but cramming these onto one line is a style regression, do one operation per line as a default.

kevingranade · Mar 15, 2014

kevingranade reviewed Mar 15, 2014

View changes

kevingranade · Mar 15, 2014

src/line.cpp

+    ret.reserve(numCells);
+    tripoint cur;
+    cur = loc1;
+    const int dx = loc2.x - loc1.x, 


This is better than putting the on one line, but I'd rather they all be individual statements.

kevingranade · Mar 15, 2014

kevingranade reviewed Mar 15, 2014

View changes

kevingranade · Mar 15, 2014

src/line.cpp

-    xmax += abs(dx);
-    ymax += abs(dy);
-
+    // The old version of this algorithm would generate points on the line and check min/max for each point


Have you tested this for speed? It's very likely faster, but I'd like to see evidence of that.

I went ahead and did some in-executable benchmarking, I don't have access to advanced profiling tools or the time/will to figure out the free stuff at this moment.

From my tests, there's basically no difference (+/- < .05%) for lines shorter than 5 tiles or so, it looks like there is a ~.08% improvement for lines over 10 tiles. I only tested up to 20-tile lines. I didn't take the time to reserve a core or anything so I wouldn't say this is in any way definitive.

Interestingly enough, it looks like the "new" 3D lines_to is something like 8-9% faster than the old function when dz is 0, at least on my machine. No idea why that is, but I'm getting ~950000 calls completed in 10 seconds, rather than ~860000 for either version of the 2D function. Maybe it's my compiler, maybe it's that the tripoint struct is in contiguous memory, maybe it's something else entirely. Either way, that seems more significant.

kevingranade · Mar 15, 2014

kevingranade reviewed Mar 15, 2014

View changes

kevingranade · Mar 15, 2014

src/line.cpp

+
+int trig_dist(const tripoint loc1, const tripoint loc2)
+{
+    return int (sqrt(double((loc1.x - loc2.x) * (loc1.x - loc2.x))


Trailing operators at the end of the line please, otherwise it looks like the statement is ending.

ClockworkZombie · Mar 16, 2014

Tweaked per review (thanks Kevin!), got rid of all the unintended tabs.

kevingranade · Mar 17, 2014

kevingranade reviewed Mar 17, 2014

View changes

kevingranade · Mar 17, 2014

src/line.cpp

@@ -1,47 +1,38 @@
 #include "line.h"
 #include "game.h"
 #include <stdlib.h>
+#include <tuple>


Missed this previously, the compiler caught it. Tuple is C++11 only. :(
We haven't migrated to C++11 due to compiler compatibility.

Shucks- that'll make replacing all of the Pairs used throughout the map stuff more awkward moving forwards since there's not really a direct replacement, would your preference be a new struct, three-element arrays, nested pairs, or something else entirely? Might as well define that now.

Anything wrong with tripoint?

For a lot of cases that'll probably be fine, but there are a few places (such as this, for calculating the slope of a line) that we'll need floating point numbers, or at least that's what's been used in the past. Although you could just use some arbitrarily large mapped value like 1M:1 to get fp-like representations with integers.

ClockworkZombie · Mar 22, 2014

Any news on the benchmarking?

kevingranade · Mar 22, 2014

kevingranade reviewed Mar 22, 2014

View changes

kevingranade · Mar 22, 2014

src/line.cpp

+    double normDy = (line.back().y - line.front().y) / len;
+    double normDz = (line.back().z - line.front().z) / len;
+    auto retXY = std::make_pair(normDx, normDy);
+    auto ret = std::make_pair(retXY, normDz); // slope of <x, y> z


das "auto" ist auch verbotten

fie... c++11 strikes again. I'll remove this later today if I have time.

I got it in the merge, just a few lingering c++11-isms. Also a == b == c does not do what you think it does.

kevingranade · Mar 22, 2014

Tested against the old version, and everything agrees (see tests/line_test.cpp), and here's the benchmark:
line_to() executed 1000000 times in 6.444942051 seconds.
cannonical_line_to() executed 1000000 times in 6.666036779 seconds.
So just a hair faster

ClockworkZombie · Mar 22, 2014

Awesome, thanks! I've only got mobile access at the moment, really appreciate the fixes. I'll check this out tonight.

ClockworkZombie added some commits Mar 2, 2014

allows compiling with visual studio

18b36b6

Generalizing line algorithms for three dimensions, refactoring.

Loading status checks…

b503f02

reverting changes to itype.h

Loading status checks…

4d5d999

Further refactoring, integer-based iterative line generation in three…
… dimensions.

Loading status checks…

b3318e2

cleanup

Loading status checks…

1e3e8ed

kevingranade referenced this pull request Mar 16, 2014
Merged
Fix testing #6712

removing tuples, tweaking continue_line to make things line up better…
… in case debugging is needed.

Loading status checks…

e8d50fe

kevingranade merged commit e8d50fe into CleverRaven:master Mar 22, 2014
1 check failed

1 check failed

default Unmergeable pull request.

CleverRaven/Cataclysm-DDA

Join GitHub today

Generalizing lines algorithms for three dimensions, refactoring. #6560

Conversation

ClockworkZombie commented Mar 11, 2014

ClockworkZombie added some commits Mar 2, 2014

This comment has been minimized.

Lain- commented Mar 12, 2014

kevingranade reviewed Mar 12, 2014 View changes

This comment has been minimized.

This comment has been minimized.

kevingranade reviewed Mar 12, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kevingranade reviewed Mar 12, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Zireael07 commented Mar 13, 2014

This comment has been minimized.

ClockworkZombie commented Mar 15, 2014

kevingranade reviewed Mar 15, 2014 View changes

This comment has been minimized.

kevingranade reviewed Mar 15, 2014 View changes

This comment has been minimized.

kevingranade reviewed Mar 15, 2014 View changes

This comment has been minimized.

This comment has been minimized.

kevingranade reviewed Mar 15, 2014 View changes

This comment has been minimized.

This comment has been minimized.

ClockworkZombie commented Mar 16, 2014

kevingranade referenced this pull request Mar 16, 2014

kevingranade reviewed Mar 17, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ClockworkZombie commented Mar 22, 2014

kevingranade reviewed Mar 22, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kevingranade commented Mar 22, 2014

Hide details View details kevingranade merged commit e8d50fe into CleverRaven:master Mar 22, 2014 1 check failed

1 check failed

This comment has been minimized.

ClockworkZombie commented Mar 22, 2014

kevingranade reviewed Mar 12, 2014

View changes

kevingranade reviewed Mar 12, 2014

View changes

kevingranade reviewed Mar 12, 2014

View changes

kevingranade reviewed Mar 15, 2014

View changes

kevingranade reviewed Mar 15, 2014

View changes

kevingranade reviewed Mar 15, 2014

View changes

kevingranade reviewed Mar 15, 2014

View changes

kevingranade reviewed Mar 17, 2014

View changes

kevingranade reviewed Mar 22, 2014

View changes

kevingranade merged commit `e8d50fe` into CleverRaven:master Mar 22, 2014
1 check failed