-
-
Notifications
You must be signed in to change notification settings - Fork 396
Add polylines with geographic coordinates #3547
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
base: main
Are you sure you want to change the base?
Conversation
Bloaty Results 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3547-compared-to-main.txtCompared to d387090 (legacy)
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3547-compared-to-legacy.txt |
Benchmark Results ⚡
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-3547-compared-to-main.txt |
Bloaty Results (iOS) 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-3547-compared-to-main.txt |
@@ -519,8 +519,23 @@ util::SimpleIdentity CustomDrawableLayerHost::Interface::addPolyline(const LineS | |||
|
|||
switch (shaderType) { | |||
case LineShaderType::Classic: { | |||
// TODO: build classic polyline with Geo coordinates | |||
return util::SimpleIdentity::Empty; | |||
// build classic polyline with Geo coordinates |
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 was left out because the transformation matrixes don't have enough precision at global scale with the standard shader, but it's handled in the Wide Vector.
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.
check out the usage of diff matrixes to enhance precision in wide vector metal shader
struct Uniforms |
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 mostly a backup for platforms/backends that don't have WideVector implementations (yet).
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.
ok, but you should check the results, as the lack of precision makes it really unusable past a certain zoom level.
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.
could we document this limitation, in code comments or other docs?
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 thinking of moving this bit in the example code (with comments) and removing the line type selection on addPolyline
(global) for now.
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've been looking over the different matrices, but only mvpMatrix
is used in the shader and this should be the tile matrix (same as the line shader). Is there something else that I'm missing or the extra precision is from the different input types (float3 vs short2)?
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.
indeed, looks like the diff is not used in the final version.
if the geometry is stable at zoom >= 20
then go ahead with the changes
Enable adding polylines with geographic coordinates for custom drawables