search

itinero / srtm

A library to load SRTM data and return heights in meter.
MIT License
17 stars 11 forks source link

add bilinear interpolation support for a smoother result #8

Closed Em3rgencyLT closed 4 years ago

Em3rgencyLT commented 4 years ago

The purpose of this PR is to add a method to get elevation using bi-linear interpolation.

This PR should close #5

Before and after shots, when using a 1 arc second data set and querying for elevation roughly every 0.3 arc seconds. Area is roughly 1 square kilometer: elevation raw elevation bilinear

I chose to restructure some of the code to prevent unnecessary duplication. The data cell downloading was moved into it's own method. Retries are now done recursively. File byte reading was also moved into it's own method.

Code available at Rosetta Code was used for the actual interpolation implementation.

Finally, my IDE was complaining about a few mismatches in some of the method documentation, so I've updated where necessary.

xivk commented 4 years ago

Again, this looks excellent! :+1: Thank you so much!

I was wondering why you made the choice not to just replace the current implementation and create an extra call 'GetElevationBilinear' vs 'GetElevation' I mean... I don't see a reason to keep the old version around as the new is always better with a very minimal impact on performance.

Em3rgencyLT commented 4 years ago

You're welcome! It was fun adding these changes ;)

TL;DR My reasoning for adding a method instead of replacing was user choice, backwards compatibility and cleaner code.

There might be cases where users of the library would want to use the raw data. Either so they can do their own manipulation or because they are sampling at large distances (like a topographical map for an entire continent for example). In those cases, a smoothed result is just giving them innacurate heights.

For backwards compatibility, people who have been using the library may have already implemented their own smoothing. And in general, updating to a newer version should not break the old version. Removing an existing method should be preceeded by having it tagged as deprecated for a release or two.

Finally, if I had just replaced the original method, I would not have needed to refractor the other areas like I did. And I believe the refactoring made the code easier to work with in the future. So that benefit would have been lost. Additionally if someone wanted to implement bi-cubic interpolation for example, it would've been harder to do than it is now.

I'd like to hear your thought on my reasoning. Change my mind :D

Em3rgencyLT commented 4 years ago

@xivk do you want me to change it to overwrite the method instead? I would love to have this merged so I could use the library with nuget.