-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add distance
expression support
#642
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #642 +/- ##
==========================================
+ Coverage 92.23% 92.45% +0.22%
==========================================
Files 100 104 +4
Lines 4196 4624 +428
Branches 1203 1303 +100
==========================================
+ Hits 3870 4275 +405
- Misses 326 349 +23 ☔ View full report in Codecov by Sentry. |
FYI: @louwers, thanks for the pointers! |
@@ -187,6 +188,7 @@ export { | |||
format, | |||
validate, | |||
migrate, | |||
classifyRings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exported here in order to remove it from maplibre-gl code base.
I'm happy with the tests coverage and the implementation, it found some bugs I needed to fix from wrong conversion of code and differences between c++ and javascript (integer division). |
Also @louwers I think the following line should use the point constant and not the line constant: I don't think it will dramatically affect the algorithm, but when I looked at the code and converted it, I came across this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar enough with the math to check it, but it looks good to me in general. Certainly great job on improving typings, and the tests look comprehensive.
This adds about 7k unzipped to the bundle size (~1%). It's not negligible but it's part of the spec, so I don't see a way around this. |
Launch Checklist
This allows the ability to use
distance
expression in web.distance
expression maplibre-gl-js#2077This is heavily based on the c++ native implementation:
https://github.com/maplibre/maplibre-native/blob/cd6a0ab11fcd9b5bbfee87896a14159d0ed56256/src/mbgl/style/expression/distance.cpp
https://github.com/maplibre/maplibre-native/blob/cd6a0ab11fcd9b5bbfee87896a14159d0ed56256/src/mbgl/tile/geometry_tile_data.cpp
https://github.com/maplibre/maplibre-native/blob/cd6a0ab11fcd9b5bbfee87896a14159d0ed56256/src/mbgl/util/geometry_util.cpp
CHANGELOG.md
under the## main
section.