search

osmandapp / OsmAnd

OsmAnd
https://osmand.net
Other
4.66k stars 1.02k forks source link

Elevation graph regressions #3514

Closed sonora closed 7 years ago

sonora commented 7 years ago

The recent refactoring of the elevation graph introduced the following bugs:

untitled

(1) All axis labels do not any more respect the app theme's color pattern (2) The meaning and significance of the orange step background pattern is unclear (3) The prominent orange background suppresses the visibility of the actual "feature", i.e. the elevation curve (only visible as faint and thin dark line) (4) The x axis is always labeled in kilometers, regardless of the distance unit selected in the app (5) When returning from a prior view with split interval (and having selecting a point on the x-axis there by tapping the graph) to one without split interval, the scaling of the x-axis remains that of the split interval (should return to the overall scale)

Here for comparison the same graph in the previous version:

untitled1

vshcherb commented 7 years ago

(5) split interval will be changed and look differently The graph looks buggy and the comment about km/miles setting is valid.

crimean commented 7 years ago

@sonora Thanks for report! Could you please send this gpx to me?

sonora commented 7 years ago

Hello Alexey, sure, it's in your mail. Best, Hardy

vshcherb commented 7 years ago

Can you please also forward to me, Alexey looks like didn't get it.

sonora commented 7 years ago

Another comment regarding the new "MyTracks" overview screen: Can we please make sure we sort all track folders again alphabetically? I have a large number of sub-folders (under osmand-data-folder/tracks), and it looks like somehow these are now not sorted quite right any more, see screenshots with marked oddities. Thx - Hardy

1 2

vshcherb commented 7 years ago

Yes, that's a regression bug.

crimean commented 7 years ago

@sonora should be fixed now. Please check and close the issue if all is fine.

sonora commented 7 years ago

Victor, Alexey, Don't know, it looks really weird .... am I trying the correct build??

screenshot_2017-02-23-16-33-10_
crimean commented 7 years ago

@sonora what OS do you have on the device? Are you sure folders were sorted right way before this update?

sonora commented 7 years ago

Alexey, all screenshots I posted in this issue were taken on a Samsung S4 Mini with stock Android 4.4.2. I just verified that a Samsung S5 Mini with stock Android 5.1.1 behaves exactly alike.

But I apologize that I seem to have missed that going back to my current productive build, our nightly of January 31, behaves much the same: The folders are mostly sorted alphabetically, but there are occasional oddities. Hence I now think I was incorrect calling this a regression, it seems to have been present before you reworked the layout. Sorry for not catching that, my fault!

crimean commented 7 years ago

I see. Yesterday I've tested new chart with your gpx on S3 (android 4.4.4). It looks ok. Without any bricks. So it is not clear why you have broken chart even on 5.1.1. Regarding messed list - please analyse tracks folder of osmand using an file browser. May be you will find the reason there.

sonora commented 7 years ago

Good idea playing with a file manager! Finding is that, regarding the sorting, it is an upper case / lower case issue: Looks like folder names starting with an upper case letter are all sorted before any folder name starting with a lower case letter, which is why in my screenshot "misc" and "rec" are sorted all the way to the end. They are sorted earlier if I change the first letter to upper case.

Regarding the blue bricks: I see these on all devices I checked. Can you post a screenshot how it is supposed to look? In general I feel that the elevation curve is too thin and the axis captions are too tiny compared to how the rest looks on our screens, it looks like it is not compatible with the rest of our style guide?

crimean commented 7 years ago

Please check last build. See if chars are ok. Also list of folders should be sorted properly - case insensitive.

sonora commented 7 years ago

PS: The graph itself has changed some with the latest build. I now see that it is a selection of the curves only on the different screens, which is okay. :)

But the colored step background is still present on all graphs .. see screenshot.

screenshot_2017-02-24-19-53-21

Side note: I have mixed feelings about the tabbed view, because I used to make single screenshots to document an overall summary of ALL key numbers for a certain track segment or split interval (both for sports and professional purposes), which is now distributed over 3 different tabs. Still not sure if there is a real benefit to loosely distribute this info over different screens, or if we could not put it back onto one screen with the code colors we had developed. But I guess this will take more people's opinions to judge.

sonora commented 7 years ago

PS: There are more bugs in connection with the new tabbed view: Take my file, and select split interval=1km (do not select "Show on map"). Scroll down on the different tabs you get. It is a mixture of different graphs (mixed from different tabs, I think), and not even the intervals and axis captions seem to properly reflect the selected 1km intervals in the right order?

crimean commented 7 years ago

It is not ready yet.

sonora commented 7 years ago

PS: I wonder if we could maybe fit all numerical data back onto only one tab again, and we could simply display or highlight the approriate curve(s) in the graph by tapping on the numerical data for speed or altitude? (by default display both as overlay, like you have it already on the first tab)?

crimean commented 7 years ago

device-2017-02-25-135417 This is how char should look like. We cannot reproduce bricks. We tried on 4.0, S3 4.4, 6.0 and charts displays ok there.

sonora commented 7 years ago

Ok, to take things from here, let me for comparison post out old analysis screen again:

screenshot_2017-02-26-08-38-24

sonora commented 7 years ago

I know things are not ready yet. But here is my a tentative list of things we should think about and likely still fix:

njohnston commented 7 years ago

Thanks for your work on this @sonora and @crimean.

These changes look encouraging and useful. I compiled OsmAnd yesterday and tested the GPX analysis. I know work is still in progress but I wanted to provide some feedback as it might be useful.

(f) Handling of tracks with multiple segments

If a track has multiple segments, a chart is shown for each segment:

1a

(GPX file)

This can be useful, but sometimes tracks can have many segments (particularly if there are pauses), so it would be useful to have a single chart showing the entire track, along with stats. In this case, the track is for a walk up a hill. To me, it's one logical track--there are two segments only because I paused at the top for a rest.

(g) Speed not always shown

No separate speed is shown for the above track (even though the track contains speed data). I'm not sure if that's because it's relatively slow (about 6 km/h on average). If I tap on the chart I can see my pace (number of minutes to walk one km) but I'd expect to see a separate line for speed, either in the main chart or the speed one.

(h) Blank track tab for GPX files containing routes instead of tracks

An empty track tab is shown for GPX files containing routes instead of tracks:

2a

Also, each route point is shown in the points tab as "important", which is probably wrong.

(i) Tracks without altitude and time aren't shown very clearly

GPX files containing tracks without altitude and time aren't shown very clearly:

3a

For tracks like this I think it would be clearer to show the time as '-' to indicate that the track contains no time data.

The detailed altitude tab also shows the hard-coded default minimum and maximum altitude from GPXUtilities.java--showing this isn't useful.

Unless there's a plan to extrapolate altitude using SRTM or other DEM data, the elevation chart shouldn't be shown if there is no elevation data in the track.

This track is also interesting as each segment is named. If a segment has a name, it would be great to show that as a heading for that segment.

Please let me know if I should open separate issues.

sonora commented 7 years ago

Thanks for the feedback! I guess it is fine for now to collect all the bugs and suggestions here. I have taken the liberty to re-size the screenshots a bit, and number the items for easier reference. Please note that

vshcherb commented 7 years ago

Hi @sonora, Looks like most of the issues are solved and it's time to reply to the feedback. I'm very keen to close this bug, so you could start new again where it could be easier to continue discussion.

(j) Bricks bug not reproducible on development devices and so far we don't want to loose "area" chart type which of course is a Plan B if we have it on many specific devices. (k) Yes it is lost in favor of manual zoom in and pan the graphics. Statistic screen should also help with that. Split interval is something to display on the map, now.

(h) blank tab is fixed now (f) may be a separate issue but this is done on purpose.

vshcherb commented 7 years ago

Again I would like to continue discussion and will be replying here as well. Though there is nothing from that list in active development, may be I missed something critical.

sonora commented 7 years ago

@vshcherb Hello Victor, I have don a quick run-through using this morning's build and from what I see our progress is good here!

Here are the remaining issues I still see, together with my judgment of how critical each is:

Best, Hardy

vshcherb commented 7 years ago

(a) Overview screen: Is it the same? I'm not sure if we understand what we are missing. May be you are talking about "split interval" numbers, over some time I've found it very cumbersome to use and it is far less intuitive comparing to UI we have now i.e. map, graph integrated (d) Number of points is the only missing, time moving is on the last tab. It is indeed not clear how to organise better. (j) We can disable only for the whole release 4.4.2 and it is not reproducible on all 4.4.2 (k) same as (a)

njohnston commented 7 years ago

I compiled OsmAnd again to look at the latest changes. I like the preview of the tracks, and the ability to tap on the chart to see the corresponding point indicated on the track.

However, there are still a few issues. I've continued @sonora's numbering.

(l) No prompt to download maps

When previewing a track from a place that you haven't downloaded maps for, a blank background is shown. This is fine, but OsmAnd should prompt the user to download maps for the area.

(m) The preview map can be very cluttered and unreadable

For this sample GPX file, the preview looks very cluttered and is almost unreadable:

cluttered_preview_map

Hopefully this is just an emulator issue.

(o) "Flash" of main map after tapping "Details" back button

details_back

On the details view, when pressing "Details" to go back, the main map "flashes" briefly while the stats activity reloads. This is a bit disruptive.

Follow-up on the other issues I raised

@vshcherb: if I understood correctly, you said (f) (separate charts and stats for each segment) is intentional. I really think that an overview section with stats and charts for the whole track is necessary. Unintentionally having multiple segments in a track is very common due to pausing or losing GPS signal. Showing each segment separately makes it hard to easily analyse an entire hike or drive.

(g) (speed not always shown) is still not fixed. For my sample GPX file, the average and maximum speed (technically pace, not speed) is shown, but the separate speed chart is empty, and there is no speed series on the main chart:

speed_missing

Being able to see speed as well as pace would be good too.

Thanks for your work on this. I'm looking forward to the changes being released :)

vshcherb commented 7 years ago

Yes pace is something to be fixed.

Taking a look at overall information of the track, something that deserves separate issue and again it was not working before. Right now overall information is displayed only distance.

crimean commented 7 years ago

Pace chart fixed

sonora commented 7 years ago

Victor, by "overview screen" I meant one single overview tab containing ALL the numbers for a track, something you can send around as a screenshot on a geologic survey ...

Maybe we can add an overview tab back on, pretty much like the old track analysis screen we used to have?

sonora commented 7 years ago

@vshcherb, @crimean Just in case this information got lost: I have just double-checked again the build we have in our download section of February 25 (osmand-master-nb-2017-02-25). That build produces perfect curves on the device in question they are filled (shaded) on the inside, and have no encapsulating "bricks" on the outside. What did we do to achieve this, can we not go back to that solution? Thx - Hardy

crimean commented 7 years ago

@sonora I checked it - this build contained splines (interpolation between points). I could restore it, but chart for big gpx (starting from 100 km) will draw much slower. Now I turned on hardware acceleration for the chart (Build #20757). May be it will help. Could you try?

sonora commented 7 years ago

Alexey, yes, you are right: The build #20493M which worked on that device had spline, which we should not do, it looks weird and wrong for tracks with few points.

I just tried #20765M, unfortunatrly no change, the encapsulating bricks are still there. Strange error. I wonder what determines the "width" of the bricks. Sometimes there are many narrow ones, hence less visible, sometimes only 1-3, spanning the entire width of the graph ...

njohnston commented 7 years ago

@vshcherb @crimean: thanks for fixing the pace chart. I've tested the latest code and the chart works, but has some problems:

pace_chart_stretched

In this track, I stopped to take a photo. I didn't stop for long enough to trigger a pause, so OsmAnd just thinks I moved very slowly.

This causes high (slow) pace values like 80 min/km, which stretch the scale, making it harder to see the more normal data.

I think speed would be better than pace here, because low speed values would only cause a dip in the line chart, rather than stretching the scale like low pace does.

Pace also seems inappropriate for anything except walking, running or cycling. In this track from a flight, where the average speed is very high, it's strange to report this as 0.1 min/km:

pace_chart_useless

The speed chart isn't very helpful, because as before, the axis or scale has been stretched so much, and pace for most of the track is 0.1. Using speed rather than pace would fix this. Thanks :)

sonora commented 7 years ago

Yes, having a pace chart like this is flawed, I'm afraid. What is required in practice is what we had before: A primarily numerical (not chart) listing of all data for any split interval you may select. A runner must e.g. be able to again select 1 mile or 1 km splits and see the time he needed for each mile/km, alongside with the characteristics of each stretch (like elevation change etc.). I can think of no other way of fixing this significant regression than what I have said all along: Let's re-add an "Overview" tab (as the 1st tab) with all numerical data on one screen (and perhaps the combined elevation/speed chart as the statistically only significant one), and the screen must be split vertically in different equivalent copies per split interval if selected, just like we had it until our Jan builds (there is a screenshot from me further up this issue).

Other information:

As I have noted elsewhere, some Issues on the UI as follows:

sonora commented 7 years ago

PS: I really feel sorry for Alexey having to follow all this in the middle of coding ... for the next such major change I guess we could do a much more professional product management job in specifying all needed functionality, UI, data, and discussing a strawman, rather than creating chain issues like this which partly talk about library bugs, partly about UI consistency, partly about regressions like dissapearing functionality or data, and partly about new feature requests like a combined view over different track segments etc...

But now we are sort of in the middle of this (and almost through I hope), let me plesse re-open this at lesst until we fix the worst regressions

EDIT:

sonora commented 7 years ago

@crimean, @vshcherb : Alexey, good idea about the hardware acceleration. I managed to fix the "bricks bug" (generally, I think) after some research via https://github.com/osmandapp/Osmand/commit/84ecf92bd38853bcf36d95edb728675ad221a5a6

But while it now becomes usable for all devices, let's please do something still about the above mentioned points before release, or I am afraid we may still earn as much praise as fire here ...:)

sonora commented 7 years ago

Summary of what I see still open:

That's all I can think of!

vshcherb commented 7 years ago

@sonora list. 2 issues won't be implemented this release and deserve separate issues/discussions:

sonora commented 7 years ago

Please see my questions and screendhot here https://github.com/osmandapp/Osmand/commit/7f9bc4d5ce8afe9d83d5a7dcdd8bb6af9258c910#commitcomment-21504954