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

Remove Pass3D #3077

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Remove Pass3D #3077

merged 1 commit into from
Dec 13, 2024

Conversation

TimSylvester
Copy link
Collaborator

... to prevent a bug where removeTile(Translucent|Pass3d) doesn't remove drawables added with just Translucent.

Resolves #3039

…)` doesn't remove drawables added with just `Translucent`.
@TimSylvester TimSylvester added the bug Something isn't working label Dec 10, 2024
@TimSylvester TimSylvester self-assigned this Dec 10, 2024
Copy link

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0%    +104  [ = ]       0    TOTAL

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

Copy link

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  -0.0%    -488  -0.0%    -168    TOTAL

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

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +30% +34.5Mi  +432% +25.8Mi    TOTAL

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

Copy link

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            +0.0074         +0.0074             0             0             0             0

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

@sjg-wdw
Copy link
Collaborator

sjg-wdw commented Dec 13, 2024

How do we feel about merging this before Tim goes on vacation?

@louwers louwers requested a review from alexcristici December 13, 2024 19:47
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.

Sounds like a good idea to get a final bugfix in before the end of the year. We can always revert it if it causes any problems.

Seems like the 3D pass is even more dead code now. But I am not too worried about it with the revival on the roadmap.

Would be good to have a regression test for this, I tried (#3073) but it passes even before this.

@TimSylvester
Copy link
Collaborator Author

Yeah, I never figured out why it doesn't fail in that test.

@TimSylvester TimSylvester merged commit b44b644 into maplibre:main Dec 13, 2024
50 checks passed
@TimSylvester TimSylvester deleted the bug/3039 branch December 13, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filters applied to fill extrusion layers are not rendered unless a manual zoom is applied to the map
3 participants