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

Add wide vector shader option to Custom Drawable Layer #2183

Merged
merged 55 commits into from
Apr 15, 2024

Conversation

stefankarschti
Copy link
Collaborator

@stefankarschti stefankarschti commented Mar 7, 2024

Introduce a new way to draw lines into the library's CustomDrawableLayer interface, by using a third party shader on the Metal platform.

This shader makes use of instancing, and that is implemented into the toolkit for the Metal backend. OpenGL instancing is not implemented by this PR, but the base interface is common and compatible.

The DrawableBuilder is added two methods to build Wide vector polylines: addWideVectorPolylineLocal and addWideVectorPolylineGlobal. These make use of local (tile) coordinates, and global (geographic) coordinates respectively.

The ExampleCustomDrawableStyleLayerHost class in ExampleCustomDrawableStyleLayer.mm contains the updated examples for adding wide vector polylines through the CustomDrawableLayer interface.

@stefankarschti stefankarschti self-assigned this Mar 7, 2024
Copy link

github-actions bot commented Mar 8, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.4% +74.8Ki  +0.4% +64.0Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2183-compared-to-main.txt

Copy link

github-actions bot commented Mar 11, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.3%  +365Ki  +0.2% +66.8Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2183-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +20% +23.4Mi  +406% +24.3Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2183-compared-to-legacy.txt

Copy link

github-actions bot commented Mar 13, 2024

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            +0.0078         +0.0075             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2183-compared-to-main.txt

@stefankarschti
Copy link
Collaborator Author

image

@stefankarschti
Copy link
Collaborator Author

Simulator Screenshot - iPad (10th generation) - 2024-04-04 at 19 03 13

Simulator Screenshot - iPad (10th generation) - 2024-04-04 at 19 02 06

Simulator Screenshot - iPad (10th generation) - 2024-04-04 at 19 02 34

Simulator Screenshot - iPad (10th generation) - 2024-04-04 at 19 03 02

Simulator Screenshot - iPad (10th generation) - 2024-04-04 at 18 58 06

@stefankarschti stefankarschti marked this pull request as ready for review April 4, 2024 16:07
Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

I hope someone from the Metal team can take a look for an in-depth review.

Can some kind of test be added?

include/mbgl/shaders/mtl/widevector.hpp Outdated Show resolved Hide resolved
include/mbgl/shaders/mtl/widevector.hpp Outdated Show resolved Hide resolved
include/mbgl/shaders/mtl/widevector.hpp Outdated Show resolved Hide resolved
src/mbgl/gfx/drawable_builder_impl.cpp Outdated Show resolved Hide resolved
@stefankarschti
Copy link
Collaborator Author

Can some kind of test be added?

@louwers : This is currently only available through the CustomDrawable API, therefore can't be included in the regular render test suite.
The API tests for CustomDrawableLayer currently don't support Metal, so we can't include a relevant test there either, since we only have a Metal shader in place.

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

@stefankarschti We need some way to at least run the code involved in the Custom Drawable Layer on CI. May I suggest this super dumb 'smoke test' to be added to platform/ios/iosapp-UITests/iosapp_UITests.swift?

    /// Make sure the Custom Drawable Layer does not crash
    func testCustomDrawableLayer() {
        app.windows.children(matching: .other).element.children(matching: .other).element.children(matching: .other).element.doubleTap()
        
        let mapSettingsButton = app.navigationBars["MapLibre Basic"].buttons["Map settings"]
        mapSettingsButton.tap()
        
        let tablesQuery = app.tables
        tablesQuery.staticTexts["Add Custom Drawable Layer"].tap()
        sleep(5)
    }

It only makes sure the Custom Drawable Layer does not crash. Then we can worry about how to test for correctness later.

@stefankarschti stefankarschti marked this pull request as draft April 12, 2024 20:02
@stefankarschti stefankarschti marked this pull request as ready for review April 15, 2024 12:40
@louwers louwers merged commit c6bcb76 into maplibre:main Apr 15, 2024
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants