Skip to content

Conversation

@kmadsen
Copy link
Contributor

@kmadsen kmadsen commented Apr 14, 2020

Description

Create parity with a new interface. The new route player should behave the same as the old replay route location engine.

  • I have added any issue links
  • I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
  • I have added the appropriate milestone and project boards

Screenshots or Gifs

old player new player
output output

Testing

Please describe the manual tests that you ran to verify your changes

  • I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
  • I have tested via a test drive, or a simulation/mock location app
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have updated the CHANGELOG including this PR

@kmadsen kmadsen force-pushed the km-replay-route-with-history-player branch from 6a11167 to 00441d7 Compare April 15, 2020 18:59
@kmadsen kmadsen marked this pull request as ready for review April 15, 2020 19:00
@kmadsen kmadsen modified the milestones: v1.1.0, v1.0.0 Apr 15, 2020
@kmadsen kmadsen force-pushed the km-replay-route-with-history-player branch from 00441d7 to 561a1d2 Compare April 15, 2020 19:53
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

This is looking good @kmadsen

One thing that I noticed when testing is that it seems we're wrongly calculating the last locations in the route 👀

replay_route_history_player_issue

I believe this is related to the fact of using a constant speed to generate the points along the route geometry. We've seen this in the past when using ReplayRouteLocationEngine / ReplayRouteLocationConverter.

Refs. #2717

@kmadsen
Copy link
Contributor Author

kmadsen commented Apr 15, 2020

One thing that I noticed when testing is that it seems we're wrongly calculating the last locations in the route 👀

Yeah I made this work the same way as the existing replay engine. So the behavior is unchanged. Can get started on improving it

@kmadsen
Copy link
Contributor Author

kmadsen commented Apr 15, 2020

I am finding that simulated locations are not working well with enhanced locations. Building GPS simulated locations from a route will have a lot of effort.

Here is the video after updating the LocationComponent with RawLocations from the history player @Guardiola31337

output

@kmadsen kmadsen force-pushed the km-replay-route-with-history-player branch 2 times, most recently from 8832326 to 95aaf0a Compare April 15, 2020 22:48
@Guardiola31337
Copy link
Contributor

@kmadsen

I am finding that simulated locations are not working well with enhanced locations.

Not sure about this as a workaround 👀

replay_route_history_player_cutting_corners

@kmadsen
Copy link
Contributor Author

kmadsen commented Apr 15, 2020

Not sure about this as a workaround 👀

If we're generating locations, I think we should be controlling the view directly. We can rely on the history data to replay GPS and real data.

Otherwise, will need some more time to see if building a location generator that is able to slow down at corners.

@kmadsen
Copy link
Contributor Author

kmadsen commented Apr 16, 2020

Closing this as it's going to go back in progress

@kmadsen kmadsen closed this Apr 16, 2020
@kmadsen kmadsen reopened this Apr 16, 2020
@codecov-io
Copy link

codecov-io commented Apr 16, 2020

Codecov Report

Merging #2766 into master will decrease coverage by 0.10%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #2766      +/-   ##
============================================
- Coverage     35.12%   35.02%   -0.11%     
  Complexity     2100     2100              
============================================
  Files           540      541       +1     
  Lines         19260    19316      +56     
  Branches       1819     1827       +8     
============================================
  Hits           6766     6766              
- Misses        11683    11739      +56     
  Partials        811      811              

@kmadsen kmadsen force-pushed the km-replay-route-with-history-player branch 2 times, most recently from 9eaa9c8 to 9681722 Compare April 16, 2020 16:07
@kmadsen kmadsen mentioned this pull request Apr 16, 2020
10 tasks
@kmadsen kmadsen force-pushed the km-replay-route-with-history-player branch 5 times, most recently from a022ec8 to b8203c1 Compare April 17, 2020 16:21
@kmadsen kmadsen force-pushed the km-replay-route-with-history-player branch from b8203c1 to 6f30136 Compare April 17, 2020 16:40
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Left a minor comment. Other than that :shipit:

…layActivity.kt

Co-Authored-By: Pablo Guardiola <pablo.guardiola@mapbox.com>
@kmadsen kmadsen merged commit 210f498 into master Apr 17, 2020
@kmadsen kmadsen deleted the km-replay-route-with-history-player branch April 17, 2020 17:57
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.

4 participants