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

mapbox-gl does not render all GeoJSON layers #5970

Closed
vicapow opened this Issue Jan 9, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@vicapow
Contributor

vicapow commented Jan 9, 2018

This is only on master and from what I can tell is not an issue with the latest published version, 0.43.0.

Here's the full example after modifying setstyle.html.

Expected: A red circle layer to be rendered as in https://jsbin.com/kurufojuvi/edit?html,output.

screen shot 2018-01-08 at 8 06 44 pm

Actual: No red layer is ever rendered.

screen shot 2018-01-08 at 8 07 35 pm

I was able to track this down to this specific diff: from @ChrisLoer 19 days ago. Can we revert?

<!DOCTYPE html>
<html>
<head>
    <title>Mapbox GL JS debug page</title>
    <meta charset='utf-8'>
    <meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
    <link rel='stylesheet' href='/dist/mapbox-gl.css' />
    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
    </style>
</head>

<body>
<div id='map'></div>
<script src='/dist/mapbox-gl-dev.js'></script>
<script src='/debug/access_token_generated.js'></script>
<script>

var data = {
    type: 'FeatureCollection',
    features: [
        {
            type: 'Feature',
            properties: {},
            geometry: { type: 'Point', coordinates: [ -122.41848707199095, 37.77554439836295 ] }
        }
    ]
};
var style = {
    version: 8,
    name: 'example',
    minzoom: 0,
    maxzoom: 22,
    sources: {
        source1: { type: "geojson", data: data },
        source2: { type: "geojson", data: data },
        source3: { type: "geojson", data: data },
        source4: { type: "geojson", data: data },
        source5: { type: "geojson", data: data }
    },
    layers: [
        {
            id: 'source1-layer',
            source: 'source1',
            type: 'circle',
            paint: { 'circle-radius': 50, 'circle-color': '#ccc' }
        }, {
            id: 'source2-layer',
            source: 'source2',
            type: 'circle',
            paint: { 'circle-radius': 40, 'circle-color': '#999' }
        }, {
            id: 'source3-layer',
            source: 'source3',
            type: 'circle',
            paint: { 'circle-radius': 30, 'circle-color': 'red' }
        }, {
            id: 'source4-layer',
            source: 'source4',
            type: 'circle',
            paint: { 'circle-radius': 20, 'circle-color': '#333' }
        }, {
            id: 'source5-layer',
            source: 'source5',
            type: 'circle',
            paint: { 'circle-radius': 10, 'circle-color': '#000' }
        }
    ]
};

var map = window.map = new mapboxgl.Map({
    container: 'map',
    zoom: 22,
    center: [-122.41848286, 37.77552867],
    style: style,
    hash: style
});

// map.loaded() never returns `true`.
// The 3rd layer (red point) never renders.
setInterval(() => { console.log(map.loaded()); }, 100);

</script>
</body>
</html>
@ChrisLoer

This comment has been minimized.

Show comment
Hide comment
@ChrisLoer

ChrisLoer Jan 10, 2018

Contributor

Thanks for the super-clear report, @vicapow! I can reproduce the problem in Safari but not Chrome or Firefox -- which browser(s) are you using? I'll investigate and come up with a fix (hopefully this won't require a revert).

Contributor

ChrisLoer commented Jan 10, 2018

Thanks for the super-clear report, @vicapow! I can reproduce the problem in Safari but not Chrome or Firefox -- which browser(s) are you using? I'll investigate and come up with a fix (hopefully this won't require a revert).

@vicapow

This comment has been minimized.

Show comment
Hide comment
@vicapow

vicapow Jan 10, 2018

Contributor

@ChrisLoer Glad to help! This is my setup

Chrome Version 63.0.3239.132 (Official Build) (64-bit)
OS X 10.12.6 (16G1036)
MacBook Pro (13-inch, 2017, Four Thunderbolt 3 Ports)
3.5 GHz Intel Core i7
16 GB 2133 MHz LPDDR3
Intel Iris Plus Graphics 650 1536 MB

Contributor

vicapow commented Jan 10, 2018

@ChrisLoer Glad to help! This is my setup

Chrome Version 63.0.3239.132 (Official Build) (64-bit)
OS X 10.12.6 (16G1036)
MacBook Pro (13-inch, 2017, Four Thunderbolt 3 Ports)
3.5 GHz Intel Core i7
16 GB 2133 MHz LPDDR3
Intel Iris Plus Graphics 650 1536 MB

@vicapow

This comment has been minimized.

Show comment
Hide comment
@vicapow

vicapow Jan 10, 2018

Contributor

I've also just tested this on Safari and FireFox and am able to reproduce the issue in those browsers, as well. Based on your changes this doesn't seem like a browser specific issue or GPU related. Might be worth double checking your cache was cleared in Chrome when you tried it.

screen shot 2018-01-10 at 10 50 11 am

Contributor

vicapow commented Jan 10, 2018

I've also just tested this on Safari and FireFox and am able to reproduce the issue in those browsers, as well. Based on your changes this doesn't seem like a browser specific issue or GPU related. Might be worth double checking your cache was cleared in Chrome when you tried it.

screen shot 2018-01-10 at 10 50 11 am

@ChrisLoer

This comment has been minimized.

Show comment
Hide comment
@ChrisLoer

ChrisLoer Jan 10, 2018

Contributor

The problem is that when one GeoJSON worker is handling multiple sources, it can mix up loadData/coalesce messages between the two sources. Whether or not it messes up depends on timing, which probably accounts for the differences between what you saw and what I saw.

The fix is pretty easy, I'll submit a PR later today.

Contributor

ChrisLoer commented Jan 10, 2018

The problem is that when one GeoJSON worker is handling multiple sources, it can mix up loadData/coalesce messages between the two sources. Whether or not it messes up depends on timing, which probably accounts for the differences between what you saw and what I saw.

The fix is pretty easy, I'll submit a PR later today.

ChrisLoer added a commit that referenced this issue Jan 10, 2018

Don't mix coalesce calls from multiple GeoJSON sources (issue #5970)
Also: send back an "abandoned" callback for coalesced calls so that foreground doesn't leak callback handles.
Add 'loadData' unit test for geojson_worker_source that simulates building up a queue of loadData messages.

@jfirebaugh jfirebaugh added this to the 0.44.0 milestone Jan 12, 2018

@ChrisLoer

This comment has been minimized.

Show comment
Hide comment
@ChrisLoer

ChrisLoer Jan 12, 2018

Contributor

Fixed temporarily with revert of fa95c19 via #5971.

We plan to restore the coalesce functionality with #5983.

Contributor

ChrisLoer commented Jan 12, 2018

Fixed temporarily with revert of fa95c19 via #5971.

We plan to restore the coalesce functionality with #5983.

@ChrisLoer ChrisLoer closed this Jan 12, 2018

ChrisLoer added a commit that referenced this issue Jan 18, 2018

Don't mix coalesce calls from multiple GeoJSON sources (issue #5970)
Also: send back an "abandoned" callback for coalesced calls so that foreground doesn't leak callback handles.
Add 'loadData' unit test for geojson_worker_source that simulates building up a queue of loadData messages.

ChrisLoer added a commit that referenced this issue Jan 19, 2018

Coalesce GeoJSON "loadData" requests.
Re-implement PR #5902 including fix for issue #5970.

ChrisLoer added a commit that referenced this issue Jan 25, 2018

Coalesce GeoJSON "loadData" requests.
Re-implement PR #5902 including fix for issue #5970.

ChrisLoer added a commit that referenced this issue Jan 25, 2018

Coalesce GeoJSON "loadData" requests.
Re-implement PR #5902 including fix for issue #5970.

ChrisLoer added a commit that referenced this issue Jan 29, 2018

Coalesce GeoJSON "loadData" requests.
Re-implement PR #5902 including fix for issue #5970.

pathmapper pushed a commit to pathmapper/mapbox-gl-js that referenced this issue Feb 13, 2018

Coalesce GeoJSON "loadData" requests.
Re-implement PR mapbox#5902 including fix for issue mapbox#5970.

pathmapper pushed a commit to pathmapper/mapbox-gl-js that referenced this issue Feb 13, 2018

Coalesce GeoJSON "loadData" requests.
Re-implement PR mapbox#5902 including fix for issue mapbox#5970.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment