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.

Sign up for GitHub

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

Jump to bottom

Activating 3D terrain distorts 3D building parts #2513

Closed
zstadler opened this issue May 13, 2023 · 8 comments · Fixed by #2541
Closed

Activating 3D terrain distorts 3D building parts #2513

zstadler opened this issue May 13, 2023 · 8 comments · Fixed by #2541
Assignees
@zstadler
Labels
💰 bounty XL Extra Large Bounty, USD 1000 bug Something isn't working terrain
Milestone
3.0.0

Comments

@zstadler
Copy link
Contributor

zstadler commented May 13, 2023 •
edited
Loading

Note: I Edited the description, updated screenshots, and removed no-longer-necessary references to openmaptiles/openmaptiles#1541

When Activating 3D Terrain, 3D building parts are enlarged and strange overlaps may occur:

image

Compare with the same location and viewpoint, with 3D Terrain inactive

image

maplibre-gl-js version:
2.4.0

browser:
Chrome 113.0.5672.93
Firefox 112.0.2

Steps to Trigger Behavior

Expected Behavior

The shapes and the relative positions of the 3D building parts should not change when 3D terrain is activated or deactivated.

Actual Behavior

See the above differences
@HarelM HarelM added bug Something isn't working terrain labels May 13, 2023
@HarelM
Copy link
Collaborator

HarelM commented May 13, 2023 •
edited
Loading

Thanks for taking the time to report this.
Adding XL bounty.
Bounty direction:
maplibre/maplibre#189

@HarelM HarelM added the 💰 bounty XL Extra Large Bounty, USD 1000 label May 13, 2023
@prozessor13
Copy link
Collaborator

Each fill-extrusion object is elevated by the altitude at its centroid coordinate. The centroid is calculated in https://github.com/maplibre/maplibre-gl-js/blob/main/src/data/bucket/fill_extrusion_bucket.ts#L162 . So, if a building has several parts, the centroid should somehow the same value. May this can be a data-value, or an expression in style. I dont know, but i think the current maplibre tools/logic is not sufficient to fix this issue.

@HarelM
Copy link
Collaborator

HarelM commented May 15, 2023

Interesting, thanks for the valuable input @prozessor13!

@zstadler
Copy link
Contributor Author

zstadler commented May 16, 2023 •
edited
Loading

@prozessor13,
If we look at this animated gif, we can see the "ceilings" are moving up while the "floors" are moving down when 3D terrain is activated.

3Dbuilding-bug

@prozessor13
Copy link
Collaborator

I think the "ceilings" moving up because of terrain. the floors are moving down because of https://github.com/maplibre/maplibre-gl-js/blob/main/src/shaders/fill_extrusion.vertex.glsl#L31. But you are right, here is a bug/problem: https://github.com/maplibre/maplibre-gl-js/blob/main/src/shaders/fill_extrusion.vertex.glsl#L39
The baseDelta offset should only applied for base 0, and even with this fix it will generate errors for buildings below terrain (if this is an issue??). In general, without some baseDelta logic, you get really a lot of ugly artefacts in heavy terrain. Are you willed to test/fixing this?

@zstadler zstadler mentioned this issue May 16, 2023
Merged
5 tasks
@birkskyum birkskyum added this to the 3.0.0 milestone May 20, 2023
HarelM pushed a commit that referenced this issue May 21, 2023
@zstadler
Fix 3D building offsets in 3D Terrain mode (#2541)
68ea301 
Fixes #2513
Fixes #2544 

* Fix 3D building offsets in 3D Terrain mode

* Rename variables for clarity

* Fix `base_terrain3d_offfset`

Code review comments:
- Substract, rather than add, 10 meters from `height_terrain3d_offfset`
- Spelling error in variable names

* Keep build-dist result as an artifact

* change build-dist result source location

* Update CHANGELOG.md

* Improve comments in the code

Clarify the original intentions of the modified calculations

* Apply the same changes to `fill_extrusion_pattern`

* Add a test

* Add dummy `expected.png`

* Use properly-sized dummy expected

* Use proper expected

* Move test location below sea level

* Increase delay for test results

* Revert "Increase delay for test results"

This reverts commit 5a06650.
Wrong file was commited

* Increase delay for test results

* test workaround: reduce pitch

* Increase delay for test results

* test: temporarily disable other tests

* test: temporarily disable more tests

* test: temporarily disable more tests

* test: Add debug, reset bearing and sleep

* test: move debugging to workflows

* test: try another place for `--debug`

* test: use `wait`, add terrain tiles

* test: another try

* test: update expected, modify --debug

* test: rename TerrainRGB tiles and use them

* test: use untimed `wait` operation

* tidy-up

* Review comments
@zstadler
Copy link
Contributor Author

I'm planning to claim this bounty once the paper work is completed.
Thanks for your patience.

@HarelM
Copy link
Collaborator

HarelM commented Aug 17, 2023

I have submitted the following expense request on behalf of @zstadler:
https://opencollective.com/maplibre/expenses/156265
@zstadler please approve that this was requested by you.

@zstadler
Copy link
Contributor Author

Thank you for submitting the request on my behalf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zstadler zstadler

Labels
💰 bounty XL Extra Large Bounty, USD 1000 bug Something isn't working terrain
Projects
None yet
Milestone
3.0.0
Development

Successfully merging a pull request may close this issue.

Fix 3D building offsets in 3D Terrain mode zstadler/maplibre-gl-js
4 participants
@prozessor13 @zstadler @HarelM @birkskyum