From 9f8c38e81c738e5c92d7f6e517e84f6439895f3d Mon Sep 17 00:00:00 2001 From: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> Date: Thu, 19 Aug 2021 16:50:00 +0200 Subject: [PATCH 1/5] MAPSJS-2983 Write to the console if we need too many attempts getting the image from the cache Some failed attempts are ok, because there are cases where the DataSource is attached and ready and the map has started to render, but the theme isn't fully set. There are other ways to fix the problem, but they are generally more invasive and could introduce regressions / performance issues, some options are: - In the TileGeometryCreator::createAllGeometries method, await on the getTheme() Promise - Don't allow new TextElements to be added while the theme is updating - Investigate and remove all calls to update the mapview while the method `addDataSource` is called and see if this can be removed, because no update calls means we don't render and request new tiles which may contain references to POI's that don't yet exist on the cache. For the sake of simplicity, this is the preferred method. Change-Id: I441a0ff5f63fd3d2a403b43b422c3044c902c7a6 Signed-off-by: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> --- @here/harp-mapview/lib/poi/PoiRenderer.ts | 35 +++++++++++------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/@here/harp-mapview/lib/poi/PoiRenderer.ts b/@here/harp-mapview/lib/poi/PoiRenderer.ts index 62673920a5..11a3011492 100644 --- a/@here/harp-mapview/lib/poi/PoiRenderer.ts +++ b/@here/harp-mapview/lib/poi/PoiRenderer.ts @@ -261,8 +261,7 @@ export class PoiBatchRegistry { const { imageItem, imageTexture } = poiInfo; if (!imageItem) { - // No image -> invisible -> ignore - poiInfo.isValid = false; + // No image found, therefore just return undefined. It will probably come in soon? return undefined; } @@ -373,8 +372,10 @@ export class PoiBatchRegistry { } } -// keep track of the missing textures, but only warn once -const missingTextureName: Map = new Map(); +// keep track of the missing textures, we throw an error if the number of attempts goes over some +// threshold. +const missingTextureName: Map = new Map(); +const SEARCH_CACHE_ATTEMPTS = 5; function findImageItem( poiInfo: PoiInfo, @@ -382,28 +383,26 @@ function findImageItem( imageTexture?: ImageTexture ): ImageItem | undefined { assert(poiInfo.imageTextureName !== undefined); - const imageTextureName = poiInfo.imageTextureName!; + const imageTextureName = imageTexture ? imageTexture.image : poiInfo.imageTextureName!; - let imageItem: ImageItem | undefined; for (const imageCache of imageCaches) { - imageItem = imageTexture - ? imageCache.findImageByName(imageTexture.image) - : imageCache.findImageByName(imageTextureName); + const imageItem = imageCache.findImageByName(imageTextureName); if (imageItem) { - break; + missingTextureName.delete(imageTextureName); + return imageItem; } } - if (!imageItem) { - logger.error(`init: No imageItem found with name - '${imageTexture?.image ?? imageTextureName}'`); + // There is a texture messing in the cache, we attempt again, and then error out. + const missingTextureCount = missingTextureName.get(imageTextureName); + missingTextureName.set(imageTextureName, missingTextureCount ? missingTextureCount + 1 : 0); + if (missingTextureName.get(imageTextureName)! >= SEARCH_CACHE_ATTEMPTS) { + logger.error(`PoiRenderer::findImageItem: No imageItem found with name: + '${imageTexture?.image ?? imageTextureName}' + after ${SEARCH_CACHE_ATTEMPTS} attempts.`); poiInfo.isValid = false; - if (missingTextureName.get(imageTextureName) === undefined) { - missingTextureName.set(imageTextureName, true); - logger.error(`preparePoi: No imageTexture with name '${imageTextureName}' found`); - } } - return imageItem; + return undefined; } /** From 02c4b0b904aa6636e0a22314a2113dff61167e6d Mon Sep 17 00:00:00 2001 From: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> Date: Thu, 19 Aug 2021 17:03:45 +0200 Subject: [PATCH 2/5] Update @here/harp-mapview/lib/poi/PoiRenderer.ts Signed-off-by: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> --- @here/harp-mapview/lib/poi/PoiRenderer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/@here/harp-mapview/lib/poi/PoiRenderer.ts b/@here/harp-mapview/lib/poi/PoiRenderer.ts index 11a3011492..d7b0093bc8 100644 --- a/@here/harp-mapview/lib/poi/PoiRenderer.ts +++ b/@here/harp-mapview/lib/poi/PoiRenderer.ts @@ -393,7 +393,7 @@ function findImageItem( } } - // There is a texture messing in the cache, we attempt again, and then error out. + // There is a texture missing in the cache, we attempt again, and then error out. const missingTextureCount = missingTextureName.get(imageTextureName); missingTextureName.set(imageTextureName, missingTextureCount ? missingTextureCount + 1 : 0); if (missingTextureName.get(imageTextureName)! >= SEARCH_CACHE_ATTEMPTS) { From 449318a6911061808a4d2b2e8e2693f70e833d8e Mon Sep 17 00:00:00 2001 From: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> Date: Fri, 20 Aug 2021 11:48:07 +0200 Subject: [PATCH 3/5] MAPSJS-2983 Fix broken test Signed-off-by: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> --- @here/harp-mapview/test/PoiBatchRegistryTest.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/@here/harp-mapview/test/PoiBatchRegistryTest.ts b/@here/harp-mapview/test/PoiBatchRegistryTest.ts index 9639185263..023282469a 100644 --- a/@here/harp-mapview/test/PoiBatchRegistryTest.ts +++ b/@here/harp-mapview/test/PoiBatchRegistryTest.ts @@ -25,12 +25,11 @@ describe("PoiBatchRegistry", () => { registry = new PoiBatchRegistry(renderer.capabilities); }); describe("registerPoi", function () { - it("marks PoiInfo as invalid if it has no image item", () => { + it("return undefined if poiInfo has no image item", () => { const poiInfo = new PoiInfoBuilder().build(textElement); const buffer = registry.registerPoi(poiInfo, defaultPoiLayer); expect(buffer).to.be.undefined; - expect((poiInfo as any).isValid).to.be.false; }); it("uses PoiInfo's imageTexture's image by default as batch key", () => { From 5a445456e2714a36e30be1709e5cdeee23c1a93c Mon Sep 17 00:00:00 2001 From: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> Date: Mon, 23 Aug 2021 11:07:24 +0200 Subject: [PATCH 4/5] MAPSJS-2983 Code review comments Signed-off-by: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> --- @here/harp-mapview/lib/poi/PoiRenderer.ts | 3 +- .../harp-mapview/test/PoiBatchRegistryTest.ts | 1 + @here/harp-mapview/test/PoiRendererTest.ts | 92 ++++++++++++++++++- karma.options.js | 1 + 4 files changed, 94 insertions(+), 3 deletions(-) diff --git a/@here/harp-mapview/lib/poi/PoiRenderer.ts b/@here/harp-mapview/lib/poi/PoiRenderer.ts index d7b0093bc8..ae72acc40a 100644 --- a/@here/harp-mapview/lib/poi/PoiRenderer.ts +++ b/@here/harp-mapview/lib/poi/PoiRenderer.ts @@ -396,11 +396,10 @@ function findImageItem( // There is a texture missing in the cache, we attempt again, and then error out. const missingTextureCount = missingTextureName.get(imageTextureName); missingTextureName.set(imageTextureName, missingTextureCount ? missingTextureCount + 1 : 0); - if (missingTextureName.get(imageTextureName)! >= SEARCH_CACHE_ATTEMPTS) { + if (missingTextureName.get(imageTextureName)! === SEARCH_CACHE_ATTEMPTS) { logger.error(`PoiRenderer::findImageItem: No imageItem found with name: '${imageTexture?.image ?? imageTextureName}' after ${SEARCH_CACHE_ATTEMPTS} attempts.`); - poiInfo.isValid = false; } return undefined; } diff --git a/@here/harp-mapview/test/PoiBatchRegistryTest.ts b/@here/harp-mapview/test/PoiBatchRegistryTest.ts index 023282469a..124868d8af 100644 --- a/@here/harp-mapview/test/PoiBatchRegistryTest.ts +++ b/@here/harp-mapview/test/PoiBatchRegistryTest.ts @@ -30,6 +30,7 @@ describe("PoiBatchRegistry", () => { const buffer = registry.registerPoi(poiInfo, defaultPoiLayer); expect(buffer).to.be.undefined; + expect(poiInfo.isValid).true; }); it("uses PoiInfo's imageTexture's image by default as batch key", () => { diff --git a/@here/harp-mapview/test/PoiRendererTest.ts b/@here/harp-mapview/test/PoiRendererTest.ts index d9efe0bd33..b743eab656 100644 --- a/@here/harp-mapview/test/PoiRendererTest.ts +++ b/@here/harp-mapview/test/PoiRendererTest.ts @@ -13,9 +13,15 @@ import { Env } from "@here/harp-datasource-protocol"; import { Math2D } from "@here/harp-utils"; import { expect } from "chai"; -import * as THREE from "three"; +import { MapViewImageCache } from "../lib/image/MapViewImageCache"; +import { MapView } from "../lib/MapView"; +import { PoiManager } from "../lib/poi/PoiManager"; import { PoiBuffer, PoiRenderer } from "../lib/poi/PoiRenderer"; +import { TextElement } from "../lib/text/TextElement"; + +import sinon = require("sinon"); +import * as THREE from "three"; describe("PoiRenderer", function () { describe("computeIconScreenBox", function () { @@ -97,4 +103,88 @@ describe("PoiRenderer", function () { expect(screenBox.h).to.equal(16); }); }); + + describe("search for cached images", function () { + const webGLRenderer = { + capabilities: { + isWebGL2: false + } + } as THREE.WebGLRenderer; + const mapViewStub = sinon.createStubInstance(MapView); + const poiManager = new PoiManager((mapViewStub as any) as MapView); + const testImageName = "testImage"; + const mapEnv = new Env(); + + const createPointLabel = (name: string) => { + return { + poiInfo: { + imageTextureName: name, + isValid: true, + buffer: undefined, + technique: {} + }, + visible: true + } as TextElement; + }; + + let x = 1; + let y = 1; + + const addRandomImageToCache = ( + testCache: MapViewImageCache, + name: string, + load: boolean + ) => { + // Note, the images must be unique, otherwise the test will fail, because the internal + // caching mechanism of the ImageCache will have already loaded the image. + return testCache.addImage(name, `https://picsum.photos/${x++}/${y++}`, load); + }; + + it("poi is valid when in cache and loading started", async function () { + this.timeout(5000); + + const testCache = new MapViewImageCache(); + const testImageItem = addRandomImageToCache(testCache, testImageName, true); + const caches = [testCache]; + const poiRenderer = new PoiRenderer(webGLRenderer, poiManager, caches); + const pointLabel = createPointLabel(testImageName); + const waitingForLoad = poiRenderer.prepareRender(pointLabel, mapEnv); + // This is false because the image is still being loaded in the background, notice the + // true flag passed to the call to testCache.addImage. + expect(waitingForLoad).false; + + // Promise.resolve used to convert the type `ImageItem | Promise` to + // `Promise`. + await Promise.resolve(testImageItem); + + const imageLoaded = poiRenderer.prepareRender(pointLabel, mapEnv); + expect(imageLoaded).true; + + // Check that a second attempt to prepareRender succeeds. + const imageLoadedAgain = poiRenderer.prepareRender(pointLabel, mapEnv); + expect(imageLoadedAgain).true; + }); + + it("poi is invalid when not in cache, adding to cache means it will load", async function () { + this.timeout(5000); + + const testCache = new MapViewImageCache(); + // Empty cache for now + const caches = [testCache]; + const poiRenderer = new PoiRenderer(webGLRenderer, poiManager, caches); + const pointLabel = createPointLabel(testImageName); + const waitingForLoad = poiRenderer.prepareRender(pointLabel, mapEnv); + expect(waitingForLoad).false; + + const testImageItem = addRandomImageToCache(testCache, testImageName, true); + + // Promise.resolve used to convert the type `ImageItem | Promise` to + // `Promise`. + await Promise.resolve(testImageItem); + + const imageLoaded = poiRenderer.prepareRender(pointLabel, mapEnv); + // Check that adding to the cache means it will now load. + expect(imageLoaded).true; + }); + }); }); diff --git a/karma.options.js b/karma.options.js index 2f5e88f4a9..8e879cd537 100644 --- a/karma.options.js +++ b/karma.options.js @@ -170,6 +170,7 @@ const options = function (isCoverage, isMapSdk, prefixDirectory) { allowJs: true }, coverageOptions: { + instrumentation: isCoverage ? true : false, // This is needed otherwise the tests are included in the code coverage %. exclude: [ /test\/+/, From 715bad566266fedc51a559fe88ee248f053e3d5e Mon Sep 17 00:00:00 2001 From: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> Date: Mon, 23 Aug 2021 15:44:37 +0200 Subject: [PATCH 5/5] code review fixes: move initialization to beforeEach and use local resource Signed-off-by: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com> --- @here/harp-mapview/test/PoiRendererTest.ts | 24 ++++++++++++---------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/@here/harp-mapview/test/PoiRendererTest.ts b/@here/harp-mapview/test/PoiRendererTest.ts index b743eab656..c5e60ed33d 100644 --- a/@here/harp-mapview/test/PoiRendererTest.ts +++ b/@here/harp-mapview/test/PoiRendererTest.ts @@ -111,10 +111,7 @@ describe("PoiRenderer", function () { } } as THREE.WebGLRenderer; const mapViewStub = sinon.createStubInstance(MapView); - const poiManager = new PoiManager((mapViewStub as any) as MapView); const testImageName = "testImage"; - const mapEnv = new Env(); - const createPointLabel = (name: string) => { return { poiInfo: { @@ -127,23 +124,29 @@ describe("PoiRenderer", function () { } as TextElement; }; - let x = 1; - let y = 1; - const addRandomImageToCache = ( testCache: MapViewImageCache, name: string, load: boolean ) => { - // Note, the images must be unique, otherwise the test will fail, because the internal - // caching mechanism of the ImageCache will have already loaded the image. - return testCache.addImage(name, `https://picsum.photos/${x++}/${y++}`, load); + return testCache.addImage( + name, + `/@here/harp-mapview/test/resources/headshot.jpg`, + load + ); }; + let poiManager: PoiManager; + let mapEnv: Env; + let testCache: MapViewImageCache; + beforeEach(() => { + poiManager = new PoiManager((mapViewStub as any) as MapView); + mapEnv = new Env(); + testCache = new MapViewImageCache(); + }); it("poi is valid when in cache and loading started", async function () { this.timeout(5000); - const testCache = new MapViewImageCache(); const testImageItem = addRandomImageToCache(testCache, testImageName, true); const caches = [testCache]; const poiRenderer = new PoiRenderer(webGLRenderer, poiManager, caches); @@ -168,7 +171,6 @@ describe("PoiRenderer", function () { it("poi is invalid when not in cache, adding to cache means it will load", async function () { this.timeout(5000); - const testCache = new MapViewImageCache(); // Empty cache for now const caches = [testCache]; const poiRenderer = new PoiRenderer(webGLRenderer, poiManager, caches);