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

Use the newer geo-data-exchange library #781

Merged
merged 3 commits into from
Jun 1, 2024
Merged

Conversation

alexcojocaru
Copy link
Contributor

The newer library has a more accurate gradient calculation algorithm: there is no more segmentation of the track into 200m segments, and so the resulting features (i.e. portions of the track with constant gradient) map much better to the elevation chart.

Before:
Screenshot_2023-11-13_17-38-29

After:
Screenshot_2023-11-13_17-37-46

@alexcojocaru
Copy link
Contributor Author

alexcojocaru commented Dec 10, 2023

We should focus on accuracy ... The concept of normalization also conflicts with another use case: The heightgraph colorization can be employed to indicate short segments with very steep slopes for route planning purposes, which now would simply get hidden.

IIRC @nrenner had a similar ask on the PR which introduced Heightgraph. The first commit in this PR does that - it tracks all gradient changes and plots them on the chart, but the result is almost unreadable in most of the cases. I did not implement/enable normalization for beautification purposes, but to make the chart more readable. At the end of this comment is an example of an elevation profile , without and with normalization.

If showing accurate gradients is preferred, I am more than happy to disable the normalization (it'd be a very easy change).

As for normalizing:

  1. There is another normalization parameter which I haven't overridden : https://github.com/alexcojocaru/geo-data-exchange/blob/master/src/index.js#L31 . I could set it to a lower value (e.g. 3px), to keep the gradient changes more accurate and, at the same time, the chart more readable.

  2. Normalizing in Heightgraph would be a big ask, for that component is generic. This use case - showing the normalized gradients on the chart, but displaying the precise gradient on mouse over - might be too specific for the component itself. If that's the case, then this logic should go into a wrapper, in which case I don't know if the wrapper could get low-level access to the Heightgraph functionality that it would need to enable this behaviour.

  3. Having a toggle to enable/disable normalization is an interesting option; are you interested in exploring this option more (e.g. where to show this toggle, what label to use on it, etc)?

  4. Could you elaborate on "Maybe having two static preconfigured values for chartWidthInPixels (one regular and one for when the toolbar at the top is collapsed)" ? I am not following what the 2nd one is for.

  5. If using a toggle, and if the user has enabled the normalization, I could disable it on chart resize - this behaviour might be confusing to the user, but at least the chart will be accurate.

Screenshot 2023-12-01 at 14-55-51 Elevation Profile

Screenshot 2023-12-10 at 11-31-16 Elevation Profile

@alexcojocaru
Copy link
Contributor Author

I think I found a better solution to this problem: instead of bucketing the gradients on the profile (i.e. gradients 1 through 3 are shown as group 1-3, gradients 4 through 6 as group 4-6, etc), the algorithm keeps track of each individual integer (e.g. gradient 0, gradient 1, gradient 2, etc) . This results in a much busier profile, but I've mitigated that by using a gradient palette to assign similar colors to gradients close to each other (i.e. orange for gradient 0, orange with a tiny bit of red for gradient 1, orange with slightly more red for 2, etc) - this makes the profile appear less busy (i.e. less busy than in the screenshot in my previous comment).
The only downside of this approach that I can think of is the larger dataset that's generated for a given profile, which has to be rendered by Heightgraph.

Here is a screenshot of a profile generated by the new code:
elevation_profile-screenshot

On a related note: since you asked to have the legend shown, I've added it right under the profile.

@bagage
Copy link
Collaborator

bagage commented Jun 1, 2024

Thank you @alexcojocaru, and sorry for the delay. I'll merge this now 🎉

@bagage bagage merged commit 12dc322 into nrenner:master Jun 1, 2024
1 check passed
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