Repository navigation
4-Connected Line Drawing - #2336
Conversation
|
Existing user-facing functions don't appear to have had their api changed, so this shouldn't incur a major version increment. |
That would be nice. |
|
Overloading the Changes to |
| line: Line, | ||
| re: RasterExtent, | ||
| options: Options | ||
| )(f: (Int, Int) => Unit) { |
There was a problem hiding this comment.
Index access to List is a O(L) cost: http://docs.scala-lang.org/overviews/collections/performance-characteristics.html
There was a problem hiding this comment.
I'll confess that I did not look very closely at this code, I just copied what was there.
There was a problem hiding this comment.
Wow, that's been there a long time... well, thank you for bringing it to the light.
| * LineString. The iteration happens in the direction from the | ||
| * first point to the last point. | ||
| */ | ||
| def foreachCellByLineString( |
There was a problem hiding this comment.
Rasterizer is a performance sensitive part of code in my experience. getCoordinates already returns an Array[Coordinate] this loop and loop below can be rolled into one. That avoids an extra collection allocation and the takes care of indexing cost comment below.
There was a problem hiding this comment.
Unless I misunderstand what you are saying, this is a copy of the existing code. Does it need to be changed too?
There was a problem hiding this comment.
Yes, please. I suppose many eyes finally have their day :)
Okay, I will rework the interface. |
|
I made an issue for the rasterizer issues. |
fadb220 to
9096531
Compare
|
Changes made |
cdb7dd1 to
5296144
Compare
3 participants
This adds 4-connected line drawing capability.
The API is "broken" in the sense that the line drawing subroutine now takes and pays attention to a potential
optionsparameter whereas it did not before.Options.DEFAULTpreserves the original (8-connected) behavior, whereas 4-connected drawing is done whenoptions.sampleType == PixelIsArea.In terms of the API, it seemed as though the choice were
Optionscase class (in terms of invasiveness, seems >= the approach taken here)This PR should probably either be closed or given the "2.0" label.
Also, while typing this PR I realized that it does not solve the vector rasterization problem for which it was intended because the line drawing code snaps line endpoints to the centers of pixels before drawing; because of that, the reported pixels are not guaranteed to cover the line. The picture below has an example.
The blue line is the original with the tiles that should be reported. The red is the snapped-to-centers line with the tiles that will be reported. Since there are some tiles in the blue set that are not in the red set, this cannot be used. (If the set of tiles is 3x3 but the set of pixels is 3nx3n, then there will be some pixels which should be drawn but which are not because there is no tile to contain them.)
I did not change endpoint-snapping behavior because seems like an even larger change than the one presented here, so the three-part list above applies. I am planning to put in a solution to the original problem that does not involve the line rasterizer.
Note: I said above that it appears from the code that the line endpoints are being snapped to the centers of the pixels. Even if that is not correct -- if they are being snapped to a corner or the middle of an edge -- the fact that the endpoints are being converted to integers means that such an example can probably still be constructed.