From 29f23f5923b8a0c2e341ea54bee47f6327eb3689 Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Wed, 1 Aug 2018 11:27:56 +0300 Subject: [PATCH 1/3] minor map.js cleanup --- src/ui/map.js | 112 +++++++++++++++++--------------------------------- 1 file changed, 38 insertions(+), 74 deletions(-) diff --git a/src/ui/map.js b/src/ui/map.js index d07534932de..4695bf0a8c6 100755 --- a/src/ui/map.js +++ b/src/ui/map.js @@ -106,7 +106,6 @@ const defaultOptions = { maxZoom: defaultMaxZoom, interactive: true, - scrollZoom: true, boxZoom: true, dragRotate: true, @@ -116,24 +115,17 @@ const defaultOptions = { touchZoomRotate: true, bearingSnap: 7, - clickTolerance: 3, hash: false, - attributionControl: true, failIfMajorPerformanceCaveat: false, preserveDrawingBuffer: false, - trackResize: true, - renderWorldCopies: true, - refreshExpiredTiles: true, - maxTileCacheSize: null, - transformRequest: null, fadeDuration: 300, crossSourceCollisions: true @@ -331,14 +323,14 @@ class Map extends Camera { this._controls = []; const transformRequestFn = options.transformRequest; - this._transformRequest = transformRequestFn ? (url, type) => transformRequestFn(url, type) || ({ url }) : (url) => ({ url }); + this._transformRequest = transformRequestFn ? + (url, type) => transformRequestFn(url, type) || ({ url }) : + (url) => ({ url }); if (typeof options.container === 'string') { - const container = window.document.getElementById(options.container); - if (!container) { + this._container = window.document.getElementById(options.container); + if (!this._container) { throw new Error(`Container '${options.container}' not found.`); - } else { - this._container = container; } } else if (options.container instanceof HTMLElement) { this._container = options.container; @@ -354,11 +346,7 @@ class Map extends Camera { '_onWindowOnline', '_onWindowResize', '_contextLost', - '_contextRestored', - '_update', - '_render', - '_onData', - '_onDataLoading' + '_contextRestored' ], this); this._setupContainer(); @@ -367,8 +355,8 @@ class Map extends Camera { throw new Error(`Failed to initialize WebGL.`); } - this.on('move', this._update.bind(this, false)); - this.on('zoom', this._update.bind(this, true)); + this.on('move', () => this._update(false)); + this.on('zoom', () => this._update(true)); if (typeof window !== 'undefined') { window.addEventListener('online', this._onWindowOnline, false); @@ -392,17 +380,23 @@ class Map extends Camera { if (options.style) this.setStyle(options.style, { localIdeographFontFamily: options.localIdeographFontFamily }); - if (options.attributionControl) this.addControl(new AttributionControl({ customAttribution: options.customAttribution })); + if (options.attributionControl) + this.addControl(new AttributionControl({ customAttribution: options.customAttribution })); + this.addControl(new LogoControl(), options.logoPosition); - this.on('style.load', function() { + this.on('style.load', () => { if (this.transform.unmodified) { - this.jumpTo(this.style.stylesheet); + this.jumpTo((this.style.stylesheet: any)); } }); - - this.on('data', this._onData); - this.on('dataloading', this._onDataLoading); + this.on('data', (event: MapDataEvent) => { + this._update(event.dataType === 'style'); + this.fire(new Event(`${event.dataType}data`, event)); + }); + this.on('dataloading', (event: MapDataEvent) => { + this.fire(new Event(`${event.dataType}dataloading`, event)); + }); } /** @@ -477,7 +471,6 @@ class Map extends Camera { .fire(new Event('move', eventData)) .fire(new Event('resize', eventData)) .fire(new Event('moveend', eventData)); - return this; } @@ -531,13 +524,11 @@ class Map extends Camera { this.transform.lngRange = [b.getWest(), b.getEast()]; this.transform.latRange = [b.getSouth(), b.getNorth()]; this.transform._constrain(); - this._update(); } else if (lnglatbounds === null || lnglatbounds === undefined) { this.transform.lngRange = null; this.transform.latRange = null; - this._update(); } - return this; + return this._update(); } @@ -610,11 +601,8 @@ class Map extends Camera { * @returns {Map} `this` */ setRenderWorldCopies(renderWorldCopies?: ?boolean) { - this.transform.renderWorldCopies = renderWorldCopies; - this._update(); - - return this; + return this._update(); } /** @@ -947,9 +935,7 @@ class Map extends Camera { return { viewport: queryGeometry, - worldCoordinate: queryGeometry.map((p) => { - return this.transform.pointCoordinate(p); - }) + worldCoordinate: queryGeometry.map((p) => this.transform.pointCoordinate(p)) }; } @@ -1079,8 +1065,7 @@ class Map extends Camera { */ addSource(id: string, source: SourceSpecification) { this.style.addSource(id, source); - this._update(true); - return this; + return this._update(true); } /** @@ -1138,8 +1123,7 @@ class Map extends Camera { */ removeSource(id: string) { this.style.removeSource(id); - this._update(true); - return this; + return this._update(true); } /** @@ -1249,8 +1233,7 @@ class Map extends Camera { */ addLayer(layer: LayerSpecification, before?: string) { this.style.addLayer(layer, before); - this._update(true); - return this; + return this._update(true); } /** @@ -1263,8 +1246,7 @@ class Map extends Camera { */ moveLayer(id: string, beforeId?: string) { this.style.moveLayer(id, beforeId); - this._update(true); - return this; + return this._update(true); } /** @@ -1277,8 +1259,7 @@ class Map extends Camera { */ removeLayer(id: string) { this.style.removeLayer(id); - this._update(true); - return this; + return this._update(true); } /** @@ -1309,8 +1290,7 @@ class Map extends Camera { */ setFilter(layer: string, filter: ?FilterSpecification) { this.style.setFilter(layer, filter); - this._update(true); - return this; + return this._update(true); } /** @@ -1325,8 +1305,7 @@ class Map extends Camera { */ setLayerZoomRange(layerId: string, minzoom: number, maxzoom: number) { this.style.setLayerZoomRange(layerId, minzoom, maxzoom); - this._update(true); - return this; + return this._update(true); } /** @@ -1355,8 +1334,7 @@ class Map extends Camera { */ setPaintProperty(layer: string, name: string, value: any) { this.style.setPaintProperty(layer, name, value); - this._update(true); - return this; + return this._update(true); } /** @@ -1382,8 +1360,7 @@ class Map extends Camera { */ setLayoutProperty(layer: string, name: string, value: any) { this.style.setLayoutProperty(layer, name, value); - this._update(true); - return this; + return this._update(true); } /** @@ -1405,8 +1382,7 @@ class Map extends Camera { */ setLight(light: LightSpecification) { this.style.setLight(light); - this._update(true); - return this; + return this._update(true); } /** @@ -1431,7 +1407,7 @@ class Map extends Camera { */ setFeatureState(feature: { source: string; sourceLayer?: string; id: string; }, state: Object) { this.style.setFeatureState(feature, state); - this._update(); + return this._update(); } /** @@ -1595,11 +1571,7 @@ class Map extends Camera { * @returns {boolean} A Boolean indicating whether the map is fully loaded. */ loaded() { - if (this._styleDirty || this._sourcesDirty) - return false; - if (!this.style || !this.style.loaded()) - return false; - return true; + return !this._styleDirty && !this._sourcesDirty && !!this.style && this.style.loaded(); } /** @@ -1611,12 +1583,13 @@ class Map extends Camera { * @private */ _update(updateStyle?: boolean) { - if (!this.style) return; + if (!this.style) return this; this._styleDirty = this._styleDirty || updateStyle; this._sourcesDirty = true; - this._rerender(); + + return this; } /** @@ -1852,15 +1825,6 @@ class Map extends Camera { // show vertices get vertices(): boolean { return !!this._vertices; } set vertices(value: boolean) { this._vertices = value; this._update(); } - - _onData(event: MapDataEvent) { - this._update(event.dataType === 'style'); - this.fire(new Event(`${event.dataType}data`, event)); - } - - _onDataLoading(event: MapDataEvent) { - this.fire(new Event(`${event.dataType}dataloading`, event)); - } } export default Map; From 6088950a8ba57df1d32500af3fe8e917bd8cfdad Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Wed, 1 Aug 2018 12:27:43 +0300 Subject: [PATCH 2/3] simplify map queryRenderedFeatures --- src/style/style.js | 6 ++-- src/ui/map.js | 57 +++++++---------------------------- test/unit/style/style.test.js | 21 +++++++------ test/unit/ui/map.test.js | 9 ++---- 4 files changed, 28 insertions(+), 65 deletions(-) diff --git a/src/style/style.js b/src/style/style.js index 4ad10f9b9e6..ea7c3b65b5f 100644 --- a/src/style/style.js +++ b/src/style/style.js @@ -899,13 +899,15 @@ class Style extends Evented { } const sourceResults = []; + const queryCoordinates = queryGeometry.map((p) => transform.pointCoordinate(p)); + for (const id in this.sourceCaches) { if (params.layers && !includedSources[id]) continue; sourceResults.push( queryRenderedFeatures( this.sourceCaches[id], this._layers, - queryGeometry.worldCoordinate, + queryCoordinates, params, transform) ); @@ -918,7 +920,7 @@ class Style extends Evented { queryRenderedSymbols( this._layers, this.sourceCaches, - queryGeometry.viewport, + queryGeometry, params, this.placement.collisionIndex, this.placement.retainedQueryData) diff --git a/src/ui/map.js b/src/ui/map.js index 4695bf0a8c6..2fd5a933fed 100755 --- a/src/ui/map.js +++ b/src/ui/map.js @@ -879,64 +879,29 @@ class Map extends Camera { // // There no way to express that in a way that's compatible with both flow and documentation.js. // Related: https://github.com/facebook/flow/issues/1556 - if (arguments.length === 2) { - geometry = arguments[0]; - options = arguments[1]; - } else if (arguments.length === 1 && isPointLike(arguments[0])) { - geometry = arguments[0]; - options = {}; - } else if (arguments.length === 1) { - geometry = undefined; - options = arguments[0]; - } else { - geometry = undefined; - options = {}; - } if (!this.style) { return []; } - return this.style.queryRenderedFeatures( - this._makeQueryGeometry(geometry), - options, - this.transform - ); - - function isPointLike(input) { - return input instanceof Point || Array.isArray(input); + if (options === undefined && geometry !== undefined && !(geometry instanceof Point) && !Array.isArray(geometry)) { + options = (geometry: Object); + geometry = undefined; } - } - _makeQueryGeometry(pointOrBox?: PointLike | [PointLike, PointLike]) { - if (pointOrBox === undefined) { - // bounds was omitted: use full viewport - pointOrBox = [ - Point.convert([0, 0]), - Point.convert([this.transform.width, this.transform.height]) - ]; - } + options = options || {}; + geometry = geometry || [[0, 0], [this.transform.width, this.transform.height]]; let queryGeometry; - - if (pointOrBox instanceof Point || typeof pointOrBox[0] === 'number') { - const point = Point.convert(pointOrBox); - queryGeometry = [point]; + if (geometry instanceof Point || typeof geometry[0] === 'number') { + queryGeometry = [Point.convert(geometry)]; } else { - const box = [Point.convert(pointOrBox[0]), Point.convert(pointOrBox[1])]; - queryGeometry = [ - box[0], - new Point(box[1].x, box[0].y), - box[1], - new Point(box[0].x, box[1].y), - box[0] - ]; + const tl = Point.convert(geometry[0]); + const br = Point.convert(geometry[1]); + queryGeometry = [tl, new Point(br.x, tl.y), br, new Point(tl.x, br.y), tl]; } - return { - viewport: queryGeometry, - worldCoordinate: queryGeometry.map((p) => this.transform.pointCoordinate(p)) - }; + return this.style.queryRenderedFeatures(queryGeometry, options, this.transform); } /** diff --git a/test/unit/style/style.test.js b/test/unit/style/style.test.js index df2d715022f..3f401c9ee24 100644 --- a/test/unit/style/style.test.js +++ b/test/unit/style/style.test.js @@ -1782,13 +1782,13 @@ test('Style#queryRenderedFeatures', (t) => { style._updateSources(transform); t.test('returns feature type', (t) => { - const results = style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {}, transform); + const results = style.queryRenderedFeatures([{x: 0, y: 0}], {}, transform); t.equal(results[0].geometry.type, 'Line'); t.end(); }); t.test('filters by `layers` option', (t) => { - const results = style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {layers: ['land']}, transform); + const results = style.queryRenderedFeatures([{x: 0, y: 0}], {layers: ['land']}, transform); t.equal(results.length, 2); t.end(); }); @@ -1798,26 +1798,26 @@ test('Style#queryRenderedFeatures', (t) => { t.stub(style, 'fire').callsFake((event) => { if (event.error && event.error.message.includes('parameters.layers must be an Array.')) errors++; }); - style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {layers:'string'}, transform); + style.queryRenderedFeatures([{x: 0, y: 0}], {layers:'string'}, transform); t.equals(errors, 1); t.end(); }); t.test('includes layout properties', (t) => { - const results = style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {}, transform); + const results = style.queryRenderedFeatures([{x: 0, y: 0}], {}, transform); const layout = results[0].layer.layout; t.deepEqual(layout['line-cap'], 'round'); t.end(); }); t.test('includes paint properties', (t) => { - const results = style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {}, transform); + const results = style.queryRenderedFeatures([{x: 0, y: 0}], {}, transform); t.deepEqual(results[2].layer.paint['line-color'], 'red'); t.end(); }); t.test('includes metadata', (t) => { - const results = style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {}, transform); + const results = style.queryRenderedFeatures([{x: 0, y: 0}], {}, transform); const layer = results[1].layer; t.equal(layer.metadata.something, 'else'); @@ -1826,14 +1826,14 @@ test('Style#queryRenderedFeatures', (t) => { }); t.test('include multiple layers', (t) => { - const results = style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {layers: ['land', 'landref']}, transform); + const results = style.queryRenderedFeatures([{x: 0, y: 0}], {layers: ['land', 'landref']}, transform); t.equals(results.length, 3); t.end(); }); t.test('does not query sources not implicated by `layers` parameter', (t) => { style.sourceCaches.mapbox.queryRenderedFeatures = function() { t.fail(); }; - style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {layers: ['land--other']}, transform); + style.queryRenderedFeatures([{x: 0, y: 0}], {layers: ['land--other']}, transform); t.end(); }); @@ -1842,7 +1842,7 @@ test('Style#queryRenderedFeatures', (t) => { t.stub(style, 'fire').callsFake((event) => { if (event.error && event.error.message.includes('does not exist in the map\'s style and cannot be queried for features.')) errors++; }); - const results = style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {layers:['merp']}, transform); + const results = style.queryRenderedFeatures([{x: 0, y: 0}], {layers:['merp']}, transform); t.equals(errors, 1); t.equals(results.length, 0); t.end(); @@ -1907,6 +1907,7 @@ test('Style#query*Features', (t) => { t.beforeEach((callback) => { transform = new Transform(); + transform.resize(100, 100); style = new Style(new StubMap()); style.loadJSON({ "version": 8, @@ -1935,7 +1936,7 @@ test('Style#query*Features', (t) => { }); t.test('queryRenderedFeatures emits an error on incorrect filter', (t) => { - t.deepEqual(style.queryRenderedFeatures({ worldCoordinate: [10, 100] }, {filter: 7}, transform), []); + t.deepEqual(style.queryRenderedFeatures([{x: 0, y: 0}], {filter: 7}, transform), []); t.match(onError.args[0][0].error.message, /queryRenderedFeatures\.filter/); t.end(); }); diff --git a/test/unit/ui/map.test.js b/test/unit/ui/map.test.js index e4f8b674fac..f708374943b 100755 --- a/test/unit/ui/map.test.js +++ b/test/unit/ui/map.test.js @@ -12,7 +12,6 @@ import simulate from 'mapbox-gl-js-test/simulate_interaction'; import fixed from 'mapbox-gl-js-test/fixed'; const fixedNum = fixed.Num; const fixedLngLat = fixed.LngLat; -const fixedCoord = fixed.Coord; function createStyleSource() { return { @@ -908,7 +907,7 @@ test('Map', (t) => { const output = map.queryRenderedFeatures(map.project(new LngLat(0, 0))); const args = map.style.queryRenderedFeatures.getCall(0).args; - t.deepEqual(args[0].worldCoordinate.map(c => fixedCoord(c)), [{ column: 0.5, row: 0.5, zoom: 0 }]); // query geometry + t.deepEqual(args[0], [{ x: 100, y: 100 }]); // query geometry t.deepEqual(args[1], {}); // params t.deepEqual(args[2], map.transform); // transform t.deepEqual(output, []); @@ -956,11 +955,7 @@ test('Map', (t) => { map.queryRenderedFeatures(map.project(new LngLat(360, 0))); - const coords = map.style.queryRenderedFeatures.getCall(0).args[0].worldCoordinate.map(c => fixedCoord(c)); - t.equal(coords[0].column, 1.5); - t.equal(coords[0].row, 0.5); - t.equal(coords[0].zoom, 0); - + t.deepEqual(map.style.queryRenderedFeatures.getCall(0).args[0], [{x: 612, y: 100}]); t.end(); }); }); From 3eaf80fedc45c2fce036e84b6828e0c5efb76246 Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Wed, 1 Aug 2018 12:41:44 +0300 Subject: [PATCH 3/3] move bounds logic from Map to Transform --- src/geo/transform.js | 40 ++++++++++++++++++++++++++++++++++++++-- src/ui/map.js | 41 ++++++++--------------------------------- 2 files changed, 46 insertions(+), 35 deletions(-) diff --git a/src/geo/transform.js b/src/geo/transform.js index 39ba0c5bde1..1209087582b 100644 --- a/src/geo/transform.js +++ b/src/geo/transform.js @@ -1,7 +1,7 @@ // @flow import LngLat from './lng_lat'; - +import LngLatBounds from './lng_lat_bounds'; import Point from '@mapbox/point-geometry'; import Coordinate from './coordinate'; import { wrap, clamp } from '../util/util'; @@ -52,7 +52,7 @@ class Transform { this._minZoom = minZoom || 0; this._maxZoom = maxZoom || 22; - this.latRange = [-85.05113, 85.05113]; + this.setMaxBounds(); this.width = 0; this.height = 0; @@ -401,6 +401,42 @@ class Transform { return new Point(p[0] / p[3], p[1] / p[3]); } + /** + * Returns the map's geographical bounds. When the bearing or pitch is non-zero, the visible region is not + * an axis-aligned rectangle, and the result is the smallest bounds that encompasses the visible region. + */ + getBounds(): LngLatBounds { + return new LngLatBounds() + .extend(this.pointLocation(new Point(0, 0))) + .extend(this.pointLocation(new Point(this.width, 0))) + .extend(this.pointLocation(new Point(this.width, this.height))) + .extend(this.pointLocation(new Point(0, this.height))); + } + + /** + * Returns the maximum geographical bounds the map is constrained to, or `null` if none set. + */ + getMaxBounds(): LngLatBounds | null { + if (!this.latRange || this.latRange.length !== 2 || + !this.lngRange || this.lngRange.length !== 2) return null; + + return new LngLatBounds([this.lngRange[0], this.latRange[0]], [this.lngRange[1], this.latRange[1]]); + } + + /** + * Sets or clears the map's geographical constraints. + */ + setMaxBounds(bounds?: LngLatBounds) { + if (bounds) { + this.lngRange = [bounds.getWest(), bounds.getEast()]; + this.latRange = [bounds.getSouth(), bounds.getNorth()]; + this._constrain(); + } else { + this.lngRange = null; + this.latRange = [-85.05113, 85.05113]; + } + } + /** * Calculate the posMatrix that, given a tile coordinate, would be used to display the tile on a map. * @param {UnwrappedTileID} unwrappedTileID; diff --git a/src/ui/map.js b/src/ui/map.js index 2fd5a933fed..9021cc5cf50 100755 --- a/src/ui/map.js +++ b/src/ui/map.js @@ -477,32 +477,16 @@ class Map extends Camera { /** * Returns the map's geographical bounds. When the bearing or pitch is non-zero, the visible region is not * an axis-aligned rectangle, and the result is the smallest bounds that encompasses the visible region. - * - * @returns {LngLatBounds} */ - getBounds() { - return new LngLatBounds() - .extend(this.transform.pointLocation(new Point(0, 0))) - .extend(this.transform.pointLocation(new Point(this.transform.width, 0))) - .extend(this.transform.pointLocation(new Point(this.transform.width, this.transform.height))) - .extend(this.transform.pointLocation(new Point(0, this.transform.height))); + getBounds(): LngLatBounds { + return this.transform.getBounds(); } /** - * Gets the map's geographical bounds. - * - * Returns the LngLatBounds by which pan and zoom operations on the map are constrained. - * - * @returns {LngLatBounds | null} The maximum bounds the map is constrained to, or `null` if none set. + * Returns the maximum geographical bounds the map is constrained to, or `null` if none set. */ - getMaxBounds () { - if (this.transform.latRange && this.transform.latRange.length === 2 && - this.transform.lngRange && this.transform.lngRange.length === 2) { - return new LngLatBounds([this.transform.lngRange[0], this.transform.latRange[0]], - [this.transform.lngRange[1], this.transform.latRange[1]]); - } else { - return null; - } + getMaxBounds(): LngLatBounds | null { + return this.transform.getMaxBounds(); } /** @@ -515,21 +499,12 @@ class Map extends Camera { * as close as possible to the operation's request while still * remaining within the bounds. * - * @param {LngLatBoundsLike | null | undefined} lnglatbounds The maximum bounds to set. If `null` or `undefined` is provided, the function removes the map's maximum bounds. + * @param {LngLatBoundsLike | null | undefined} bounds The maximum bounds to set. If `null` or `undefined` is provided, the function removes the map's maximum bounds. * @returns {Map} `this` */ - setMaxBounds(lnglatbounds: LngLatBoundsLike) { - if (lnglatbounds) { - const b = LngLatBounds.convert(lnglatbounds); - this.transform.lngRange = [b.getWest(), b.getEast()]; - this.transform.latRange = [b.getSouth(), b.getNorth()]; - this.transform._constrain(); - } else if (lnglatbounds === null || lnglatbounds === undefined) { - this.transform.lngRange = null; - this.transform.latRange = null; - } + setMaxBounds(bounds: LngLatBoundsLike) { + this.transform.setMaxBounds(LngLatBounds.convert(bounds)); return this._update(); - } /**