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

Improvements to marker transparency #3431

Merged
merged 25 commits into from
Dec 6, 2023
Merged

Conversation

SnailBones
Copy link
Contributor

@SnailBones SnailBones commented Dec 1, 2023

Fixes #2869. Markers now update when removing terrain.

disable-terrain.webm

Fixes #1134.

Instead of checking the position of the marker and the unprojected screenpoint in LngLat space, now we sample the depth texture (with function terrain.depthAtPoint) and convert the marker's position to depth in GL space (with function transform.lngLatToCameraDepth) and compare these values. For more precision, I've adjusted the near plane. This means that buildings clip slightly sooner but it doesn't seem to be an issue. I reverted this change as it caused incorrect render order with DeckGL and extrusions. Some precision is lost but given the following double-check it's rarely noticeable.

Fixes #2870

If the initial check at the marker's lng/lat finds a colission we check again at a position with elevation adjusted to the center of the marker. This resolves this issue where markers were being occluded by a small hill at the base that only covers a small portion of the marker.

fade-behind-hill.webm

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (bd6780a) 74.72% compared to head (55fd5f0) 74.74%.

Files Patch % Lines
src/render/terrain.ts 0.00% 8 Missing ⚠️
src/gl/framebuffer.ts 0.00% 1 Missing ⚠️
src/ui/marker.ts 96.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3431      +/-   ##
==========================================
+ Coverage   74.72%   74.74%   +0.01%     
==========================================
  Files         242      242              
  Lines       19113    19146      +33     
  Branches     4233     4240       +7     
==========================================
+ Hits        14283    14311      +28     
- Misses       4830     4835       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SnailBones SnailBones changed the title Fix marker remaining transparent after removing terrain Improvements to marker transparency Dec 4, 2023
@SnailBones SnailBones marked this pull request as ready for review December 4, 2023 22:45
src/geo/transform.ts Outdated Show resolved Hide resolved
src/render/terrain.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Dec 5, 2023

Looks good in general, can you add some videos with 2D and 3D after the fix?

@HarelM
Copy link
Collaborator

HarelM commented Dec 5, 2023

I see that some tests are failing...
Also I'm not sure about the accuracy of the coverage, but it should be higher...

@SnailBones
Copy link
Contributor Author

SnailBones commented Dec 5, 2023

Thanks for the review @HarelM! There's no more consistently failing tests, I'm only seeing failures from flaky tests that are probably unrelated to this PR.

Also I'm not sure about the accuracy of the coverage, but it should be higher...

Just added a unit test for transform.lngLatToCameraDepth! Unfortunately I don't think there's a simple way to unit testterrain.depthAtPoint .

src/ui/map.test.ts Outdated Show resolved Hide resolved
src/geo/transform.ts Outdated Show resolved Hide resolved
src/ui/marker.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Dec 6, 2023

In general this looks good. See my last comment above.

@HarelM
Copy link
Collaborator

HarelM commented Dec 6, 2023

I have merged node 20 branch just now, which changes a bit how we use webgl in the jest tests, let me know if you need help converting the test to the new way.
Sorry, was planning to do it after this PR was merged but merged it too soon by accident...

Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

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

THANKS!!

@HarelM
Copy link
Collaborator

HarelM commented Dec 6, 2023

According to the coverage the following lines are still not covered:

marker.ts                                  |   96.09 |     88.7 |   90.62 |    96.7 | 266,462-466,516,520,534-535 

516, 520, and 534-535 are part of the changes made here.
Let me know if you would like to write tests to cover those, I don' know how critical those are, but it's probably easier to write now than later...

@SnailBones
Copy link
Contributor Author

SnailBones commented Dec 6, 2023

516, 520, and 534-535 are part of the changes made here.
Let me know if you would like to write tests to cover those, I don' know how critical those are, but it's probably easier to write now than later...

@HarelM Just expanded the test, I think it should cover most of those!

How are you seeing the list of lines not covered? Going to https://app.codecov.io/gh/maplibre/maplibre-gl-js/pull/3431 I see "No Files covered by tests were changed"

EDIT: Got them all but 520, not sure how to cover that one.

@HarelM
Copy link
Collaborator

HarelM commented Dec 6, 2023

Not sure why codecov is not presenting the lines...
I looked in the CI, it prints the coverage to the console as a backup.

@HarelM HarelM merged commit 7e45696 into maplibre:main Dec 6, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants