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

map: Transfer to MapLibre #31185

Merged
merged 20 commits into from
Feb 1, 2024
Merged

map: Transfer to MapLibre #31185

merged 20 commits into from
Feb 1, 2024

Conversation

bongbui321
Copy link
Contributor

@bongbui321 bongbui321 commented Jan 27, 2024

Description
Currently translate the codebase to maplibre-qt already.

Verification
Trying to build maplibre-qt, but is currently facing an issue similar to this. Failing to build because of missing qtlocation5-private-dev package and it is not available on apt

Will test using map_renderer.py, not sure if this is a good way to test this? Probably better test with replay

@bongbui321 bongbui321 marked this pull request as draft January 27, 2024 00:05
@jnewb1 jnewb1 linked an issue Jan 27, 2024 that may be closed by this pull request
@adeebshihadeh
Copy link
Contributor

Will test using map_renderer.py, not sure if this is a good way to test this? Probably better test with replay

I'd probably start with replaying the demo route and just checking the map generally works in the UI, then test the map renderer. See here for getting setup: https://github.com/commaai/openpilot/tree/master/selfdrive/navd#development

@adeebshihadeh
Copy link
Contributor

What does full desktop support mean? We can't make this work with 5.12?

@bongbui321
Copy link
Contributor Author

bongbui321 commented Jan 27, 2024

@adeebshihadeh
From the README, it is "fully supported on desktop platform" which I think for the main desktop OSes. I think it is fine since Agnos is mainly based on Ubuntu 20.04?

anything below 5.15 doesn't support Location which I think is used for getting routes and positions. I think we can kind of work around this if we have our own fork. I haven't dive deep into the codebase, but it might be possible

@adeebshihadeh
Copy link
Contributor

adeebshihadeh commented Jan 27, 2024

Ah, if there's lot of hacks, then I don't think this is worth it. We'll be switching to 24.04 in a few months when it's out anyway. If you confirm there are lots of hacks to get it working on our Qt, then we can just keep this bounty locked to you until the switch.

@bongbui321
Copy link
Contributor Author

bongbui321 commented Jan 28, 2024

Okay, I made it work with 5.12. I made a small PR that makes it compilable with Qt 5.12. We don't need Location since we are getting position and location from cereal already. so we can disable the Location option with
cmake -DMLN_QT_WITH_LOCATION=OFF ..

I was trying to test with the demo route of replay but that one doesn't seem to have map enabled, is there a different route that I can test with?.
I also test with map_renderer.py and got a segfault. I tried to implement the exact same function in .c here is the file:
test.txt and it works just fine with no segfault, not entirely sure why it doesn't work for python implementation

@adeebshihadeh
Copy link
Contributor

Okay, I made it work with 5.12. I made a small PR that makes it compilable with Qt 5.12. We don't need Location since we are getting position and location from cereal already. so we can disable the Location option with cmake -DMLN_QT_WITH_LOCATION=OFF ..

I was trying to test with the demo route of replay but that one doesn't seem to have map enabled, is there a different route that I can test with?. I also test with map_renderer.py and got a segfault. I tried to implement the exact same function in .c here is the file: test.txt and it works just fine with no segfault, not entirely sure why it doesn't work for python implementation

Nice! I see that got merged already.

Testing it doesn't require a route with nav enabled; the map will be available if you have everything setup, such as a token.

@bongbui321
Copy link
Contributor Author

@adeebshihadeh it seems to work perfectly now, do you need me to upload the compiled library or do you want me to write a shell to download and set it up automatically. Here is a snapshot of the demo with maplibre. Still figuring out how to record videos :)
image

@adeebshihadeh
Copy link
Contributor

Great. We always want a script to build the third_party dependencies so that they're reproducible, then you can check them in once that's done. This one is a good example https://github.com/commaai/openpilot/blob/master/third_party/acados/build.sh

@adeebshihadeh
Copy link
Contributor

@mitchellgoffpc any validation you want to do before merging this?

@bongbui321
Copy link
Contributor Author

@adeebshihadeh added the build script

I also test with map_renderer.py and got a segfault. I tried to implement the exact same function in .c here is the file: test.txt and it works just fine with no segfault, not entirely sure why it doesn't work for python implementation

The UI works fine, if possible you can test it out just to make sure I didn't introduce any bug, but the map_renderer.py doesn't seem to work. I created the same code in .c file as above and it doesn't have any segfault, I'm not sure why it doesn't work

@bongbui321 bongbui321 changed the title Transferring to MapLibre map: Transfer to MapLibre Jan 30, 2024
@mitchellgoffpc
Copy link
Contributor

@mitchellgoffpc any validation you want to do before merging this?

Yeah we should verify that it doesn't impact model performance, will run those tests tomorrow morning.

@bongbui321 bongbui321 marked this pull request as ready for review January 30, 2024 21:44
Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

I haven't run it yet, but it looks pretty good! I left a few minor things to cleanup.

@adeebshihadeh
Copy link
Contributor

CI is passing now, but there's some issues building it on device (CMake is too old and it's missing libqconnmanbearer.so). I'll look into those when I get a chance.

@adeebshihadeh
Copy link
Contributor

Got it working on the comma 3X, and all the tests pass except navModel replay. I'll update the refs once you verify @mitchellgoffpc.

@mitchellgoffpc
Copy link
Contributor

@adeebshihadeh lgtm, model behavior appears to be pretty much identical

@adeebshihadeh adeebshihadeh merged commit 0803759 into commaai:master Feb 1, 2024
25 of 26 checks passed
@bongbui321 bongbui321 deleted the maplibre branch February 1, 2024 23:18
@SurferSD

This comment was marked as resolved.

@bongbui321
Copy link
Contributor Author

@SurferSD please use the build script, and i think its better to open an issue rather than commenting on Pr since it mostly relates to development

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

Successfully merging this pull request may close these issues.

[$300 bounty] nav: switch to MapLibre widget
4 participants