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

respect padding in cameraForBounds when using globe projection #13126

Merged
merged 1 commit into from Apr 23, 2024
Merged

respect padding in cameraForBounds when using globe projection #13126

merged 1 commit into from Apr 23, 2024

Conversation

jonasnoki
Copy link
Contributor

@jonasnoki jonasnoki commented Mar 23, 2024 •
edited

Closes #12498

As described in the issue #12498 the fitBounds and subsequently the cameraForBounds methods do not respect the padding when globe projection is used.

In the code _cameraForBoundsOnGlobe just ignored the padding altogether. I extracted the part of the code that adds the padding to the aabbs in camera space and used it in globe and mercator case. I also added a test to the camera.test.js file.

With Padding

Previous
Screenshot 2024-03-23 at 21 42 23
With the changes
Screenshot 2024-03-23 at 21 41 35

Without padding

Previous:
Screenshot 2024-03-23 at 21 40 56
With the changes:
Screenshot 2024-03-23 at 21 41 24
(It's not the same picture I swear! :D)

Visual Testing

In order to verify the functionality visually I created a test scenario html file, there you can move around the markers and see that all possible cases produce a functional camera transform:

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <title>Padding and fitBounds</title>
    <style>
        body {
            margin: 0;
            padding: 0;
        }

        #map {
            position: absolute;
            top: 0;
            bottom: 0;
            width: 100%;
        }

        #sidebar {
            position: absolute;
            top: 0;
            left: 0;
            width: 200px;
            height: 100%;
            background-color: rgba(255, 255, 255, 0.7);
            padding: 12px
        }
    </style>
    <script src='mapbox-gl-dev.js'></script>
    <link href='https://api.mapbox.com/mapbox-gl-js/v3.2.0/mapbox-gl.css' rel='stylesheet' />

</head>
<body>
<div id="map"></div>
<div id="sidebar">
    <h2>
        This sidebar should be respected by the padding.
    </h2>


    <p>
        Move around the markers to examine which configs are working.
    </p>

    <button onclick="zoomToBounds()">Zoom to bounds</button>
    <br>
    <button onclick="removePadding()">Remove padding</button>
    <button onclick="addPadding()">Add padding</button>

</div>
<script type="text/javascript">
    mapboxgl.accessToken = 'pk.eyJ1Ijoiam9uYXNub2tpIiwiYSI6ImNrbWdqdDNscTB5dWgybnBqeG05MTZyaWQifQ.nntxZCDkDa9k65FOnI24RA';

    const map = new mapboxgl.Map({
        projection: {name: 'globe'},
        container: 'map', // container ID
    });

    const latLanSeville = [-5.9844, 37.3894];
    const latLanIstanbul = [28.9784, 41.0082];

    const markerSeville = new mapboxgl.Marker({draggable: true})
        .setLngLat(latLanSeville)
        .addTo(map);

    const markerIstanbul = new mapboxgl.Marker({draggable: true})
        .setLngLat(latLanIstanbul)
        .addTo(map);

    markerSeville.on('dragend', onDragEnd);
    markerIstanbul.on('dragend', onDragEnd);

    function onDragEnd() {
        zoomToBounds();
    }


    const padding = {
        left: 300,
        bottom: 100,
        right: 100,
        top: 100
    }

    map.setPadding(padding)
    map.showPadding = true;

    function removePadding() {
        map.setPadding({top: 0, bottom: 0, left: 0, right: 0});
    }

    function addPadding() {
        map.setPadding(padding);
    }

    function zoomToBounds() {

        const bounds = new mapboxgl.LngLatBounds();
        bounds.setNorthEast(markerIstanbul.getLngLat());
        bounds.setSouthWest(markerSeville.getLngLat());
        map.fitBounds(bounds);
    }

    zoomToBounds();

</script>
</body>
</html>

Launch Checklist

  • Make sure the PR title is descriptive and preferably reflects the change from the user's perspective.
  • Add additional detail and context in the PR description (with screenshots/videos if there are visual changes).
  • Manually test the debug page.
  • Write tests for all new functionality and make sure the CI checks pass.
  • Document any changes to public APIs.
  • Post benchmark scores if the change could affect performance.
  • Tag @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes.
  • Tag @mapbox/gl-native if this PR includes shader changes or needs a native port.

@jonasnoki jonasnoki requested a review from a team as a code owner March 23, 2024 20:52
@jonasnoki jonasnoki force-pushed the fitBounds-padding-with-globe-projection-12498 branch 3 times, most recently from fe089f0 to 762b084 Compare March 25, 2024 11:09
@jonasnoki jonasnoki force-pushed the fitBounds-padding-with-globe-projection-12498 branch from 762b084 to e7a1dcf Compare April 21, 2024 10:07
@jonasnoki jonasnoki mentioned this pull request Apr 22, 2024
Closed
stepankuzmin
stepankuzmin approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@stepankuzmin stepankuzmin left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the contribution, @jonasnoki! Overall, it looks good to me 👍

@stepankuzmin stepankuzmin merged commit 360bfd2 into mapbox:main Apr 23, 2024
23 checks passed
@underoot underoot mentioned this pull request May 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stepankuzmin stepankuzmin stepankuzmin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

map.fitBounds does not account for padding options when used with globe projection
2 participants
@jonasnoki @stepankuzmin