Elevation per polygon for fill extrusions

[antonmarsden]

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:

After:

[HarelM]

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]

• 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]

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]

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]

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]

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]

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

[antonmarsden]

@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]

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.