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

change the cluster properties on the sources option #1998

Merged
merged 1 commit into from
Jan 7, 2023
Merged

change the cluster properties on the sources option #1998

merged 1 commit into from
Jan 7, 2023

Conversation

IhsenBen
Copy link
Contributor

@IhsenBen IhsenBen commented Dec 26, 2022

Launch Checklist

Exemple:

map.addSource('earthquakes', {
                type: 'geojson',
                data:
                    'https://maplibre.org/maplibre-gl-js-docs/assets/earthquakes.geojson',
                cluster: true,
            });
///////////////
         map.getSource('earthquakes').setClusterOptions(cluster:false);
        map.getSource('earthquakes').setClusterOptions(clust: true, clusterRadius: 80, clusterMaxZoom: 50 });
/////////////

@HarelM
Copy link
Collaborator

HarelM commented Dec 27, 2022

Thanks for taking the time to contribute!!
Unfortunately, I don't think this code works.
Geojson source uses supercluster and a worker to split the data into tiles.
I haven't seen any changes in that code, which I would expect.
Did you test it?
Please also add the relevant tests fort this use case.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 27, 2022

Bundle size report:

Size Change: +68 B
Total Size Before: 208 kB
Total Size After: 208 kB

Output file Before After Change
maplibre-gl.js 198 kB 199 kB +68 B
maplibre-gl.css 9.1 kB 9.1 kB 0 B
ℹ️ View Details
Source file Before After Change
src/source/geojson_source.ts 1.32 kB 1.37 kB +51 B
src/render/draw_terrain.ts 1.74 kB 1.75 kB +10 B
src/render/draw_background.ts 575 B 582 B +7 B
src/render/program/fill_program.ts 567 B 573 B +6 B
src/render/program/hillshade_program.ts 883 B 889 B +6 B
src/data/pos_attributes.ts 80 B 85 B +5 B
src/render/draw_collision_debug.ts 1.13 kB 1.14 kB +5 B
src/source/image_source.ts 1.14 kB 1.14 kB +4 B
src/style/style.ts 7.38 kB 7.38 kB +4 B
src/render/program/heatmap_program.ts 554 B 558 B +4 B
src/gl/index_buffer.ts 347 B 351 B +4 B
src/render/draw_circle.ts 615 B 619 B +4 B
src/ui/handler/tap_zoom.ts 365 B 369 B +4 B
src/shaders/background.vertex.glsl.g.ts 103 B 106 B +3 B
src/render/program/circle_program.ts 453 B 456 B +3 B
src/ui/handler/one_finger_touch_drag.ts 412 B 415 B +3 B
src/ui/control/scale_control.ts 733 B 736 B +3 B
src/util/worker_pool.ts 416 B 418 B +2 B
src/shaders/background_pattern.vertex.glsl.g.ts 224 B 226 B +2 B
src/shaders/clipping_mask.fragment.glsl.g.ts 59 B 61 B +2 B
src/shaders/heatmap.vertex.glsl.g.ts 366 B 368 B +2 B
src/shaders/heatmap_texture.vertex.glsl.g.ts 143 B 145 B +2 B
src/shaders/fill_outline.vertex.glsl.g.ts 216 B 218 B +2 B
src/shaders/fill_pattern.fragment.glsl.g.ts 394 B 396 B +2 B
src/shaders/fill_extrusion.fragment.glsl.g.ts 117 B 119 B +2 B
src/shaders/hillshade_prepare.vertex.glsl.g.ts 190 B 192 B +2 B
src/shaders/line_pattern.vertex.glsl.g.ts 787 B 789 B +2 B
src/shaders/line_sdf.vertex.glsl.g.ts 807 B 809 B +2 B
src/shaders/symbol_icon.fragment.glsl.g.ts 226 B 228 B +2 B
src/render/draw_fill_extrusion.ts 794 B 796 B +2 B
node_modules/gl-matrix/esm/mat2.js 203 B 205 B +2 B
src/render/image_manager.ts 1.53 kB 1.53 kB +1 B
src/style/load_glyph_range.ts 221 B 222 B +1 B
src/render/line_atlas.ts 983 B 984 B +1 B
src/util/offscreen_canvas_supported.ts 152 B 153 B +1 B
src/source/tile_cache.ts 554 B 555 B +1 B
src/source/pixels_to_tile_units.ts 103 B 104 B +1 B
src/style-spec/empty.ts 155 B 156 B +1 B
src/shaders/debug.fragment.glsl.g.ts 147 B 148 B +1 B
src/shaders/fill.fragment.glsl.g.ts 177 B 178 B +1 B
src/shaders/hillshade.vertex.glsl.g.ts 138 B 139 B +1 B
src/shaders/line.fragment.glsl.g.ts 324 B 325 B +1 B
src/render/program/clipping_mask_program.ts 103 B 104 B +1 B
src/render/program/symbol_program.ts 1.29 kB 1.29 kB +1 B
src/gl/framebuffer.ts 291 B 292 B +1 B
src/gl/color_mode.ts 172 B 173 B +1 B
src/gl/stencil_mode.ts 151 B 152 B +1 B
src/ui/hash.ts 938 B 939 B +1 B
src/ui/handler/handler_util.ts 178 B 179 B +1 B
src/ui/handler/scroll_zoom.ts 1.28 kB 1.28 kB +1 B
src/util/debug.ts 160 B 161 B +1 B
src/util/smart_wrap.ts 234 B 235 B +1 B
src/ui/control/geolocate_control.ts 2.24 kB 2.24 kB +1 B
src/style/light.ts 560 B 559 B -1 B
src/source/source.ts 354 B 353 B -1 B
src/symbol/collision_index.ts 1.64 kB 1.64 kB -1 B
src/shaders/background.fragment.glsl.g.ts 138 B 137 B -1 B
src/shaders/background_pattern.fragment.glsl.g.ts 296 B 295 B -1 B
src/shaders/collision_box.fragment.glsl.g.ts 148 B 147 B -1 B
src/shaders/collision_circle.vertex.glsl.g.ts 546 B 545 B -1 B
src/shaders/fill_outline.fragment.glsl.g.ts 254 B 253 B -1 B
src/shaders/fill_extrusion.vertex.glsl.g.ts 623 B 622 B -1 B
src/shaders/hillshade_prepare.fragment.glsl.g.ts 501 B 500 B -1 B
src/shaders/line_sdf.fragment.glsl.g.ts 460 B 459 B -1 B
src/shaders/symbol_icon.vertex.glsl.g.ts 949 B 948 B -1 B
src/shaders/symbol_text_and_icon.fragment.glsl.g.ts 598 B 597 B -1 B
src/shaders/terrain_coords.fragment.glsl.g.ts 169 B 168 B -1 B
src/render/program/background_program.ts 472 B 471 B -1 B
src/gl/value.ts 1.09 kB 1.09 kB -1 B
src/gl/cull_face_mode.ts 153 B 152 B -1 B
src/render/draw_heatmap.ts 1.04 kB 1.04 kB -1 B
src/render/draw_custom.ts 342 B 341 B -1 B
src/geo/edge_insets.ts 430 B 429 B -1 B
src/util/throttle.ts 144 B 143 B -1 B
src/ui/handler/box_zoom.ts 706 B 705 B -1 B
src/ui/handler/mouse.ts 524 B 523 B -1 B
src/ui/handler/two_fingers_touch.ts 1.02 kB 1.02 kB -1 B
src/ui/handler/shim/dblclick_zoom.ts 150 B 149 B -1 B
src/ui/default_locale.ts 311 B 310 B -1 B
src/gl/render_pool.ts 584 B 583 B -1 B
src/ui/control/navigation_control.ts 1.81 kB 1.81 kB -1 B
src/ui/marker.ts 2.87 kB 2.87 kB -1 B
src/source/query_features.ts 1.22 kB 1.21 kB -2 B
src/util/global_worker_pool.ts 327 B 325 B -2 B
src/shaders/heatmap_texture.fragment.glsl.g.ts 207 B 205 B -2 B
src/shaders/debug.vertex.glsl.g.ts 163 B 161 B -2 B
src/shaders/fill_pattern.vertex.glsl.g.ts 389 B 387 B -2 B
src/shaders/hillshade.fragment.glsl.g.ts 555 B 553 B -2 B
src/shaders/line.vertex.glsl.g.ts 709 B 707 B -2 B
src/shaders/line_pattern.fragment.glsl.g.ts 707 B 705 B -2 B
src/render/program.ts 1.16 kB 1.15 kB -2 B
src/render/program/collision_program.ts 725 B 723 B -2 B
src/gl/context.ts 1.28 kB 1.28 kB -2 B
src/render/painter.ts 3.76 kB 3.76 kB -2 B
src/ui/camera.ts 3.41 kB 3.41 kB -2 B
src/shaders/clipping_mask.vertex.glsl.g.ts 106 B 103 B -3 B
src/render/program/debug_program.ts 195 B 192 B -3 B
src/data/raster_bounds_attributes.ts 97 B 93 B -4 B
src/shaders/terrain.vertex.glsl.g.ts 222 B 218 B -4 B
src/shaders/shaders.ts 1.49 kB 1.49 kB -4 B
src/render/program/raster_program.ts 564 B 560 B -4 B
src/render/draw_hillshade.ts 1.16 kB 1.15 kB -4 B
src/render/draw_symbol.ts 2.61 kB 2.6 kB -5 B
src/render/draw_raster.ts 1.05 kB 1.05 kB -5 B
src/ui/handler/drag_handler.ts 499 B 494 B -5 B
src/index.ts 866 B 861 B -5 B
src/render/draw_debug.ts 1.36 kB 1.36 kB -7 B
src/render/program/terrain_program.ts 704 B 690 B -14 B
src/render/program/program_uniforms.ts 955 B 928 B -27 B

@IhsenBen
Copy link
Contributor Author

Thanks for taking the time to contribute!!

Unfortunately, I don't think this code works.

Geojson source uses supercluster and a worker to split the data into tiles.

I haven't seen any changes in that code, which I would expect.

Did you test it?

Please also add the relevant tests fort this use case.

Hey, Sorry to hear that, I’m only changing the property ( not removing the clusters ), I tested it with the CSS/Js build and it was working.
Besides testing, Can you please tell me more about what you would expect from this change?
Thx in advance

@HarelM
Copy link
Collaborator

HarelM commented Dec 27, 2022

Can you share the code that you used for testing this?
Since the addition to the code only changes the _options and since changing these don't call the _updateWorkerData and doesn't change this.workerOptions I don't see how this would work. I might be wrong though :-)
The only thing I can think of, is if you set this before the source is added to the map and so this option is used only on startup once.
Can you share a working example, or maybe a movie where you can toggle on and off the cluster?

@IhsenBen
Copy link
Contributor Author

IhsenBen commented Dec 27, 2022

Oh I see, that does make sense, because I had this use case while I was reloading it with React, I tested the build with a simple HTML code with buttons like this (so as you said this only runs once):

<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <script src="maplibre-gl.js"></script>
    <link rel="stylesheet" href="maplibre-gl.css">
    <title>Document</title>
</head>

<body>
    <style>
        body {
            margin: 0;
            padding: 0;
        }

        .container {
            position: relative;
            width: 100%;
            height: 100vh;
            display: flex;
            flex-direction: column;
            justify-content: center;
        }

        #map {
            position: absolute;
            top: 0;
            bottom: 0;
            width: 100%;
        }
        .buttons{
            position: absolute;
            top: 0;
            left: 0;
            z-index: 1;
        }
    </style>
    <h1>Webpack</h1>
    <div class="container">
        <div class="buttons">
            <button id="on">Turn On</button>
            <button id="off">Turn Off</button>
        </div>
        <div class="mapcontainer">
            <div id="map"></div>
        </div>

    </div>
    <script>

        var map = new maplibregl.Map({
            container: 'map',
            style:
                'https://api.maptiler.com/maps/streets/style.json?key=get_your_own_OpIi9ZULNHzrESv6T2vL',
            center: [-103.59179687498357, 40.66995747013945],
            zoom: 3
        });

        map.on('load', function () {
            // Add a new source from our GeoJSON data and
            // set the 'cluster' option to true. GL-JS will
            // add the point_count property to your source data.
            map.addSource('earthquakes', {
                type: 'geojson',
                // Point to GeoJSON data. This example visualizes all M1.0+ earthquakes
                // from 12/22/15 to 1/21/16 as logged by USGS' Earthquake hazards program.
                data:
                    'https://maplibre.org/maplibre-gl-js-docs/assets/earthquakes.geojson',
                cluster: true,
                clusterMaxZoom: 50, // Max zoom to cluster points on
                clusterRadius: 50 // Radius of each cluster when clustering points (defaults to 50)
            });

            map.addLayer({
                id: 'clusters',
                type: 'circle',
                source: 'earthquakes',
                filter: ['has', 'point_count'],
                paint: {
                    // Use step expressions (https://maplibre.org/maplibre-gl-js-docs/style-spec/#expressions-step)
                    // with three steps to implement three types of circles:
                    //   * Blue, 20px circles when point count is less than 100
                    //   * Yellow, 30px circles when point count is between 100 and 750
                    //   * Pink, 40px circles when point count is greater than or equal to 750
                    'circle-color': [
                        'step',
                        ['get', 'point_count'],
                        '#51bbd6',
                        100,
                        '#f1f075',
                        750,
                        '#f28cb1'
                    ],
                    'circle-radius': [
                        'step',
                        ['get', 'point_count'],
                        20,
                        100,
                        30,
                        750,
                        40
                    ]
                }
            });

            map.addLayer({
                id: 'cluster-count',
                type: 'symbol',
                source: 'earthquakes',
                filter: ['has', 'point_count'],
                layout: {
                    'text-field': '{point_count_abbreviated}',
                    'text-font': ['DIN Offc Pro Medium', 'Arial Unicode MS Bold'],
                    'text-size': 12
                }
            });

            map.addLayer({
                id: 'unclustered-point',
                type: 'circle',
                source: 'earthquakes',
                filter: ['!', ['has', 'point_count']],
                paint: {
                    'circle-color': '#11b4da',
                    'circle-radius': 4,
                    'circle-stroke-width': 1,
                    'circle-stroke-color': '#fff'
                }
            });

            // inspect a cluster on click
            map.on('click', 'clusters', function (e) {
                var features = map.queryRenderedFeatures(e.point, {
                    layers: ['clusters']
                });
                var clusterId = features[0].properties.cluster_id;
                map.getSource('earthquakes').getClusterExpansionZoom(
                    clusterId,
                    function (err, zoom) {
                        if (err) return;

                        map.easeTo({
                            center: features[0].geometry.coordinates,
                            zoom: zoom
                        });
                    }
                );
            });

            // When a click event occurs on a feature in
            // the unclustered-point layer, open a popup at
            // the location of the feature, with
            // description HTML from its properties.
            map.on('click', 'unclustered-point', function (e) {
                var coordinates = e.features[0].geometry.coordinates.slice();
                var mag = e.features[0].properties.mag;
                var tsunami;

                if (e.features[0].properties.tsunami === 1) {
                    tsunami = 'yes';
                } else {
                    tsunami = 'no';
                }

                // Ensure that if the map is zoomed out such that
                // multiple copies of the feature are visible, the
                // popup appears over the copy being pointed to.
                while (Math.abs(e.lngLat.lng - coordinates[0]) > 180) {
                    coordinates[0] += e.lngLat.lng > coordinates[0] ? 360 : -360;
                }

                new maplibregl.Popup()
                    .setLngLat(coordinates)
                    .setHTML(
                        'magnitude: ' + mag + '<br>Was there a tsunami?: ' + tsunami
                    )
                    .addTo(map);

            });

   //--------------------Test ---------------------
            const turnOnBtn = document.getElementById('on');
            const turnOffBtn = document.getElementById('off');

            turnOnBtn.addEventListener('click', () => {
                map.setLayoutProperty('clusters', 'visibility', 'visible');
                map.getSource('earthquakes').setCluster(true);
                console.log(map.getSource('earthquakes')._options.cluster)
            });

            turnOffBtn.addEventListener('click', () => {
                map.setLayoutProperty('clusters', 'visibility', 'none');
                map.getSource('earthquakes').setCluster(false);
                console.log(map.getSource('earthquakes')._options.cluster)
            });
  //--------------------Test ---------------------

        });


    </script>

</body>

</html>

I'll try to update my pull request in the upcoming days

@IhsenBen IhsenBen changed the title change the cluster property on the sources option change the cluster properties on the sources option Jan 1, 2023
@IhsenBen
Copy link
Contributor Author

IhsenBen commented Jan 1, 2023

I updated the pull request and added more options, I'm open to your suggestions or requirements 😃

@HarelM
Copy link
Collaborator

HarelM commented Jan 1, 2023

Thanks for the update, please see if you can add tests to this scenario.
I would also recommend to reduce the changes in the changelog, did you format it?

@IhsenBen
Copy link
Contributor Author

IhsenBen commented Jan 2, 2023

Hi,
Sorry about the auto formatting I had it "on save", So :

  • I added a unit test .
  • cleaned my log on changelog .
  • and squashed my commits.
    I hope it's fine now.
    Also please don't hesitate to ask if you want more changes.
    thx in advance

@HarelM
Copy link
Collaborator

HarelM commented Jan 3, 2023

This looks great!
Can you please add types to _options and workerOptions?
It's unclear what can be set there, what is passed to the worker, what is kept in the source etc.
I'm guessing some types can be fetched from supercluster library, others might be needed to be added manually.
Fixing this will allow us to see if there are places where the current change is missing (i.e. more places where we need to change the values of the saved members).
Otherwise, this looks good.

@IhsenBen
Copy link
Contributor Author

IhsenBen commented Jan 4, 2023

This looks great! Can you please add types to _options and workerOptions?

I don't understand, I already typed the arguments of setCluster().
Can you please tell me what do you mean exactly?

@HarelM
Copy link
Collaborator

HarelM commented Jan 4, 2023

The class members have a type any. Since you changes the code that uses them, I was hoping you could add types to them to improve readability and future maintenance.

@IhsenBen
Copy link
Contributor Author

IhsenBen commented Jan 5, 2023

The class members have a type any. Since you changes the code that uses them, I was hoping you could add types to them to improve readability and future maintenance.

I rebased my branch and added the type for _options with optional properties for flexibility, also tbh I didn't want to bother with workerOptions because this has a lot values and I didn't want to mess anything up.

Hope this will do it :D
thx for your time and consideration

src/source/geojson_source.ts Outdated Show resolved Hide resolved
src/source/geojson_source.ts Outdated Show resolved Hide resolved
src/source/geojson_source.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Jan 5, 2023

Would be nice to type the worker option as best as you could at this point in time, we can always improve this in the future. Anything is better than any.
If this is too much to ask then please open an issue on it so someone else could pick this up.
Boy Scout role etc... 😀

@IhsenBen
Copy link
Contributor Author

IhsenBen commented Jan 6, 2023

Would be nice to type the worker option as best as you could at this point in time, we can always improve this in the future. Anything is better than any. If this is too much to ask then please open an issue on it so someone else could pick this up. Boy Scout role etc... 😀

Hey
I updated my work and added the extra types. ( I can move the types to a different file if you suggest one)
made them flexible with optional properties, and so far seems to be passing all the tests.
hopefully, this will do it.
looking forward for your feedback

src/source/geojson_source.ts Outdated Show resolved Hide resolved
- Adding unit test for setClusterOptions
- Adding Types for _options
- Adding Types for workerOptions
- Log these changes in CHANGELOG.md
@HarelM HarelM merged commit 541d530 into maplibre:main Jan 7, 2023
@IhsenBen
Copy link
Contributor Author

IhsenBen commented Jan 7, 2023

Thanks Harel, this was my first valuable open-source contribution.
good team work

@HarelM
Copy link
Collaborator

HarelM commented Jan 7, 2023

First of many hopefully 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants