Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

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

Merged
merged 5 commits into from
Aug 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 17 additions & 19 deletions @here/harp-mapview/lib/poi/PoiRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: we could do missingTextureName.get(imageTextureName) only once.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I would need to then do the +1 twice. What is preferred?

logger.error(`PoiRenderer::findImageItem: No imageItem found with name:
'${imageTexture?.image ?? imageTextureName}'
after ${SEARCH_CACHE_ATTEMPTS} attempts.`);
nzjony marked this conversation as resolved.
Show resolved Hide resolved
}
return imageItem;
return undefined;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions @here/harp-mapview/test/PoiBatchRegistryTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
nzjony marked this conversation as resolved.
Show resolved Hide resolved
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", () => {
Expand Down
94 changes: 93 additions & 1 deletion @here/harp-mapview/test/PoiRendererTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -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;
});
});
});
1 change: 1 addition & 0 deletions karma.options.js
Original file line number Diff line number Diff line change
Expand Up @@ -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\/+/,
Expand Down