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

DEM updates #38

Closed
wants to merge 3 commits into from
Closed

DEM updates #38

wants to merge 3 commits into from

Conversation

danfos
Copy link
Contributor

@danfos danfos commented Jan 21, 2018

The commit description should say it, on the reason why allowing negative minimum heights in the DEM layer, parts of the Netherlands is below sea-level.

@rnorris
Copy link
Collaborator

rnorris commented Jan 29, 2018

Thanks for the continued interest.

  1. Remove debug printf
  • I will apply this.
  1. Fix whitespace: replace tab by spaces
  • Agreed it's kind of annoying that some files have inconsistent whitespacing. It's more than just vikdemlayer.c file though.
    I've often thought about applying some auto indent to the code for consistency, but however I decided that being able to see who and when initially wrote the code via a clean source code history (e.g. just git blame if possible, rather than having to sift back through commits) was preferable.
    Thus I won't be applying this
  1. Allow for negative minimum heights in the DEM layer
  • Some of England is also lower than the sea too - particularly Norfolk & 'The Wash'
    Unfortunately this code doesn't maintain the current behaviour - the 'sea' turns brown when using a value of 0 (which is also the default)

Maybe the test on line 682 needs to be '<=' so that the min_elev value is included in the 'below_minimum' range? Albeit 'below_minimum' is then not an accurate variable name - may be rename that to 'minimum_level' or similar.

Note DEM values over sea is 0, just as it can be on land - so from DEM information one can not really distinguish between land and sea. But it's nice to have the option to draw a pseudo sea.

@danfos
Copy link
Contributor Author

danfos commented Feb 11, 2018

On 2) I see your point. On the other hand, annotating for a past tag (git annotate ^) is easy enough, on my job the code was once fixed for spacing and I regularly do that. No problem dropping it though.

Good catch about blue for sea, yes changing < to <= will do the trick and by inverting the below_minimum variable and calling it above_minimum I think the code remains readable, let me try to update the patch on this.

@rnorris
Copy link
Collaborator

rnorris commented Nov 14, 2020

Allow for negative minimum heights in the DEM layer functionality has (finally!) been added into the source code.

@rnorris rnorris closed this Nov 14, 2020
@danfos
Copy link
Contributor Author

danfos commented Nov 21, 2020

Thanks for merging!

@danfos danfos deleted the dem_updates branch November 21, 2020 12:07
@danfos
Copy link
Contributor Author

danfos commented Nov 21, 2020

Thanks for merging!

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.

2 participants