MAPSJS-2983 Write to the console if we need too many attempts getting… (

#2284)

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.

Signed-off-by: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com>
Signed-off-by: Fischer, Thomas <thomas.fischer@here.com>

@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,37 +372,36 @@ export class PoiBatchRegistry {
     }
 }
 
-// keep track of the missing textures, but only warn once
-const missingTextureName: Map<string, boolean> = new Map();
+// keep track of the missing textures, we throw an error if the number of attempts goes over some
+// threshold.
+const missingTextureName: Map<string, number> = new Map();
+const SEARCH_CACHE_ATTEMPTS = 5;
 
 function findImageItem(
     poiInfo: PoiInfo,
     imageCaches: MapViewImageCache[],
     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}'`);
-        poiInfo.isValid = false;
-        if (missingTextureName.get(imageTextureName) === undefined) {
-            missingTextureName.set(imageTextureName, true);
-            logger.error(`preparePoi: No imageTexture with name '${imageTextureName}' found`);
-        }
+    // 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) {
+        logger.error(`PoiRenderer::findImageItem: No imageItem found with name:
+            '${imageTexture?.image ?? imageTextureName}'
+            after ${SEARCH_CACHE_ATTEMPTS} attempts.`);
     }
-    return imageItem;
+    return undefined;
 }
 
 /**

@here/harp-mapview/test/PoiBatchRegistryTest.ts

@@ -25,12 +25,12 @@ 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;
+            expect(poiInfo.isValid).true;
         });
 
         it("uses PoiInfo's imageTexture's image by default as batch key", () => {

@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,90 @@ 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 testImageName = "testImage";
+        const createPointLabel = (name: string) => {
+            return {
+                poiInfo: {
+                    imageTextureName: name,
+                    isValid: true,
+                    buffer: undefined,
+                    technique: {}
+                },
+                visible: true
+            } as TextElement;
+        };
+
+        const addRandomImageToCache = (
+            testCache: MapViewImageCache,
+            name: string,
+            load: boolean
+        ) => {
+            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 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<ImageItem>` to
+            // `Promise<ImageItem>`.
+            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);
+
+            // 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<ImageItem>` to
+            // `Promise<ImageItem>`.
+            await Promise.resolve(testImageItem);
+
+            const imageLoaded = poiRenderer.prepareRender(pointLabel, mapEnv);
+            // Check that adding to the cache means it will now load.
+            expect(imageLoaded).true;
+        });
+    });
 });

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\/+/,