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

Hillshade geometry update fixes #2842

Merged
merged 10 commits into from
Sep 23, 2024
Merged

Conversation

TimSylvester
Copy link
Collaborator

@TimSylvester TimSylvester commented Sep 16, 2024

Fixes a problem where masking changes in hillshade tile geometry are not recognized as updated.

Fixes a bug where vertex attributes on a drawable replaced by ones created earlier are not applied to the underlying buffers. This would have been an issue previously, if the newly-applied attributes were not marked dirty, as with shared static tile geometry.

Fixes a bug where fill updates would apply the fill vertices to the triangulated outline drawables (if enabled), which would lead to errors if they were applied, but they weren't because of the previous item.

Resolves #2838 ?

Also do it in `clear` though that's probably not necessary.
@TimSylvester TimSylvester added the bug Something isn't working label Sep 16, 2024
@TimSylvester TimSylvester self-assigned this Sep 16, 2024
Copy link

github-actions bot commented Sep 16, 2024

Bloaty Results (iOS) 🐋

Compared to main

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

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

Copy link

github-actions bot commented Sep 17, 2024

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            +0.0033         +0.0034             0             0             0             0

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

Copy link

github-actions bot commented Sep 17, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0% +57.2Ki  +0.1% +19.3Ki    TOTAL

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

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +27% +31.9Mi  +424% +25.3Mi    TOTAL

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

@louwers louwers linked an issue Sep 17, 2024 that may be closed by this pull request
@louwers
Copy link
Collaborator

louwers commented Sep 17, 2024

Better, but now some tiles don't get loaded at all until I change the zoom a bit:

591EFB3E-87D3-44A3-B26E-7A978891AECD_1_102_o

6C121030-F114-4AA5-BF46-0F1B691C0F48_1_102_o

@alasram
Copy link
Collaborator

alasram commented Sep 17, 2024

@louwers the behavior in your screenshots is already present in main on Android

…ted earlier are not applied to the underlying buffers.

Fix a bug where fill updates would apply the fill vertices to the outline drawables, leading to errors (if they were applied)
@TimSylvester TimSylvester changed the title Add missing modification flagging in hillshade masking, to match raster Hillshade geometry update fixes Sep 18, 2024
Copy link
Collaborator

@JesseCrocker JesseCrocker left a comment

Choose a reason for hiding this comment

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

I have not looked at the code, but i tested this in my application, and it fixes the issue.

@TimSylvester
Copy link
Collaborator Author

Looks like we have a longstanding issue with updating the vertices without updating the segments. That's fine in Metal, but leads to problems in GL because the segments retain Vertex Array identifiers, so we get GL errors if we actually replace the vertices alone.

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.

@TimSylvester Render tests are failing unfortunately.

@louwers louwers merged commit 84ca231 into maplibre:main Sep 23, 2024
42 checks passed
@louwers louwers deleted the bug/hillshade branch September 23, 2024 15:36
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
Status: Done
Development

Successfully merging this pull request may close these issues.

hillshade rendering artifacts, regression in iOS 6.6.0
5 participants