diff --git a/@here/harp-mapview/lib/Tile.ts b/@here/harp-mapview/lib/Tile.ts index e58c9cb067..18b9c000ce 100644 --- a/@here/harp-mapview/lib/Tile.ts +++ b/@here/harp-mapview/lib/Tile.ts @@ -360,7 +360,7 @@ export class Tile implements CachedResource { // Used for [[TextElement]]s which the developer defines. Group created with maximum priority // so that user text elements are placed before others. - private readonly m_userTextElements = new TextElementGroup(Number.MAX_SAFE_INTEGER); + private m_userTextElements = new TextElementGroup(Number.MAX_SAFE_INTEGER); // Used for [[TextElement]]s that are stored in the data, and that are placed explicitly, // fading in and out. @@ -369,8 +369,9 @@ export class Tile implements CachedResource { // Blocks other labels from showing. private readonly m_pathBlockingElements: PathBlockingElement[] = []; - // If `true`, the text content of the [[Tile]] changed. - private m_textElementsChanged: boolean = false; + // If `true`, the text content of the [[Tile]] changed after the last time it was rendered. + // It's `Undefined` when no text content has been added yet. + private m_textElementsChanged: boolean | undefined; private m_visibleArea: number = 0; private m_minElevation: number = 0; @@ -505,6 +506,15 @@ export class Tile implements CachedResource { * @param textElement The Text element to add. */ addUserTextElement(textElement: TextElement) { + if (this.m_textElementsChanged === false) { + // HARP-8733: Text content in the tile is about to change, but it has already been + // rendered at least once (otherwise m_textElementsChanged would be undefined). Clone + // the text element group so that it's handled as a new group by TextElementsRenderer + // and it doesn't reuse the same state stored for the old one. + // TODO: HARP-8910 Deprecate user text elements. + this.m_userTextElements = this.m_userTextElements.clone(); + } + this.m_userTextElements.elements.push(textElement); this.textElementsChanged = true; } @@ -517,12 +527,21 @@ export class Tile implements CachedResource { */ removeUserTextElement(textElement: TextElement): boolean { const foundIndex = this.m_userTextElements.elements.indexOf(textElement); - if (foundIndex >= 0) { - this.m_userTextElements.elements.splice(foundIndex, 1); - this.textElementsChanged = true; - return true; + if (foundIndex === -1) { + return false; } - return false; + + if (this.m_textElementsChanged === false) { + // HARP-8733: Text content in the tile is about to change, but it has already been + // rendered at least once (otherwise m_textElementsChanged would be undefined). Clone + // the text element group so that it's handled as a new group by TextElementsRenderer + // and it doesn't reuse the same state stored for the old one. + // TODO: HARP-8910 Deprecate user text elements. + this.m_userTextElements = this.m_userTextElements.clone(); + } + this.m_userTextElements.elements.splice(foundIndex, 1); + this.textElementsChanged = true; + return true; } /** @@ -579,7 +598,7 @@ export class Tile implements CachedResource { * value is `true` the TextElement is placed for rendering during the next frame. */ get textElementsChanged(): boolean { - return this.m_textElementsChanged; + return this.m_textElementsChanged ?? false; } set textElementsChanged(changed: boolean) { diff --git a/@here/harp-mapview/lib/text/TextElement.ts b/@here/harp-mapview/lib/text/TextElement.ts index a1ce40520d..961ab34485 100644 --- a/@here/harp-mapview/lib/text/TextElement.ts +++ b/@here/harp-mapview/lib/text/TextElement.ts @@ -334,7 +334,7 @@ export class TextElement { * Creates a new `TextElement`. * * @param text The text to display. - * @param points The position or a list of points for a curved text, both in local tile space. + * @param points The position or a list of points for a curved text, both in world space. * @param renderParams `TextElement` text rendering parameters. * @param layoutParams `TextElement` text layout parameters. * @param priority The priority of the `TextElement. Elements with the highest priority get @@ -376,7 +376,7 @@ export class TextElement { /** * The text element position or the first point of the path used to render a curved text, both - * in local tile space. + * in world space. */ get position(): THREE.Vector3 { if (this.points instanceof Array) { @@ -387,7 +387,7 @@ export class TextElement { } /** - * The list of points in local tile space used to render the text along a path or `undefined`. + * The list of points in world space used to render the text along a path or `undefined`. */ get path(): THREE.Vector3[] | undefined { if (this.points instanceof Array) { diff --git a/@here/harp-mapview/test/PoiInfoBuilder.ts b/@here/harp-mapview/test/PoiInfoBuilder.ts index 49c74ce102..5af7ab93db 100644 --- a/@here/harp-mapview/test/PoiInfoBuilder.ts +++ b/@here/harp-mapview/test/PoiInfoBuilder.ts @@ -57,6 +57,11 @@ export class PoiInfoBuilder { return this; } + withMayOverlap(mayOverlap: boolean): PoiInfoBuilder { + this.m_mayOverlap = mayOverlap; + return this; + } + build(textElement: TextElement): PoiInfo { return { technique: this.m_technique, diff --git a/@here/harp-mapview/test/TextElementBuilder.ts b/@here/harp-mapview/test/TextElementBuilder.ts index c02d242b5b..64a0a09f20 100644 --- a/@here/harp-mapview/test/TextElementBuilder.ts +++ b/@here/harp-mapview/test/TextElementBuilder.ts @@ -20,6 +20,7 @@ export const DEF_PRIORITY: number = 0; export const DEF_POSITION = new THREE.Vector3(0, 0, 0); export const DEF_PATH = [new THREE.Vector3(0, 0, 0), new THREE.Vector3(0.1, 0.1, 0)]; export const DEF_IGNORE_DISTANCE: boolean = true; +export const DEF_MAY_OVERLAP: boolean = false; const DEF_TILE_CENTER = new THREE.Vector3(0, 0, 0.1); export function pointTextBuilder(text: string = DEF_TEXT): TextElementBuilder { @@ -57,6 +58,7 @@ export class TextElementBuilder { private m_featureId: number | undefined; private m_pathLengthSqr: number | undefined; private m_userData: any; + private m_mayOverlap: boolean = DEF_MAY_OVERLAP; withPoiInfo(poiInfoBuilder: PoiInfoBuilder): TextElementBuilder { this.m_poiInfoBuilder = poiInfoBuilder; @@ -117,6 +119,10 @@ export class TextElementBuilder { return this; } + withMayOverlap(mayOverlap: boolean): TextElementBuilder { + this.m_mayOverlap = mayOverlap; + return this; + } build(sandbox: sinon.SinonSandbox): TextElement { const textElement = new TextElement( this.m_text, @@ -134,6 +140,8 @@ export class TextElementBuilder { } textElement.userData = this.m_userData; textElement.pathLengthSqr = this.m_pathLengthSqr; + textElement.mayOverlap = this.m_mayOverlap; + // Stub render style setter, so that a spy is installed on the style opacity // whenever it's called. const renderStyleProperty = Object.getOwnPropertyDescriptor( diff --git a/@here/harp-mapview/test/TextElementsRendererTest.ts b/@here/harp-mapview/test/TextElementsRendererTest.ts index 22120a2b2e..f674e429fd 100644 --- a/@here/harp-mapview/test/TextElementsRendererTest.ts +++ b/@here/harp-mapview/test/TextElementsRendererTest.ts @@ -39,6 +39,7 @@ import { fadeInAndFadedOut, FadeState, firstNFrames, + framesEnabled, frameStates, INITIAL_TIME, InputTextElement, @@ -125,6 +126,93 @@ const tests: TestCase[] = [ ], frameTimes: FADE_2_CYCLES }, + // USER LABELS + { + name: "Newly visited, visible user poi fades in", + tiles: [{ userLabels: [[poiBuilder(), FADE_IN]] }], + frameTimes: FADE_CYCLE + }, + { + name: "User poi added after first frames fades in", + tiles: [ + { + userLabels: [ + [ + poiBuilder(), + fadedOut(FADE_IN.length).concat( + fadeIn(FADE_2_CYCLES.length - FADE_IN.length) + ), + not(firstNFrames(FADE_2_CYCLES, FADE_IN.length)) + ] + ] + } + ], + frameTimes: FADE_2_CYCLES + }, + { + name: "Two user poi added in different frames to same tile fade in", + tiles: [ + { + userLabels: [ + [ + poiBuilder("Marker 1") + .withMayOverlap(true) + .withPoiInfo(new PoiInfoBuilder().withMayOverlap(true)), + fadedOut(2).concat(fadeIn(FADE_2_CYCLES.length - 2)), + not(firstNFrames(FADE_2_CYCLES, 2)) + ], + [ + poiBuilder("Marker 2") + .withMayOverlap(true) + .withPoiInfo(new PoiInfoBuilder().withMayOverlap(true)), + fadedOut(FADE_IN.length).concat( + fadeIn(FADE_2_CYCLES.length - FADE_IN.length) + ), + not(firstNFrames(FADE_2_CYCLES, FADE_IN.length)) + ] + ] + } + ], + frameTimes: FADE_2_CYCLES + }, + { + name: "Removed user poi fades out", + tiles: [ + { + userLabels: [ + [ + poiBuilder(), + FADE_IN.concat(fadedOut(FADE_2_CYCLES.length - FADE_IN.length)), + firstNFrames(FADE_2_CYCLES, FADE_IN.length) + ] + ] + } + ], + frameTimes: FADE_2_CYCLES + }, + { + name: "From two user pois in same tile, removed poi fades out, remaining poi is faded in", + tiles: [ + { + userLabels: [ + [ + poiBuilder("Marker 1") + .withMayOverlap(true) + .withPoiInfo(new PoiInfoBuilder().withMayOverlap(true)), + fadeIn(FADE_2_CYCLES.length) + ], + [ + poiBuilder("Marker 2") + .withMayOverlap(true) + .withPoiInfo(new PoiInfoBuilder().withMayOverlap(true)), + FADE_IN.concat(fadedOut(FADE_2_CYCLES.length - FADE_IN.length)), + firstNFrames(FADE_2_CYCLES, FADE_IN.length) + ] + ] + } + ], + frameTimes: FADE_2_CYCLES + }, // LABEL COLLISIONS { name: "Least prioritized from two colliding persistent point texts fades out", @@ -520,6 +608,25 @@ describe("TextElementsRenderer", function() { } }); + function buildLabels( + inputElements: InputTextElement[] | undefined, + elementFrameStates: Array<[TextElement, FadeState[]]>, + frameCount: number + ): TextElement[] { + if (!inputElements) { + return []; + } + return inputElements.map((inputElement: InputTextElement) => { + expect(frameStates(inputElement).length).equal(frameCount); + // Only used to identify some text elements for testing purposes. + const dummyUserData = {}; + const element = builder(inputElement) + .withUserData(dummyUserData) + .build(sandbox); + elementFrameStates.push([element, frameStates(inputElement)]); + return element; + }); + } async function initTest( test: TestCase ): Promise<{ @@ -541,18 +648,14 @@ describe("TextElementsRenderer", function() { expect(tile.terrainFrames.length).equal(test.frameTimes.length); enableElevation = true; } - const elements = tile.labels.map((inputElement: InputTextElement) => { - expect(frameStates(inputElement).length).equal(test.frameTimes.length); - // Only used to identify some text elements for testing purposes. - const dummyUserData = {}; - const element = builder(inputElement) - .withUserData(dummyUserData) - .build(sandbox); - elementFrameStates.push([element, frameStates(inputElement)]); - return element; - }); + const labels = buildLabels(tile.labels, elementFrameStates, test.frameTimes.length); + const userLabels = buildLabels( + tile.userLabels, + elementFrameStates, + test.frameTimes.length + ); allTileIndices.push(tileIndex); - fixture.addTile(elements); + fixture.addTile(labels, userLabels); }); // Keeps track of the opacity that text elements had in the previous frame. @@ -562,7 +665,6 @@ describe("TextElementsRenderer", function() { // to initialized. await fixture.renderFrame(INITIAL_TIME, allTileIndices, []); fixture.setElevationProvider(enableElevation); - return { elementFrameStates, prevOpacities }; } @@ -586,6 +688,19 @@ describe("TextElementsRenderer", function() { return { tileIdcs, terrainTileIdcs }; } + function prepareUserLabels(frameIdx: number, tiles: InputTile[], tileIndices: number[]) { + for (const tileIndex of tileIndices) { + const tile = tiles[tileIndex]; + if (!tile.userLabels) { + continue; + } + const elementsEnabled = tile.userLabels.map( + label => framesEnabled(label)?.[frameIdx] ?? true + ); + fixture.setTileUserTextElements(tileIndex, elementsEnabled); + } + } + for (const test of tests) { it(test.name, async function() { const { elementFrameStates, prevOpacities } = await initTest(test); @@ -595,6 +710,7 @@ describe("TextElementsRenderer", function() { const frameTime = test.frameTimes[frameIdx]; const collisionEnabled = test.collisionFrames === undefined ? true : test.collisionFrames[frameIdx]; + prepareUserLabels(frameIdx, test.tiles, tileIdcs); await fixture.renderFrame(frameTime, tileIdcs, terrainTileIdcs, collisionEnabled); let elementIdx = 0; diff --git a/@here/harp-mapview/test/TextElementsRendererTestFixture.ts b/@here/harp-mapview/test/TextElementsRendererTestFixture.ts index 619db181c6..32b272b369 100644 --- a/@here/harp-mapview/test/TextElementsRendererTestFixture.ts +++ b/@here/harp-mapview/test/TextElementsRendererTestFixture.ts @@ -114,6 +114,7 @@ export class TestFixture { private m_textRenderer: TextElementsRenderer | undefined; private m_defaultTile: Tile | undefined; private m_allTiles: Tile[] = []; + private m_allUserTextElements: TextElement[][] = []; constructor(readonly sandbox: sinon.SinonSandbox) { this.m_screenCollisions = new ScreenCollisions(); @@ -219,7 +220,7 @@ export class TestFixture { * can later be referenced by index when rendering a frame. See [[renderFrame]]. * @param elements The text elements the new tile will contain. */ - addTile(elements: TextElement[]) { + addTile(elements: TextElement[], userElements: TextElement[]) { const tile = this.m_allTiles.length > 0 ? this.m_dataSource.getTile( @@ -233,7 +234,29 @@ export class TestFixture { for (const element of elements) { tile.addTextElement(element); } + for (const element of userElements) { + tile.addUserTextElement(element); + } this.m_allTiles.push(tile); + this.m_allUserTextElements.push(userElements); + } + + setTileUserTextElements(tileIndex: number, elementEnabled: boolean[]) { + const tile = this.m_allTiles[tileIndex]; + const allElements = this.m_allUserTextElements[tileIndex]; + const tileElements = tile.userTextElements; + assert(allElements.length === elementEnabled.length); + + for (let i = 0; i < allElements.length; ++i) { + const element = allElements[i]; + const addElement = elementEnabled[i]; + const elementPresent = tileElements.elements.indexOf(element) !== -1; + if (addElement && !elementPresent) { + tile.addUserTextElement(element); + } else if (!addElement && elementPresent) { + tile.removeUserTextElement(element); + } + } } /** diff --git a/@here/harp-mapview/test/TextElementsRendererTestUtils.ts b/@here/harp-mapview/test/TextElementsRendererTestUtils.ts index 488a5c4af4..58cdc7d2a9 100644 --- a/@here/harp-mapview/test/TextElementsRendererTestUtils.ts +++ b/@here/harp-mapview/test/TextElementsRendererTestUtils.ts @@ -118,7 +118,9 @@ export function allFrames(frames: number[]): boolean[] { /** * Types to hold input data for TextElementsRenderer tests. */ -export type InputTextElement = [TextElementBuilder, FadeState[]]; +export type InputTextElement = + | [TextElementBuilder, FadeState[]] + | [TextElementBuilder, FadeState[], boolean[]]; export function builder(input: InputTextElement) { return input[0]; @@ -128,9 +130,14 @@ export function frameStates(input: InputTextElement) { return input[1]; } +export function framesEnabled(input: InputTextElement): boolean[] | undefined { + return input.length > 2 ? input[2] : undefined; +} + export interface InputTile { // Labels in the tile, including their builder and expected fade state per frame. - labels: InputTextElement[]; + labels?: InputTextElement[]; + userLabels?: InputTextElement[]; // Frames where tile will be visited (default: all) frames?: boolean[]; // Frames where corresponding terrain tile will be available (default: terrain disabled)