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

Fix camera under terrain when exaggeration changes #11578

Merged
merged 2 commits into from
Mar 11, 2022

Conversation

astojilj
Copy link
Contributor

@astojilj astojilj commented Mar 9, 2022

Fixes issues when camera center would not change altitude if exaggeration changed and that camera could appear under terrain.

Changelog:

Fixes issues where camera appears under terrain, or map gets bumpy repositioning when exaggeration changes.

Similar to #11461, the customer was affected (https://github.com/mapbox/mapbox-maps-android-internal/issues/850) by camera moved underground when animating exaggeration, increasing it from 0. In such use case, we didn't adjust center and camera MSL altitude with exaggeration change.

There is a chance that previous behavior, that is anchoring camera center to sea level while terrain elevates, is desired. However, it is not productized and not feasible in high altitude areas (e.g. camera positioned over Denver would quickly go underground when changing exaggeration 0 -> 0.5). There's also issue with position of e.g. markers noticeable on the capture (before, under terrain at the end of video):

before.mov

The change aligns the behavior to existing terrain enable / disable: content under center of screen remains under the center of screen at the same distance when terrain is enabled and when exaggeration changes.

Extended usage of Transform._centerAltitudeValid to renamed _centerAltitudeValidForExaggeration: while data is not available, _centerAltitudeValidForExaggeration is 0. As soon as center elevation is available, new value for _centerAltitudeValidForExaggeration would trigger camera altitude adjustment in updateElevation.

Fog demo: change exaggeration by pitch and zoom options
Added exaggeration change on zoom (issue #11044 is still open) and pitch (bug #11589) to fog demo.

Fog change:
There was a flicker when disabling terrain, related to average elevation calculated before terrain update (fog would render during the first frame using high elevation), followed by fog transition animation. Average elevation easing is disabled for camera exaggeration change.

Screenshot 2022-03-09 at 11 22 26

fogtransitionbefore.mov

After fix there is still flicker noticeable when exaggeration changes to 0. This one is related to tile cover missing tiles (fog matrices used in rendering are up to date):

after.mov

Follow up (a separate issue that exists in main branch too):

Fixes: #11589

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • keep existing behavior for drag and zoom
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

Fixes issues when camera center would not change altitude if exaggeration changed and that camera could appear under terrain.

Similar to #11461, the customer was affected (https://github.com/mapbox/mapbox-maps-android-internal/issues/850) by camera moved underground when animating exaggeration, increasing it from 0 in high altitude areas. In such an use case, we didn't adjust center and camera MSL altitude with exaggeration change.

Extended usage of Transform._centerAltitudeValid to renamed _centerAltitudeValidForExaggeration: while data is not available, _centerAltitudeValidForExaggeration is 0. As soon as center elevation is available, new value for _centerAltitudeValidForExaggeration would trigger camera altitude adjustment in updateElevation.
@astojilj astojilj requested a review from karimnaaji March 9, 2022 09:55
@astojilj astojilj self-assigned this Mar 9, 2022
@astojilj
Copy link
Contributor Author

astojilj commented Mar 10, 2022

The PR doesn't handle drag and zoom properly (sea elevation reference) when exaggeration changes with zoom

drag.mov

Needs to be resolved within this PR.

Edit:
This exists in main branch too, #11044 (comment) and needs to be fixed later.

Added exaggeration change on zoom (issue  #11044 is still open) and pitch (bug #11589) to fog demo.

Fixes: #11589
@astojilj
Copy link
Contributor Author

The PR doesn't handle drag and zoom properly (sea elevation reference) when exaggeration changes with zoom

drag.mov
Needs to be resolved within this PR.

This issue exists in main branch too: #11044 (comment) and needs to be fixed later.

Added exaggeration change on pitch (bug #11589) to fog demo can be used to verify that this fixes issue #11589):

fix.mov

@karimnaaji
Copy link
Contributor

This issue exists in main branch too: #11044 (comment) and needs to be fixed later.

Thanks for capturing that issue!

@karimnaaji karimnaaji merged commit b0f9af0 into main Mar 11, 2022
@karimnaaji karimnaaji deleted the astojilj-exaggeration branch March 11, 2022 23:47
ahocevar pushed a commit to ahocevar/mapbox-gl-js that referenced this pull request Mar 23, 2022
* Fix camera under terrain when exaggeration changes

Fixes issues when camera center would not change altitude if exaggeration changed and that camera could appear under terrain.

Similar to mapbox#11461, the customer was affected (https://github.com/mapbox/mapbox-maps-android-internal/issues/850) by camera moved underground when animating exaggeration, increasing it from 0 in high altitude areas. In such an use case, we didn't adjust center and camera MSL altitude with exaggeration change.

Extended usage of Transform._centerAltitudeValid to renamed _centerAltitudeValidForExaggeration: while data is not available, _centerAltitudeValidForExaggeration is 0. As soon as center elevation is available, new value for _centerAltitudeValidForExaggeration would trigger camera altitude adjustment in updateElevation.

* Fog demo: change exaggeration by pitch and zoom options

Added exaggeration change on zoom (issue  mapbox#11044 is still open) and pitch (bug mapbox#11589) to fog demo.

Fixes: mapbox#11589

Co-authored-by: Aleksandar Stojiljković <aleksandar.stojijkovic@mapbox.com>
astojilj pushed a commit that referenced this pull request Apr 27, 2022
The regression is originally noticed in mobile SDK mapbox/mapbox-maps-ios#1279, where the same approach is ported from #11578.
It was incorrect to (mis)use exaggeration 0 with semantics of invalid exaggeration value. Using undefined instead.
karimnaaji pushed a commit that referenced this pull request May 2, 2022
The regression is originally noticed in mobile SDK mapbox/mapbox-maps-ios#1279, where the same approach is ported from #11578.
It was incorrect to (mis)use exaggeration 0 with semantics of invalid exaggeration value. Using undefined instead.

Co-authored-by: Aleksandar Stojiljković <aleksandar.stojijkovic@mapbox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terrain exaggeration change on pitch leads to wrong camera movement
2 participants