From 5195adea8521ba90da1ae97a325678461d3508ba Mon Sep 17 00:00:00 2001 From: Frederic Junod Date: Tue, 18 Dec 2018 09:06:41 +0100 Subject: [PATCH 1/7] Remove 'layerStates' property from the FrameState --- src/ol/PluggableMap.js | 9 +------- src/ol/layer/Layer.js | 2 -- src/ol/renderer/Map.js | 23 ++++++++++++++++--- src/ol/source/Raster.js | 14 +---------- test/spec/ol/layer/layer.test.js | 8 ++----- .../ol/renderer/canvas/vectorlayer.test.js | 4 +--- .../renderer/canvas/vectortilelayer.test.js | 4 +--- 7 files changed, 26 insertions(+), 38 deletions(-) diff --git a/src/ol/PluggableMap.js b/src/ol/PluggableMap.js index 1a702641aa..7447acace7 100644 --- a/src/ol/PluggableMap.js +++ b/src/ol/PluggableMap.js @@ -41,7 +41,6 @@ import {create as createTransform, apply as applyTransform} from './transform.js * @property {null|import("./extent.js").Extent} extent * @property {import("./coordinate.js").Coordinate} focus * @property {number} index - * @property {Object} layerStates * @property {Array} layerStatesArray * @property {import("./transform.js").Transform} pixelToCoordinateTransform * @property {Array} postRenderFunctions @@ -1188,11 +1187,6 @@ class PluggableMap extends BaseObject { let frameState = null; if (size !== undefined && hasArea(size) && view && view.isDef()) { const viewHints = view.getHints(this.frameState_ ? this.frameState_.viewHints : undefined); - const layerStatesArray = this.getLayerGroup().getLayerStatesArray(); - const layerStates = {}; - for (let i = 0, ii = layerStatesArray.length; i < ii; ++i) { - layerStates[getUid(layerStatesArray[i].layer)] = layerStatesArray[i]; - } viewState = view.getState(this.pixelRatio_); frameState = /** @type {FrameState} */ ({ animate: false, @@ -1200,8 +1194,7 @@ class PluggableMap extends BaseObject { extent: extent, focus: this.focus_ ? this.focus_ : viewState.center, index: this.frameIndex_++, - layerStates: layerStates, - layerStatesArray: layerStatesArray, + layerStatesArray: this.getLayerGroup().getLayerStatesArray(), pixelRatio: this.pixelRatio_, pixelToCoordinateTransform: this.pixelToCoordinateTransform_, postRenderFunctions: [], diff --git a/src/ol/layer/Layer.js b/src/ol/layer/Layer.js index 723c7b5be1..d735a4eeef 100644 --- a/src/ol/layer/Layer.js +++ b/src/ol/layer/Layer.js @@ -3,7 +3,6 @@ */ import {listen, unlistenByKey} from '../events.js'; import EventType from '../events/EventType.js'; -import {getUid} from '../util.js'; import {getChangeEventType} from '../Object.js'; import BaseLayer from './Base.js'; import LayerProperty from './Property.js'; @@ -219,7 +218,6 @@ class Layer extends BaseLayer { layerState.zIndex = Infinity; } renderEvent.frameState.layerStatesArray.push(layerState); - renderEvent.frameState.layerStates[getUid(this)] = layerState; }, this); this.mapRenderKey_ = listen(this, EventType.CHANGE, map.render, map); this.changed(); diff --git a/src/ol/renderer/Map.js b/src/ol/renderer/Map.js index dcd4f96a69..105d41014f 100644 --- a/src/ol/renderer/Map.js +++ b/src/ol/renderer/Map.js @@ -1,6 +1,7 @@ /** * @module ol/renderer/Map */ +import {includes} from '../array.js'; import {abstract, getUid} from '../util.js'; import Disposable from '../Disposable.js'; import {listen, unlistenByKey} from '../events.js'; @@ -105,6 +106,11 @@ class MapRenderer extends Disposable { let result; const viewState = frameState.viewState; const viewResolution = viewState.resolution; + const managedLayers = frameState.layerStatesArray.map(function(state) { + if (state.managed) { + return getUid(state.layer); + } + }); /** * @param {import("../Feature.js").FeatureLike} feature Feature. @@ -112,7 +118,7 @@ class MapRenderer extends Disposable { * @return {?} Callback result. */ function forEachFeatureAtCoordinate(feature, layer) { - const managed = frameState.layerStates[getUid(layer)].managed; + const managed = includes(managedLayers, getUid(layer)); if (!(getUid(feature) in frameState.skippedFeatureUids && !managed)) { return callback.call(thisArg, feature, managed ? layer : null); } @@ -255,8 +261,9 @@ class MapRenderer extends Disposable { * @private */ removeUnusedLayerRenderers_(map, frameState) { + const layersUids = getLayersUids(frameState.layerStatesArray); for (const layerKey in this.layerRenderers_) { - if (!frameState || !(layerKey in frameState.layerStates)) { + if (!frameState || !(includes(layersUids, layerKey))) { this.removeLayerRendererByKey_(layerKey).dispose(); } } @@ -284,8 +291,9 @@ class MapRenderer extends Disposable { * @protected */ scheduleRemoveUnusedLayerRenderers(frameState) { + const layersUids = getLayersUids(frameState.layerStatesArray); for (const layerKey in this.layerRenderers_) { - if (!(layerKey in frameState.layerStates)) { + if (!(includes(layersUids, layerKey))) { frameState.postRenderFunctions.push( /** @type {import("../PluggableMap.js").PostRenderFunction} */ (this.removeUnusedLayerRenderers_.bind(this)) ); @@ -304,6 +312,15 @@ function expireIconCache(map, frameState) { iconImageCache.expire(); } +/** + * @param {Array} layerStatesArray Layer states array. + * @return {Array} Layers uid. + */ +function getLayersUids(layerStatesArray) { + return layerStatesArray.map(function(state) { + return getUid(state.layer); + }); +} /** * @param {import("../layer/Layer.js").State} state1 First layer state. diff --git a/src/ol/source/Raster.js b/src/ol/source/Raster.js index 6f3cbe229c..0e448d69f8 100644 --- a/src/ol/source/Raster.js +++ b/src/ol/source/Raster.js @@ -1,7 +1,6 @@ /** * @module ol/source/Raster */ -import {getUid} from '../util.js'; import ImageCanvas from '../ImageCanvas.js'; import TileQueue from '../TileQueue.js'; import {createCanvasContext2D} from '../dom.js'; @@ -185,16 +184,6 @@ class RasterSource extends ImageSource { return 1; }, this.changed.bind(this)); - const layerStatesArray = getLayerStatesArray(this.layers_); - - /** - * @type {Object} - */ - const layerStates = {}; - for (let i = 0, ii = layerStatesArray.length; i < ii; ++i) { - layerStates[getUid(layerStatesArray[i].layer)] = layerStatesArray[i]; - } - /** * The most recently requested frame state. * @type {import("../PluggableMap.js").FrameState} @@ -225,8 +214,7 @@ class RasterSource extends ImageSource { extent: null, focus: null, index: 0, - layerStates: layerStates, - layerStatesArray: layerStatesArray, + layerStatesArray: getLayerStatesArray(this.layers_), pixelRatio: 1, pixelToCoordinateTransform: createTransform(), postRenderFunctions: [], diff --git a/test/spec/ol/layer/layer.test.js b/test/spec/ol/layer/layer.test.js index ff3aeb97a5..e077455ac1 100644 --- a/test/spec/ol/layer/layer.test.js +++ b/test/spec/ol/layer/layer.test.js @@ -1,4 +1,3 @@ -import {getUid} from '../../../../src/ol/util.js'; import Map from '../../../../src/ol/Map.js'; import Layer, {visibleAtResolution} from '../../../../src/ol/layer/Layer.js'; import {get as getProjection} from '../../../../src/ol/proj.js'; @@ -396,15 +395,12 @@ describe('ol.layer.Layer', function() { map: map }); const frameState = { - layerStatesArray: [], - layerStates: {} + layerStatesArray: [] }; - map.dispatchEvent(new RenderEvent('precompose', null, - frameState, null, null)); + map.dispatchEvent(new RenderEvent('precompose', null, frameState, null, null)); expect(frameState.layerStatesArray.length).to.be(1); const layerState = frameState.layerStatesArray[0]; expect(layerState.layer).to.equal(layer); - expect(frameState.layerStates[getUid(layer)]).to.equal(layerState); }); }); diff --git a/test/spec/ol/renderer/canvas/vectorlayer.test.js b/test/spec/ol/renderer/canvas/vectorlayer.test.js index d540eb781d..b34d91ead7 100644 --- a/test/spec/ol/renderer/canvas/vectorlayer.test.js +++ b/test/spec/ol/renderer/canvas/vectorlayer.test.js @@ -1,4 +1,3 @@ -import {getUid} from '../../../../../src/ol/util.js'; import Feature from '../../../../../src/ol/Feature.js'; import Map from '../../../../../src/ol/Map.js'; import View from '../../../../../src/ol/View.js'; @@ -203,14 +202,13 @@ describe('ol.renderer.canvas.VectorLayer', function() { const spy = sinon.spy(); const coordinate = [0, 0]; const frameState = { - layerStates: {}, + layerStatesArray: [{}], skippedFeatureUids: {}, viewState: { resolution: 1, rotation: 0 } }; - frameState.layerStates[getUid(layer)] = {}; renderer.forEachFeatureAtCoordinate( coordinate, frameState, 0, spy, undefined); expect(spy.callCount).to.be(1); diff --git a/test/spec/ol/renderer/canvas/vectortilelayer.test.js b/test/spec/ol/renderer/canvas/vectortilelayer.test.js index 5521b586af..8e44634d7a 100644 --- a/test/spec/ol/renderer/canvas/vectortilelayer.test.js +++ b/test/spec/ol/renderer/canvas/vectortilelayer.test.js @@ -1,4 +1,3 @@ -import {getUid} from '../../../../../src/ol/util.js'; import {clear} from '../../../../../src/ol/obj.js'; import Feature from '../../../../../src/ol/Feature.js'; import Map from '../../../../../src/ol/Map.js'; @@ -329,7 +328,7 @@ describe('ol.renderer.canvas.VectorTileLayer', function() { const spy = sinon.spy(); const coordinate = [0, 0]; const frameState = { - layerStates: {}, + layerStatesArray: [{}], skippedFeatureUids: {}, viewState: { projection: getProjection('EPSG:3857'), @@ -337,7 +336,6 @@ describe('ol.renderer.canvas.VectorTileLayer', function() { rotation: 0 } }; - frameState.layerStates[getUid(layer)] = {}; renderer.renderedTiles = [new TileClass([0, 0, -1], undefined, 1)]; renderer.forEachFeatureAtCoordinate( coordinate, frameState, 0, spy, undefined); From 1750ff43e0de864dba3d5d20d40cd95f90e12c9c Mon Sep 17 00:00:00 2001 From: Frederic Junod Date: Tue, 18 Dec 2018 09:46:36 +0100 Subject: [PATCH 2/7] Always schedule unused layers removal function --- src/ol/renderer/Map.js | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/ol/renderer/Map.js b/src/ol/renderer/Map.js index 105d41014f..0c524e34af 100644 --- a/src/ol/renderer/Map.js +++ b/src/ol/renderer/Map.js @@ -291,15 +291,7 @@ class MapRenderer extends Disposable { * @protected */ scheduleRemoveUnusedLayerRenderers(frameState) { - const layersUids = getLayersUids(frameState.layerStatesArray); - for (const layerKey in this.layerRenderers_) { - if (!(includes(layersUids, layerKey))) { - frameState.postRenderFunctions.push( - /** @type {import("../PluggableMap.js").PostRenderFunction} */ (this.removeUnusedLayerRenderers_.bind(this)) - ); - return; - } - } + frameState.postRenderFunctions.push(this.removeUnusedLayerRenderers_.bind(this)); } } From 5d1c27d05d01c63bb05a996ea04dae750ec9b9f7 Mon Sep 17 00:00:00 2001 From: Frederic Junod Date: Tue, 18 Dec 2018 13:17:00 +0100 Subject: [PATCH 3/7] Don't create an array of managed layers --- src/ol/renderer/Map.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/ol/renderer/Map.js b/src/ol/renderer/Map.js index 0c524e34af..1098688a63 100644 --- a/src/ol/renderer/Map.js +++ b/src/ol/renderer/Map.js @@ -106,19 +106,14 @@ class MapRenderer extends Disposable { let result; const viewState = frameState.viewState; const viewResolution = viewState.resolution; - const managedLayers = frameState.layerStatesArray.map(function(state) { - if (state.managed) { - return getUid(state.layer); - } - }); /** + * @param {boolean} managed Managed layer. * @param {import("../Feature.js").FeatureLike} feature Feature. * @param {import("../layer/Layer.js").default} layer Layer. * @return {?} Callback result. */ - function forEachFeatureAtCoordinate(feature, layer) { - const managed = includes(managedLayers, getUid(layer)); + function forEachFeatureAtCoordinate(managed, feature, layer) { if (!(getUid(feature) in frameState.skippedFeatureUids && !managed)) { return callback.call(thisArg, feature, managed ? layer : null); } @@ -147,9 +142,10 @@ class MapRenderer extends Disposable { const layerRenderer = this.getLayerRenderer(layer); const source = layer.getSource(); if (layerRenderer && source) { + const callback = forEachFeatureAtCoordinate.bind(null, layerState.managed); result = layerRenderer.forEachFeatureAtCoordinate( source.getWrapX() ? translatedCoordinate : coordinate, - frameState, hitTolerance, forEachFeatureAtCoordinate); + frameState, hitTolerance, callback); } if (result) { return result; From 016d7382693bb3feef1cbc25b3a9b265c0dee731 Mon Sep 17 00:00:00 2001 From: Frederic Junod Date: Tue, 18 Dec 2018 13:34:19 +0100 Subject: [PATCH 4/7] Move frameState test on top in removeUnusedLayerRenderers_ --- src/ol/renderer/Map.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/ol/renderer/Map.js b/src/ol/renderer/Map.js index 1098688a63..1387192392 100644 --- a/src/ol/renderer/Map.js +++ b/src/ol/renderer/Map.js @@ -257,10 +257,12 @@ class MapRenderer extends Disposable { * @private */ removeUnusedLayerRenderers_(map, frameState) { - const layersUids = getLayersUids(frameState.layerStatesArray); - for (const layerKey in this.layerRenderers_) { - if (!frameState || !(includes(layersUids, layerKey))) { - this.removeLayerRendererByKey_(layerKey).dispose(); + if (frameState) { + const layersUids = getLayersUids(frameState.layerStatesArray); + for (const layerKey in this.layerRenderers_) { + if (!includes(layersUids, layerKey)) { + this.removeLayerRendererByKey_(layerKey).dispose(); + } } } } From a4fe067aad5ee19be3fca953e8232fde7c3a25c6 Mon Sep 17 00:00:00 2001 From: Frederic Junod Date: Wed, 19 Dec 2018 10:49:50 +0100 Subject: [PATCH 5/7] Create a layer state map instead of an array of uids --- src/ol/renderer/Map.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/ol/renderer/Map.js b/src/ol/renderer/Map.js index 1387192392..9bbbc61115 100644 --- a/src/ol/renderer/Map.js +++ b/src/ol/renderer/Map.js @@ -1,7 +1,6 @@ /** * @module ol/renderer/Map */ -import {includes} from '../array.js'; import {abstract, getUid} from '../util.js'; import Disposable from '../Disposable.js'; import {listen, unlistenByKey} from '../events.js'; @@ -258,9 +257,9 @@ class MapRenderer extends Disposable { */ removeUnusedLayerRenderers_(map, frameState) { if (frameState) { - const layersUids = getLayersUids(frameState.layerStatesArray); + const layerStatesMap = getLayerStatesMap(frameState.layerStatesArray); for (const layerKey in this.layerRenderers_) { - if (!includes(layersUids, layerKey)) { + if (!(layerKey in layerStatesMap)) { this.removeLayerRendererByKey_(layerKey).dispose(); } } @@ -304,12 +303,13 @@ function expireIconCache(map, frameState) { /** * @param {Array} layerStatesArray Layer states array. - * @return {Array} Layers uid. + * @return {Object} States mapped by layer uid. */ -function getLayersUids(layerStatesArray) { - return layerStatesArray.map(function(state) { - return getUid(state.layer); - }); +function getLayerStatesMap(layerStatesArray) { + return layerStatesArray.reduce(function(acc, state) { + acc[getUid(state.layer)] = state; + return acc; + }, {}); } /** From 3193de0906a2cd961d5f40de960b3caf530db29b Mon Sep 17 00:00:00 2001 From: Frederic Junod Date: Wed, 19 Dec 2018 14:25:02 +0100 Subject: [PATCH 6/7] Only schedule the icon cache expire when it's needed --- src/ol/PluggableMap.js | 2 +- src/ol/renderer/Map.js | 4 +++- src/ol/style/IconImageCache.js | 9 ++++++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/ol/PluggableMap.js b/src/ol/PluggableMap.js index 7447acace7..573c0276ef 100644 --- a/src/ol/PluggableMap.js +++ b/src/ol/PluggableMap.js @@ -54,7 +54,7 @@ import {create as createTransform, apply as applyTransform} from './transform.js /** - * @typedef {function(PluggableMap, ?FrameState): boolean} PostRenderFunction + * @typedef {function(PluggableMap, ?FrameState): any} PostRenderFunction */ diff --git a/src/ol/renderer/Map.js b/src/ol/renderer/Map.js index 9bbbc61115..7555ec3335 100644 --- a/src/ol/renderer/Map.js +++ b/src/ol/renderer/Map.js @@ -280,7 +280,9 @@ class MapRenderer extends Disposable { * @protected */ scheduleExpireIconCache(frameState) { - frameState.postRenderFunctions.push(/** @type {import("../PluggableMap.js").PostRenderFunction} */ (expireIconCache)); + if (iconImageCache.canExpireCache()) { + frameState.postRenderFunctions.push(expireIconCache); + } } /** diff --git a/src/ol/style/IconImageCache.js b/src/ol/style/IconImageCache.js index 0780d814e1..b9809c7c89 100644 --- a/src/ol/style/IconImageCache.js +++ b/src/ol/style/IconImageCache.js @@ -37,11 +37,18 @@ class IconImageCache { this.cacheSize_ = 0; } + /** + * @return {boolean} Can expire cache. + */ + canExpireCache() { + return this.cacheSize_ > this.maxCacheSize_; + } + /** * FIXME empty description for jsdoc */ expire() { - if (this.cacheSize_ > this.maxCacheSize_) { + if (this.canExpireCache()) { let i = 0; for (const key in this.cache_) { const iconImage = this.cache_[key]; From a0f15e1eb6c00a5bb236f0ac218075ad057e5b7c Mon Sep 17 00:00:00 2001 From: Frederic Junod Date: Wed, 19 Dec 2018 14:49:28 +0100 Subject: [PATCH 7/7] Only schedule the unused layers renderer removal when it's needed --- src/ol/renderer/Map.js | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/src/ol/renderer/Map.js b/src/ol/renderer/Map.js index 7555ec3335..ef2cb51338 100644 --- a/src/ol/renderer/Map.js +++ b/src/ol/renderer/Map.js @@ -250,22 +250,6 @@ class MapRenderer extends Disposable { return layerRenderer; } - /** - * @param {import("../PluggableMap.js").default} map Map. - * @param {import("../PluggableMap.js").FrameState} frameState Frame state. - * @private - */ - removeUnusedLayerRenderers_(map, frameState) { - if (frameState) { - const layerStatesMap = getLayerStatesMap(frameState.layerStatesArray); - for (const layerKey in this.layerRenderers_) { - if (!(layerKey in layerStatesMap)) { - this.removeLayerRendererByKey_(layerKey).dispose(); - } - } - } - } - /** * Render. * @abstract @@ -290,7 +274,14 @@ class MapRenderer extends Disposable { * @protected */ scheduleRemoveUnusedLayerRenderers(frameState) { - frameState.postRenderFunctions.push(this.removeUnusedLayerRenderers_.bind(this)); + const layerStatesMap = getLayerStatesMap(frameState.layerStatesArray); + for (const layerKey in this.layerRenderers_) { + if (!(layerKey in layerStatesMap)) { + frameState.postRenderFunctions.push(function() { + this.removeLayerRendererByKey_(layerKey).dispose(); + }.bind(this)); + } + } } }