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

Upgrade to MapLibre 11.0.0 #110

Merged
merged 23 commits into from
Nov 8, 2024
Merged

Conversation

Fabi755
Copy link
Collaborator

@Fabi755 Fabi755 commented Jun 7, 2024

Upgrade MapLibre to major version 11.x.x.

Closes #104

Following things have done for this:

  1. Upgrade MapLibre
    a. Update maplibre-native to version 11.0.0
    b. Update maplibre-java modules to version 6.0.1
    c. Rename affected imports
    d. Replace MapLibre hosted mapbox-sdk-services (5.9.0) by Mapbox hosted mapbox-sdk-services (5.8.0) to solve package conflicts. This step was required because the project is currently using some classes from the services module. But the module was removed from the MapLibre project, and therefore not upgraded to the new package name.
  2. Rename Mapbox classes to MapLibre

Things we should thinking about, before release the new major version:

  1. Change package naming/structure. For example remove v5 from path.
  2. Rename the Gradle modules. For example in core & ui.

Please note that the GL drawing is crashing (on emulator ?!) when roating the map. See: maplibre/maplibre-native#2371.

@Fabi755 Fabi755 added the dependencies Issue or pull request for updating dependencies label Jun 7, 2024
@Fabi755 Fabi755 requested a review from boldtrn June 7, 2024 22:16
@Fabi755 Fabi755 self-assigned this Jun 7, 2024
@Fabi755 Fabi755 changed the title Feature/maplibre 11 Upgrade to MapLibre v11 Jun 7, 2024
@Fabi755 Fabi755 changed the title Upgrade to MapLibre v11 Upgrade to MapLibre 11.0.0 Jun 7, 2024
@Fabi755 Fabi755 force-pushed the feature/maplibre-11 branch 3 times, most recently from e695301 to ddc8848 Compare June 10, 2024 21:35
@Fabi755 Fabi755 changed the title Upgrade to MapLibre 11.0.0 WIP: Upgrade to MapLibre 11.0.0 Jun 10, 2024
@Fabi755
Copy link
Collaborator Author

Fabi755 commented Jun 10, 2024

I see always existing Mapbox names on resources that could changed. I will updates this near time.

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Jun 14, 2024

In favor of maplibre/maplibre-native#2505 I noticed that the layer IDs of the native project always on mapbox naming. We need a follow up update, if the changes are released on their site.

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Jun 14, 2024

I tried to rename all Mapbox names to MapLibre. But while we use at the moment Mapbox requests as our default API, it was not very easy to differentiate the components and which names should renamed and which not.

Feel free to have an own look, and give some hints to missing parts. Alternative we can create a follow up PR when we detect missing things.

From my side this PR is ready to merge. A test usage has worked for me.

To force Jitpack to rebuild I create a tag. I added the library with

implementation 'com.github.fabi755:maplibre-navigation-android:jitpack-test1

@Fabi755 Fabi755 changed the title WIP: Upgrade to MapLibre 11.0.0 Upgrade to MapLibre 11.0.0 Jun 14, 2024
@nitrag
Copy link

nitrag commented Jun 28, 2024

@boldtrn Can you review this?

@boldtrn
Copy link
Collaborator

boldtrn commented Jun 28, 2024

@nitrag this PR is still on my review list. Please note, this PR is huge, while a lot of the changes are simple renames, I still have to take a look. If you like to help with the review, we are grateful for any feedback you have, so please feel free to also review the code and let us know if you spot an issue. Feel free to run the sample code an integrate this in your project so we catch possible issues early on.

@rubenmx
Copy link
Contributor

rubenmx commented Jul 1, 2024

@Fabi755 Thanks for taking the time to do this! While implementing your changes into our Flitsmeister app we noticed two things in NavigationRoute:

  • The origin(Point, Double, Double) function on line 239 still uses com.mapbox.geojson.Point instead of org.maplibre.geojson.Point. I see you did change it in the other variant, so the toMapboxPoint() needs to be moved to this one.
  • The addWayPoint(Point, Double, Double) function on line 314 also uses com.mapbox.geojson.Point instead of org.maplibre.geojson.Point (this also requires the MapLibreRouteFetcher to change to org.maplibre.geojson.Point).

After editing this, and changing all MapBox references to MapLibre, our app ran perfectly fine 👍. Edit; when calculating a route using the NavigationRoute.builder, the app crashes on java.lang.NoClassDefFoundError: Failed resolution of: Lcom/mapbox/api/directions/v5/MapboxDirections;. Could also have something to do with adding this sdk as a local aar file to test. I also needed to add retrofit2 dependencies.

@frankkienl
Copy link
Collaborator

Can this PR be merged, or are additional changes needed?

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Jul 26, 2024

There are still issues, that I need to fix. I'm currently on holiday until august. I will continue working after this.

@nitrag
Copy link

nitrag commented Aug 29, 2024

I'll be happy to test this when it's finished.

@frankkienl
Copy link
Collaborator

Oh dear, there are merge conflicts :/

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Sep 19, 2024

@Fabi755 Thanks for taking the time to do this! While implementing your changes into our Flitsmeister app we noticed two things in NavigationRoute:

The origin(Point, Double, Double) function on line 239 still uses com.mapbox.geojson.Point instead of org.maplibre.geojson.Point. I see you did change it in the other variant, so the toMapboxPoint() needs to be moved to this one.
The addWayPoint(Point, Double, Double) function on line 314 also uses com.mapbox.geojson.Point instead of org.maplibre.geojson.Point (this also requires the MapLibreRouteFetcher to change to org.maplibre.geojson.Point).

Fixed!

when calculating a route using the NavigationRoute.builder, the app crashes on java.lang.NoClassDefFoundError: Failed resolution of: Lcom/mapbox/api/directions/v5/MapboxDirections;. Could also have something to do with adding this sdk as a local aar file to test. I also needed to add retrofit2 dependencies.

In the example project it works fine. Do you use the NavigationRoute from the right package org.maplibre.navigation.android.navigation.ui.v5.route)?


Feel free to test the recent changes again and let me know if something is broken. But I noticed some issue with the location puck (since v11.0.1). I'm not sure yet, whats causing this.

@rubenmx
Copy link
Contributor

rubenmx commented Sep 20, 2024

@Fabi755 Thanks, everything seems to work so far! The last time I tested it everything worked on emulator as well, but now the map seems to crash on emulator. I guess I should report that to the native team.

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Sep 20, 2024

Crashes on the emulator are known issues at the native project. There already exist issues on native project:

But sure, that is more an issue of the native project, and not part of the navigation 👍


Seems like it's fixed in 1.5.x:

@rubenmx
Copy link
Contributor

rubenmx commented Sep 20, 2024

Thanks, I'm subscribing to that issue. Hopefully it can be fixed 🤞

@ravenfeld
Copy link

As it's a big refacto, I think it should be merged before other PRs come into conflict with it.

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Oct 26, 2024

When other things come up, and a conflict will created, it's easy to solve, while this changes are only renamings :-)

But this will merged soon. Review is already in progress.

LICENSE Outdated
@@ -1,8 +1,6 @@
The MIT License (MIT)

Copyright (c) 2021 Maplibre
Copyright (c) 2019 Flitsmeister
Copyright (c) 2018 Mapbox
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove previous copyrights? I think we have to keep them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we have an legal expert at MapLibre?

But yes, I can roll this also back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe @ramyaragupathy knows more? My understanding is that we can't just remove copyrights.

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 rolled back this for now. We can change this later if necessary.

options.windowTitle("Mapbox Android Navigation UI SDK $VERSION_NAME Reference")
options.docTitle("Mapbox Android Navigation UI SDK $VERSION_NAME")
options.header("Mapbox Android Navigation UI SDK $VERSION_NAME Reference")
options.bottom("© 2019 Mapbox. All rights reserved.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this copyright notice also be deleted/kept?
( I am not a lawyer )

It might be silly to delay this PR, just because of some copyright notices;
So maybe it's better to check that in a separate issue/PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also not a lawyer, but the copyright seems to be wrong anyway, as the repo is under MIT, so I think it should be alright to do this, but maybe link to the license file or something like that (if it would be hosted on some webpage, which I don't think it is right now?). Either way, we are currently testing and reviewing this PR on our end, so it's not blocked due to that, sorry if there isn't a lot of feedback right now I hope we have some by the end of next week.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That said, please feel free to also review the code @frankkienl - my plan would be to merge this ASAP (which is more like 1-2 weeks or so).

options.windowTitle("Mapbox Android Navigation SDK $VERSION_NAME Reference")
options.docTitle("Mapbox Android Navigation SDK $VERSION_NAME")
options.header("Mapbox Android Navigation SDK $VERSION_NAME Reference")
options.bottom("© 2017 Mapbox. All rights reserved.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here ;-) (see comment above)

Copy link

@sotomski sotomski left a comment

Choose a reason for hiding this comment

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

Hi folks, inspired by @boldtrn, I've spent some time looking through the changes and integrating this PR into the Kurviger app.
Apart from a blinking puck icon (presumably fixed in maplibre/maplibre-native#2879), things are looking good on our side 👍 Thanks!

build.gradle Outdated Show resolved Hide resolved
@boldtrn
Copy link
Collaborator

boldtrn commented Nov 5, 2024

Ideally the puck flickering will be fixed with the next release of Maplibre-Native maplibre/maplibre-native#2992

@boldtrn
Copy link
Collaborator

boldtrn commented Nov 7, 2024

The flickering issue has been resolved and this PR should be almost ready to be merged, if anyone finds an issue, please report this very soon or this PR will be merged (we can always include fixes later as well).

Copy link
Collaborator

@boldtrn boldtrn left a comment

Choose a reason for hiding this comment

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

LGTM, I'd vote to merge this whenever you are ready @Fabi755 👍

@Fabi755 Fabi755 merged commit b0d2f57 into maplibre:main Nov 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Issue or pull request for updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update for MapLibre Native Android 11.0.0
7 participants