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

WIP: Route eating prototype #105

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

Fabi755
Copy link
Collaborator

@Fabi755 Fabi755 commented Feb 29, 2024

Here my final candidate for route drawing improvements inclusive route eating. @boldtrn let me know if you if you agree with this implementation. After that I will clean up the code and add tests and documentation.

I marked the duplicated and outdated NavigationMapRoute class as deprecated.
Followed by them I removed MapRouteLine and extracted the logic to separate classes:

MapLibrePrimaryRouteDrawer --> Creates required layers and generates the geo source for the primary route. Here the route-eating feature is included.
MapLibreCongestionPrimaryRouteDrawer --> Creates required layers and generates the geo source for the primary route congestion segments. This will also supporting the route eating feature.
MapLibreAlternativeRouteDrawer --> All (other) alternative routes are drawn by this class.
MapLibreCongestionAlternativeRouteDrawer --> Congestion segments for all (other) alternative routes are drawn by this class.
MapLibreRouteArrow (Already existing class) --> Draws arrows for the next maneuver
MapLibreWayPointDrawer --> Draws the waypoints

All drawers are delegated by the NavigationMapRoute as it was before.

The drawer components can now injected in the constructor. This allows all users to set custom implementations for route drawing. If the fields are NULL, we use the MapLibre default drawers.

If you agree with this implementation I will continue with the tasks around this implementation. These things to do are:

  • JavaDoc
  • Add, update and fix tests
  • Maybe adjust naming
  • Validate and improve class & function scopes
  • Improve congestion performance
  • Check & solve open code-todos

The congestion logic needs also some performance improvements, while it's a bit lagging. If you use the MapLibrePrimaryRouteDrawer (without congestion) it works well on my tests. Additionally I tried to move some route logic in a background task, but the thread creation seems to took longer than the calculation itself. I not 100% sure how the native line drawing is working, but maybe here is more potential to improve the performance. Also the FPS throttling for (location puck and route eating) is only used on full navigation UI implementation. Here we can add same logic for the custom implementation. The route eating is disabled by default for now.


Is route eating the official name for this? 🤔

@Fabi755 Fabi755 added the enhancement New feature or request label Feb 29, 2024
@Fabi755 Fabi755 requested a review from boldtrn February 29, 2024 23:07
@Fabi755 Fabi755 self-assigned this Feb 29, 2024
@Fabi755 Fabi755 force-pushed the feature/route-eating branch from 73f5937 to eca809a Compare May 13, 2024 16:45
long duration = (long) ((locationUpdateTimestamp - previousUpdateTimeStamp) * 1.1);
duration = Math.min(duration, 2000L); //TODO: outsource max duration as field
valueAnimator.setDuration(duration);
valueAnimator.start();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is incredibly impressive what you've done here with the valueAnimator ❤️.

I am wondering about the performance implication though. Have you tried and/or measured the energy impact of this, for example for a 100 or 1000km route?

For example nearest point on line can be quite costly for larger geometries.

Copy link
Collaborator Author

@Fabi755 Fabi755 May 14, 2024

Choose a reason for hiding this comment

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

I think it is incredibly impressive what you've done here with the valueAnimator ❤️.

Thanks! It's similar to the location puck logic. Not sure if there is a more efficient way.

Have you tried and/or measured the energy impact of this, for example for a 100 or 1000km route?

Not yet. But the congestion drawing is not performing very good yet.

For example nearest point on line can be quite costly for larger geometries.

Yeah, I have reduced this calls yesterday on the first drawer 👍 I will have also a look to the other ones.

Do you know how the source/feature-drawing is worked in detail? I have thought about to split the line in multiple features and have only update the active part, instead update the whole line. But I'm not sure if this will improve or degrade the performance.

Or maybe it make sense to draw the full line as driven and overlay with undriven path (or vice versa). In this case we need only calculate one line and also update only one feature.

Maybe you have here more experience and more ideas.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know how the source/feature-drawing is worked in detail? I have thought about to split the line in multiple features and have only update the active part, instead update the whole line. But I'm not sure if this will improve or degrade the performance.

I think this might be a good idea, so we could split the route line into geometries for every maneuver or leg. I think this will make other things more complicated though (like matching the features, to the current feature and only changing that etc.).

Or maybe it make sense to draw the full line as driven and overlay with undriven path (or vice versa). In this case we need only calculate one line and also update only one feature.

This is actually an interesting idea as well. I think, as long as we stay on the Java layer, our options are somewhat limited.

@nyurik asked about this in the last navigation meeting. One more thing that we could consider, and I believe the maplibre-org would also consider adding to the maplibre-native, would be to actually enable this for maplibre-native. For example iOS is facing similar issues and I believe the Java implementation will never be as performant as a native solution. Maybe @louwers has some insight into this?

So my idea on a high level would be to allow a line to have different styles for different parts of the line, so maybe the first 50% has style A and the second 50% has style B. This might be useful for quite a lot of different things as well.

}

@Nullable
private Feature createWayPointFeature(RouteLeg leg, int index) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For rendering related things like this, I personally prefer protected to allow customisation, but since we have an interface, implementers could also overwrite the whole class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm on your side, I will have a look and make all logic that make sense to protected 👍

@Fabi755
Copy link
Collaborator Author

Fabi755 commented May 31, 2024

For example nearest point on line can be quite costly for larger geometries.

I changed now the logic. The route is prepared and calculated on setRoute. This will reduce the calculations on updateRouteProgress. Only the animated path needs to be calculated, and the current step needs splitted into two features.

This works very well for the MapLibrePrimaryRouteDrawer (without congestion). And okay working for long routes. Only the setRoute hangs on initial preparements, but I think, we can outsource to a background task.

The congestion drawing also works, but is still lagging a little bit. But these congestion states are a lot of small segments to draw.. there are potential to improve here much more.

@Fabi755
Copy link
Collaborator Author

Fabi755 commented May 31, 2024

I would suggest now, that I clean up the code, add missing things like documentation and tests. And try again to give more performance to the drawing logic.

Then I would expect that we can merge this into main. The route eating is disabled by default, and the users can test and decide by them self if they want to use this feature. Maybe some other developer has good ideas for more improvements later.

Any objections?

@boldtrn
Copy link
Collaborator

boldtrn commented Jun 3, 2024

Fine by me, thanks for working on this. The only important thing right now from point of view is, that we don't create performance issues for non route-eating users.

@juschmitt
Copy link
Contributor

Hey @Fabi755
is this feature still being worked on?
We're currently depending on this PR in our app, but seeing that it is slowly falling behind main and not receiving any updates we're wondering what needs to be done to get this merged.

If you need any help on this, I'd be happy to help out, just need to know what you need done 🙂

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Nov 26, 2024

We are currently converting the core module (libandroid-navigation) to Kotlin in PR #127. The changes in this route-eating PR affecting more the UI part.

After this is done, I will thinking about how to continue with this feature.

For me are two options a good way:

  1. Solve conflicts and finish this PR
  2. Re-create this logic with using Kotlin as code and create a new PR. This allows us, to make a smarter and also performance improved logic. Additional I have thought about some improvements that not included in this PR yet.

I will continue here, the next weeks. Stay tuned. Or remind me again, if you feel it takes too long 😄

@rubenmx
Copy link
Contributor

rubenmx commented Jan 8, 2025

@Fabi755 We're also interested in this feature 😁

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Jan 8, 2025

Kotlin converting (#133) is near the end. After this, I will continue work on this feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants