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

Tile diff #2719

Draft
wants to merge 67 commits into
base: main
Choose a base branch
from
Draft

Tile diff #2719

wants to merge 67 commits into from

Conversation

TimSylvester
Copy link
Collaborator

Only calculate which tiles were added and removed once per frame instead of separately in each layer update.

Copy link

github-actions bot commented Sep 9, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.2% +20.9Ki  +0.1% +16.0Ki    TOTAL

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

# Conflicts:
#	src/mbgl/renderer/layers/render_circle_layer.cpp
#	src/mbgl/renderer/layers/render_fill_extrusion_layer.cpp
#	src/mbgl/renderer/layers/render_fill_layer.cpp
#	src/mbgl/renderer/layers/render_symbol_layer.cpp
#	src/mbgl/renderer/render_layer.cpp
#	src/mbgl/renderer/sources/render_tile_source.hpp
Copy link

github-actions bot commented Sep 10, 2024

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            -0.0327         -0.0327             0             0             0             0

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

@TimSylvester
Copy link
Collaborator Author

Well, after all that, it doesn't really help at all, and in fact seems to be just a bit slower.

In addition, I thought this was going to simplify things but, because layers can not be updated every frame, the complications involved in re-synchronizing offset the benefits. It does potentially allow the large layer update methods to be split up into three parts, but they remain dependent (updates can find that a remove/add is necessary).

Finally, I think this would actually make it more difficult to transition the layer updates to a background thread.

So I'm leaning towards dropping this one.

image

image

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

Successfully merging this pull request may close these issues.

1 participant