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

Elevation per polygon for fill extrusions #3841

Merged

Conversation

antonmarsden
Copy link
Contributor

@antonmarsden antonmarsden commented Mar 14, 2024

This PR is a basic solution to issue #3313, and renders multipolygons at their own centroid's elevation.

It is potentially a breaking change for some edge cases though, and it may be worth introducing a new fill-extrusion style property (e.g., ""fill-extrusion-elevation-per-polygon") to optionally activate this behaviour. Suggestions welcomed :)

  • 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.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

Before:
before

After:
after

@antonmarsden antonmarsden marked this pull request as ready for review March 14, 2024 22:06
@HarelM
Copy link
Member

HarelM commented Mar 15, 2024

Thanks for taking the time to look into this!

This is only relevant for terrain as it will be kept at 0 for non terrain maps, right?
If my above assumption is correct I don't think there's a need to change the style spec for that.
Can you add a render test that demonstrates this issue/solution?
Also, in the linked bug, there's a jump in the elevation between two zoom levels (the buildings starts floating), did you happen to try and solve that as well?

@antonmarsden
Copy link
Contributor Author

  • Fill extrusions render as expected with no terrain
  • Regarding floating buildings, I couldn't exactly reproduce what you experienced when moving between zoom levels. I did notice buildings floating consistently in the same area.
  • This improvement removes most (if not all) of the floating/subsurface issues. I did notice some temporary floating buildings when the DEM tiles had not fully loaded, but these self-corrected.
  • Have not had much to do with this code base yet, but will look into a few test cases in the near future.

@HarelM
Copy link
Member

HarelM commented Mar 15, 2024

Also worth making sure is that buildings with multiple parts are presented correctly after this fix.
We had an issue in the past with terrain and second level building porch etc.

@acalcutt
Copy link
Contributor

acalcutt commented Mar 15, 2024

I just want to mention I was talking to someone in slack a while back that wanted to keep the heights of separate polygons the same when using terrain ( https://osmus.slack.com/archives/C01G3D28DAB/p1705407931974739 ) . I had suggested they could possibly use the Multipolygon grouping to kind of do what they wanted, but the fix we are trying to use here would break that option for them (like I had mentioned to them at the time) . If you did add an option like the 'fill-extrusion-elevation-per-polygon' it could still allow that kind of use case.

@HarelM
Copy link
Member

HarelM commented Mar 15, 2024

I think that the default should be that every polygon would be placed in the right place.
If someone needs something "special" then a design proposal in the spec and a PR with implementation can go a long way :-)

@acalcutt
Copy link
Contributor

acalcutt commented Mar 16, 2024

Makes sense to me and it would fix my original issue #3313 , I had just wanted to bring it up because there may be some cases where this isn't wanted, like we talked about around #3313 (comment) . The comment on slack they were trying to make a crude bridge, where the pillars and other pieces needed to be aligned.

I think it makes sense that the change in this PR is the default.

@HarelM
Copy link
Member

HarelM commented May 19, 2024

@antonmarsden Can you please add a render test so I can merge this?

@antonmarsden
Copy link
Contributor Author

@HarelM I have added a basic test to verify that centroids are created on a per-polygon basis. Have not had much spare time recently, am hoping this is sufficient to move things forward.

@HarelM
Copy link
Member

HarelM commented Jun 2, 2024

Thanks! I was hoping for a render test, something similar to the screenshot you added, but hopefully simpler.
Unit test is great but less powerful in this case I think.

@antonmarsden
Copy link
Contributor Author

@HarelM I assume you mean adding to integration/render/tests?

@HarelM
Copy link
Member

HarelM commented Jun 2, 2024

Yes, create a style with a few building in a multipolygon and use a terrain from other tests to show that the buildings don't have the same elevation.

package.json Outdated Show resolved Hide resolved
@HarelM
Copy link
Member

HarelM commented Jun 3, 2024

CC: @louwers note for parity. Should be excluded as it used terrain.

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.46%. Comparing base (f70ce60) to head (cd15f2e).
Report is 5 commits behind head on main.

Files Patch % Lines
src/data/bucket/fill_extrusion_bucket.ts 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3841      +/-   ##
==========================================
- Coverage   87.57%   87.46%   -0.12%     
==========================================
  Files         242      242              
  Lines       33082    33081       -1     
  Branches     2167     2180      +13     
==========================================
- Hits        28973    28934      -39     
- Misses       3130     3159      +29     
- Partials      979      988       +9     

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

louwers
louwers previously requested changes Jun 3, 2024
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.

Can you move the render tests under terrain since it is related to (and requires) this feature? Then it won't show up as a disparity.

CHANGELOG.md Outdated Show resolved Hide resolved
@HarelM HarelM enabled auto-merge (squash) June 4, 2024 08:34
@louwers
Copy link
Collaborator

louwers commented Jun 4, 2024

Thanks!

@HarelM HarelM merged commit b94fac4 into maplibre:main Jun 4, 2024
15 checks passed
@HarelM HarelM mentioned this pull request Jun 4, 2024
8 tasks
@petrsloup
Copy link
Contributor

@HarelM any plans for a bugfix release soon? We would like to get this fix to production as soon as possible...

@HarelM
Copy link
Member

HarelM commented Jun 13, 2024

Sure, I'll push a release in the next few days.

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

Successfully merging this pull request may close these issues.

None yet

6 participants