New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4-Connected Line Drawing #2336

Merged
merged 6 commits into from Aug 23, 2017

Conversation

Projects
None yet
3 participants
@jamesmcclain
Member

jamesmcclain commented Aug 18, 2017

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 options parameter whereas it did not before. Options.DEFAULT preserves the original (8-connected) behavior, whereas 4-connected drawing is done when options.sampleType == PixelIsArea.

In terms of the API, it seemed as though the choice were

  • This
  • Awkwardly add something along side the existing API
  • Extend the Options case 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.

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.

@fosskers

This comment has been minimized.

Contributor

fosskers commented Aug 18, 2017

Existing user-facing functions don't appear to have had their api changed, so this shouldn't incur a major version increment.

@jamesmcclain

This comment has been minimized.

Member

jamesmcclain commented Aug 18, 2017

so this shouldn't incur a major version increment.

That would be nice.

@echeipesh

This comment has been minimized.

Contributor

echeipesh commented Aug 22, 2017

Overloading the sampleType really strains the imagination here unless there is a conceptual explanation I'm missing. The shape of the solution is good though except the new overloads for foreachCellInGridLine, foreachCellByLineString, and foreachCellByMultiLineString should accept Connectivity that will not break the API.

Changes to foreachCellByGeometry are no go, they should just use the default 8-connected algorithm. Until 2.0 the user interested in 4-connected variant will have to match on the geometry themselves and pass in the appropriate Connectivity

for(i <- 1 until cells.size) {
foreachCellInGridLine(
cells(i - 1)._1, cells(i - 1)._2,

This comment has been minimized.

@echeipesh

This comment has been minimized.

@jamesmcclain

jamesmcclain Aug 22, 2017

Member

I'll confess that I did not look very closely at this code, I just copied what was there.

This comment has been minimized.

@echeipesh

echeipesh Aug 23, 2017

Contributor

Wow, that's been there a long time... well, thank you for bringing it to the light.

)(f: (Int, Int) => Unit) {
val cells = (for(coord <- line.jtsGeom.getCoordinates()) yield {
(re.mapXToGrid(coord.x), re.mapYToGrid(coord.y))
}).toList

This comment has been minimized.

@echeipesh

echeipesh Aug 22, 2017

Contributor

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.

This comment has been minimized.

@jamesmcclain

jamesmcclain Aug 22, 2017

Member

Unless I misunderstand what you are saying, this is a copy of the existing code. Does it need to be changed too?

This comment has been minimized.

@echeipesh

echeipesh Aug 23, 2017

Contributor

Yes, please. I suppose many eyes finally have their day :)

@jamesmcclain

This comment has been minimized.

Member

jamesmcclain commented Aug 22, 2017

Changes to foreachCellByGeometry are no go, they should just use the default 8-connected algorithm. Until 2.0 the user interested in 4-connected variant will have to match on the geometry themselves and pass in the appropriate Connectivity

Okay, I will rework the interface.

@jamesmcclain

This comment has been minimized.

Member

jamesmcclain commented Aug 22, 2017

I made an issue for the rasterizer issues.

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/4-connected branch from fadb220 to 9096531 Aug 23, 2017

@jamesmcclain

This comment has been minimized.

Member

jamesmcclain commented Aug 23, 2017

Changes made

jamesmcclain added some commits Aug 18, 2017

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/4-connected branch from cdb7dd1 to 5296144 Aug 23, 2017

@echeipesh echeipesh added this to the 1.2 milestone Aug 23, 2017

@echeipesh echeipesh merged commit 7bd7936 into locationtech:master Aug 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jamesmcclain jamesmcclain deleted the jamesmcclain:feature/4-connected branch Aug 24, 2017

@rossbernet rossbernet referenced this pull request Sep 29, 2017

Closed

rasterizer bug #510

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment