Skip to content
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

Viewshed #2917

Merged
merged 6 commits into from
May 8, 2019
Merged

Viewshed #2917

merged 6 commits into from
May 8, 2019

Conversation

jamesmcclain
Copy link
Member

@jamesmcclain jamesmcclain commented May 7, 2019

  • Artifacts
  • Meters per pixel calculation

Overview

The issue is that some fidelity is lost when visibility information is transferred between adjacent tiles. The reconstituted data, a collection of line segments associated with each tile, is not guaranteed to cover every pixel in the tile. The solution presented here is to optionally allow light to scatter perpendicularly from each ray.

There have been no changes made to the unit tests because it is unclear how to test a fix like this. Suggestions welcome. Can be tested with https://github.com/jamesmcclain/ViewshedDebug (locally-publish this branch then type ./sbt "project vs" assembly, then $SPARK_HOME/bin/spark-submit --driver-memory=8G --master='local[*]' vs-assembly-0.jar).

Artifacts

Before:

Screenshot_2019-05-07_19-23-33

Screenshot_2019-05-07_19-23-09

After:

Screenshot_2019-05-07_19-23-25

Screenshot_2019-05-07_19-23-17

Meters per Pixel

Before:

Screenshot_2019-05-08_11-58-33

After:

Screenshot_2019-05-08_11-58-57

Comparison:

Screenshot_2019-05-08_11-58-47

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • docs guides update, if necessary
  • New user API has useful Scaladoc strings
  • Unit tests added for bug-fix or new feature

Demo

Optional. Screenshots/REPL

Notes

Optional. Ancillary topics, caveats, alternative strategies that didn't work out, anything else.

Closes #2910

James McClain added 2 commits May 7, 2019 19:11
Allow non-local illumination to compensate for the fact that the union
of the pixels in the rays is not guranteed to completely cover the
entire tile (especially far from the source at severe angels).
@@ -90,8 +90,12 @@ object R2Viewshed extends Serializable {
* @param cols The number of columns
* @param rows The number of rows
*/
def generateEmptyViewshedTile(cols: Int, rows: Int) =
ArrayTile.empty(IntConstantNoDataCellType, cols, rows)
def generateEmptyViewshedTile(cols: Int, rows: Int, debug: Int = 0) = {
Copy link
Member

@pomadchin pomadchin May 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like debug should be Boolean here, if you still want to keep this flag. (sorry that it's a bit early comment since your pr is in progress :))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an integer because it allows one to generate tiles with different "background colors" for debugging purposes. For example, setting debug = key.col + 101*key.row in IterativeViewshed allows one confirm in QGIS which spatial key each tile comes from. It is the visual equivalent of a printf.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamesmcclain thanks for clarifying 👍

@pomadchin pomadchin changed the title [WiP] Viewshed Viewshed May 7, 2019
Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I only have a note about incorrect change log :)

@@ -1,6 +1,14 @@
Changelog
=========

3.0.1-SNAPSHOT
Copy link
Member

@pomadchin pomadchin May 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move your update into 3.0.0-SNAPSHOT section, please?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sure.

@pomadchin
Copy link
Member

Thanks, James!

@pomadchin pomadchin merged commit 0683298 into locationtech:master May 8, 2019
@jamesmcclain jamesmcclain deleted the viewshed branch May 8, 2019 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected Viewshed results
2 participants