Fix this thing aka RouteDrawer again... #1231
Conversation
Hopefully forever
| @@ -190,6 +244,7 @@ private int getAlternativePointArrayCount() { | |||
| result.add(new Line2D.Double(points[0], points[1])); | |||
| } | |||
| } | |||
| result.forEach(e -> e = null); | |||
There was a problem hiding this comment.
Why are all elements being set to null.
| new Point2D.Double(point.getX() + mapWidth, point.getY() - mapHeight), | ||
| new Point2D.Double(point.getX() + mapWidth, point.getY() + mapHeight))); | ||
| } | ||
| if (isInfiniteX) { |
There was a problem hiding this comment.
If both isInfiniteX and isInfiniteY are true, this if statement will also be executed. Given the points are added, they look redundant, yet are different. What is the purpose of these points that are being added, and how are we computing the indexes? It's not clear the reasoning behind these values.
There was a problem hiding this comment.
This is intended! When isInfiniteY we want the point-clones above and below the center..., when isInfiniteX we want the ones left and right the center, but if both are we also want the clones in the upper left, upper right, lower right and lower left.
There was a problem hiding this comment.
Okay, please note that in the code as a comment so we have that context there. (I prefer when code is self documenting through method and variable names, but the point is to get the contextual information of why this is being done captured in the code one way or another.) I recently had to look at a git history, back track changes to their commit dates, look for PRs on those dates to then find review discussion that actually told me why some code was the way it was. You can probably guess that was an enjoyable and efficient process ;)
|
@DanVanAtta It's very hard to explain here how my system works. |
|
@DanVanAtta I understand your good intentions, but my time is limited, and therefore it might take a day or two until I added tests etc. |
|
Gave the route drawer a good testing on WaW, and on HexGlobe (has both X + Y wrap) - looked good 👍 |
|
I might be able to give you a hand with retrofitting some unit tests. In hindsight, making sure everything is pretty solid, tested, and potentially shipped sooner, is paramount. I don't think the mass regression test has been that successful. .. With that said, yeah. Let's get 1.9 out.. @RoiEXLab , I 'll hold you to your word to revisit the route drawer! |
|
@DanVanAtta Thanks! I wont let you down! |
Hopefully forever!
@DanVanAtta I was too lazy to undo all the changes, so I created a fix!
I removed a couple of usages of the normalizePoint method!
The only possible downside of this could be, that maybe when scrolling to far away from a route (when watching history the route may become invisible if this ever happens we need to simply increase the mirroring radius from 2 -> 9 screens to 3 -> 16 screens which means less performance for the user)
Based on pre #1199
If you didn't guess already this fixes #1056
This change is