Skip to content

Commit ef81b07

Browse files
ibesoragithub-actions[bot]
ibesora
authored and
github-actions[bot]
committed
Fixes atlas cache returning another scope images
GitOrigin-RevId: 57019ad05ab4d439f0eb177513978f37183d2e26
1 parent 9d4de14 commit ef81b07

5 files changed

Lines changed: 138 additions & 30 deletions

File tree

‎src/render/atlas_content_descriptor.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ export type ImageVersionsMap = Map<string, number>;
6666
export class AtlasContentDescriptor {
6767
hash: number;
6868
requiresMipMaps: boolean;
69+
private scope: string;
6970

7071
private iconDescriptors: ImageDescriptor[];
7172
private patternDescriptors: ImageDescriptor[];
@@ -75,8 +76,10 @@ export class AtlasContentDescriptor {
7576
patterns: StyleImageMap<StringifiedImageVariant>,
7677
imageVersions: ImageVersionsMap,
7778
lut: LUT | null,
79+
scope: string = '',
7880
variantCache?: Map<StringifiedImageVariant, ImageVariant>
7981
) {
82+
this.scope = scope;
8083
this.iconDescriptors = [];
8184
this.patternDescriptors = [];
8285
// Mipmaps are required when patterns are present
@@ -94,6 +97,8 @@ export class AtlasContentDescriptor {
9497

9598
// Calculate combined hash
9699
let seed = 0;
100+
seed = combineHash(seed, hashCode(scope));
101+
seed = combineHash(seed, 1); // separator
97102
const lutData = lut ? lut.data : '';
98103
if (lutData) {
99104
seed = combineHash(seed, hashCode(lutData));
@@ -121,6 +126,8 @@ export class AtlasContentDescriptor {
121126
* Returns true if all images in this descriptor are also present in the other descriptor.
122127
*/
123128
subsetOf(other: AtlasContentDescriptor): boolean {
129+
if (this.scope !== other.scope) return false;
130+
124131
return (
125132
this.isSubsetArray(this.iconDescriptors, other.iconDescriptors, compareImageDescriptors) &&
126133
this.isSubsetArray(this.patternDescriptors, other.patternDescriptors, compareImageDescriptors)

‎src/render/image_atlas.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ export default class ImageAtlas {
192192
lut: LUT | null;
193193
contentDescriptor: AtlasContentDescriptor | null | undefined;
194194

195-
constructor(icons: StyleImageMap<StringifiedImageVariant>, patterns: StyleImageMap<StringifiedImageVariant>, lut: LUT | null, imageVersions?: ImageVersionsMap) {
195+
constructor(icons: StyleImageMap<StringifiedImageVariant>, patterns: StyleImageMap<StringifiedImageVariant>, lut: LUT | null, imageVersions?: ImageVersionsMap, scope: string = '') {
196196
const iconPositions: ImagePositionMap = new Map();
197197
const patternPositions: ImagePositionMap = new Map();
198198
this.haveRenderCallbacks = [];
@@ -259,7 +259,7 @@ export default class ImageAtlas {
259259

260260
// Create content descriptor if atlas caching is enabled
261261
if (enableAtlasCaching) {
262-
this.contentDescriptor = new AtlasContentDescriptor(sortedIcons, sortedPatterns, imageVersions, lut, variantCache);
262+
this.contentDescriptor = new AtlasContentDescriptor(sortedIcons, sortedPatterns, imageVersions, lut, scope, variantCache);
263263
}
264264
}
265265

‎src/source/worker_tile.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -620,7 +620,7 @@ class WorkerTile {
620620
const variantCache = new Map<StringifiedImageVariant, ImageVariant>();
621621
const sortedIcons = sortImagesMap(iconMap, variantCache);
622622
const sortedPatterns = sortImagesMap(patternMap, variantCache);
623-
const descriptor = new AtlasContentDescriptor(sortedIcons, sortedPatterns, imageVersions, this.lut, variantCache);
623+
const descriptor = new AtlasContentDescriptor(sortedIcons, sortedPatterns, imageVersions, this.lut, this.scope, variantCache);
624624

625625
actor.send('checkAtlasCache', {descriptor, scope: this.scope})
626626
.then((cachedPositions) => {
@@ -631,7 +631,7 @@ class WorkerTile {
631631
imageAtlasForTransfer = new ImageAtlasReference(cachedPositions.sourceHash);
632632
positions = cachedPositions;
633633
} else {
634-
const imageAtlas = new ImageAtlas(iconMap, patternMap, this.lut, imageVersions);
634+
const imageAtlas = new ImageAtlas(iconMap, patternMap, this.lut, imageVersions, this.scope);
635635
imageAtlasForTransfer = imageAtlas;
636636
positions = imageAtlas;
637637
}
@@ -640,7 +640,7 @@ class WorkerTile {
640640
})
641641
.catch((err: Error) => {
642642
if (err.name !== 'AbortError') warnOnce(`[Worker] Error checking atlas cache: ${err.message}`);
643-
const imageAtlas = new ImageAtlas(iconMap, patternMap, this.lut, imageVersions);
643+
const imageAtlas = new ImageAtlas(iconMap, patternMap, this.lut, imageVersions, this.scope);
644644
completeBucketProcessing(imageAtlas, imageAtlas);
645645
});
646646
} else {

‎test/unit/render/image_atlas.test.ts‎

Lines changed: 76 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ describe('AtlasContentDescriptor', () => {
119119
const patterns: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('pattern'), createMockImage('pattern')]]);
120120
const versions: Map<string, number> = new Map([['icon', 1], ['pattern', 1]]);
121121

122-
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null);
122+
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null, '');
123123

124124
expect(descriptor.hash).toBeDefined();
125125
expect(typeof descriptor.hash).toEqual('number');
@@ -130,8 +130,8 @@ describe('AtlasContentDescriptor', () => {
130130
const patterns: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('pattern'), createMockImage('pattern')]]);
131131
const versions: Map<string, number> = new Map([['icon', 1], ['pattern', 1]]);
132132

133-
const descriptor1 = new AtlasContentDescriptor(icons, patterns, versions, null);
134-
const descriptor2 = new AtlasContentDescriptor(icons, patterns, versions, null);
133+
const descriptor1 = new AtlasContentDescriptor(icons, patterns, versions, null, '');
134+
const descriptor2 = new AtlasContentDescriptor(icons, patterns, versions, null, '');
135135

136136
expect(descriptor1.hash).toEqual(descriptor2.hash);
137137
});
@@ -142,8 +142,8 @@ describe('AtlasContentDescriptor', () => {
142142
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
143143
const versions: Map<string, number> = new Map([['icon1', 1], ['icon2', 1]]);
144144

145-
const descriptor1 = new AtlasContentDescriptor(icons1, patterns, versions, null);
146-
const descriptor2 = new AtlasContentDescriptor(icons2, patterns, versions, null);
145+
const descriptor1 = new AtlasContentDescriptor(icons1, patterns, versions, null, '');
146+
const descriptor2 = new AtlasContentDescriptor(icons2, patterns, versions, null, '');
147147

148148
expect(descriptor1.hash).not.toEqual(descriptor2.hash);
149149
});
@@ -154,18 +154,44 @@ describe('AtlasContentDescriptor', () => {
154154
const versions1: Map<string, number> = new Map([['icon', 1]]);
155155
const versions2: Map<string, number> = new Map([['icon', 2]]);
156156

157-
const descriptor1 = new AtlasContentDescriptor(icons, patterns, versions1, null);
158-
const descriptor2 = new AtlasContentDescriptor(icons, patterns, versions2, null);
157+
const descriptor1 = new AtlasContentDescriptor(icons, patterns, versions1, null, '');
158+
const descriptor2 = new AtlasContentDescriptor(icons, patterns, versions2, null, '');
159159

160160
expect(descriptor1.hash).not.toEqual(descriptor2.hash);
161161
});
162162

163+
test('different scopes produce different hash for identical id/version/scale (GLJS-1882)', () => {
164+
// Images and their versions are namespaced per style scope (ImageManager), so
165+
// two unrelated scopes (e.g. distinct imported styles) can each independently
166+
// hold an image with the same id at the same version but different pixels.
167+
// The scope must be part of the hash or the two collide.
168+
const icons: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('shared-icon'), createMockImage('shared-icon')]]);
169+
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
170+
const versions: Map<string, number> = new Map([['shared-icon', 1]]);
171+
172+
const privateDescriptor = new AtlasContentDescriptor(icons, patterns, versions, null, 'private');
173+
const publicDescriptor = new AtlasContentDescriptor(icons, patterns, versions, null, 'public');
174+
175+
expect(privateDescriptor.hash).not.toEqual(publicDescriptor.hash);
176+
});
177+
178+
test('subsetOf returns false across different scopes even when content would otherwise be a subset (GLJS-1882)', () => {
179+
const icons: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('shared-icon'), createMockImage('shared-icon')]]);
180+
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
181+
const versions: Map<string, number> = new Map([['shared-icon', 1]]);
182+
183+
const privateDescriptor = new AtlasContentDescriptor(icons, patterns, versions, null, 'private');
184+
const publicDescriptor = new AtlasContentDescriptor(icons, patterns, versions, null, 'public');
185+
186+
expect(publicDescriptor.subsetOf(privateDescriptor)).toBe(false);
187+
});
188+
163189
test('sets requiresMipMaps when patterns are present', () => {
164190
const icons: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('icon'), createMockImage('icon')]]);
165191
const patterns: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('pattern'), createMockImage('pattern')]]);
166192
const versions: Map<string, number> = new Map([['icon', 1], ['pattern', 1]]);
167193

168-
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null);
194+
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null, '');
169195

170196
expect(descriptor.requiresMipMaps).toBe(true);
171197
});
@@ -175,7 +201,7 @@ describe('AtlasContentDescriptor', () => {
175201
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
176202
const versions: Map<string, number> = new Map([['icon', 1]]);
177203

178-
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null);
204+
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null, '');
179205

180206
expect(descriptor.requiresMipMaps).toBe(false);
181207
});
@@ -189,8 +215,8 @@ describe('AtlasContentDescriptor', () => {
189215
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
190216
const versions: Map<string, number> = new Map([['icon1', 1], ['icon2', 1]]);
191217

192-
const descriptor1 = new AtlasContentDescriptor(icons1, patterns, versions, null);
193-
const descriptor2 = new AtlasContentDescriptor(icons2, patterns, versions, null);
218+
const descriptor1 = new AtlasContentDescriptor(icons1, patterns, versions, null, '');
219+
const descriptor2 = new AtlasContentDescriptor(icons2, patterns, versions, null, '');
194220

195221
expect(descriptor1.subsetOf(descriptor2)).toBe(true);
196222
});
@@ -201,8 +227,8 @@ describe('AtlasContentDescriptor', () => {
201227
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
202228
const versions: Map<string, number> = new Map([['icon1', 1], ['icon2', 1]]);
203229

204-
const descriptor1 = new AtlasContentDescriptor(icons1, patterns, versions, null);
205-
const descriptor2 = new AtlasContentDescriptor(icons2, patterns, versions, null);
230+
const descriptor1 = new AtlasContentDescriptor(icons1, patterns, versions, null, '');
231+
const descriptor2 = new AtlasContentDescriptor(icons2, patterns, versions, null, '');
206232

207233
expect(descriptor1.subsetOf(descriptor2)).toBe(false);
208234
});
@@ -216,8 +242,8 @@ describe('AtlasContentDescriptor', () => {
216242
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
217243
const versions: Map<string, number> = new Map([['icon1', 1], ['icon2', 1]]);
218244

219-
const descriptor1 = new AtlasContentDescriptor(icons1, patterns, versions, null);
220-
const descriptor2 = new AtlasContentDescriptor(icons2, patterns, versions, null);
245+
const descriptor1 = new AtlasContentDescriptor(icons1, patterns, versions, null, '');
246+
const descriptor2 = new AtlasContentDescriptor(icons2, patterns, versions, null, '');
221247

222248
// Should return false immediately without iterating
223249
expect(descriptor1.subsetOf(descriptor2)).toBe(false);
@@ -235,7 +261,7 @@ describe('ImageAtlasCache', () => {
235261
const icons: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('icon'), createMockImage('icon')]]);
236262
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
237263
const versions: Map<string, number> = new Map([['icon', 1]]);
238-
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null);
264+
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null, '');
239265

240266
const result = cache.findCachedAtlas(descriptor);
241267

@@ -252,7 +278,7 @@ describe('ImageAtlasCache', () => {
252278
cache.getOrCache(atlas);
253279

254280
// Try to find it with same descriptor
255-
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null);
281+
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null, '');
256282
const result = cache.findCachedAtlas(descriptor);
257283

258284
expect(result).toBe(atlas);
@@ -273,12 +299,37 @@ describe('ImageAtlasCache', () => {
273299

274300
// Try to find it with subset descriptor (only icon1)
275301
const subsetIcons: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('icon1'), createMockImage('icon1')]]);
276-
const descriptor = new AtlasContentDescriptor(subsetIcons, patterns, versions, null);
302+
const descriptor = new AtlasContentDescriptor(subsetIcons, patterns, versions, null, '');
277303
const result = cache.findCachedAtlas(descriptor);
278304

279305
expect(result).toBe(atlas);
280306
});
281307

308+
test('findCachedAtlas does not return another scope\'s atlas via subset match (GLJS-1882)', () => {
309+
// The subset-match scan in findCachedAtlas iterates the whole cache regardless
310+
// of hash, so scope must also be checked in subsetOf, not just baked into the
311+
// hash used for the exact-match lookup.
312+
const icons: StyleImageMap<StringifiedImageVariant> = new Map([
313+
[createImageVariantId('icon1'), createMockImage('icon1')],
314+
[createImageVariantId('icon2'), createMockImage('icon2')]
315+
]);
316+
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
317+
const versions: Map<string, number> = new Map([['icon1', 1], ['icon2', 1]]);
318+
319+
// Cache an atlas belonging to the "private" scope containing icon1 and icon2.
320+
const privateAtlas = new ImageAtlas(icons, patterns, null, versions, 'private');
321+
cache.getOrCache(privateAtlas);
322+
323+
// A "public" scope tile requests only icon1, with the same id/version/scale.
324+
// Content-wise this looks like a subset of the private atlas, but the two
325+
// scopes are unrelated and must never share cached atlases.
326+
const subsetIcons: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('icon1'), createMockImage('icon1')]]);
327+
const descriptor = new AtlasContentDescriptor(subsetIcons, patterns, versions, null, 'public');
328+
const result = cache.findCachedAtlas(descriptor);
329+
330+
expect(result).toBeUndefined();
331+
});
332+
282333
test('getOrCache returns same atlas for duplicate content', () => {
283334
const icons: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('icon'), createMockImage('icon')]]);
284335
const patterns: StyleImageMap<StringifiedImageVariant> = new Map();
@@ -327,7 +378,7 @@ describe('ImageAtlasCache', () => {
327378
cache.getOrCache(atlas);
328379

329380
// Request without patterns (no mipmaps needed)
330-
const descriptor = new AtlasContentDescriptor(icons, new Map(), versions, null);
381+
const descriptor = new AtlasContentDescriptor(icons, new Map(), versions, null, '');
331382
const result = cache.findCachedAtlas(descriptor);
332383

333384
expect(result).toBe(atlas); // Should find it despite mipmap difference
@@ -344,7 +395,7 @@ describe('ImageAtlasCache', () => {
344395
// Request with patterns (mipmaps needed)
345396
const patternsNeeded: StyleImageMap<StringifiedImageVariant> = new Map([[createImageVariantId('pattern'), createMockImage('pattern')]]);
346397
const versionsWithPattern: Map<string, number> = new Map([['icon', 1], ['pattern', 1]]);
347-
const descriptor = new AtlasContentDescriptor(icons, patternsNeeded, versionsWithPattern, null);
398+
const descriptor = new AtlasContentDescriptor(icons, patternsNeeded, versionsWithPattern, null, '');
348399
const result = cache.findCachedAtlas(descriptor);
349400

350401
expect(result).toBeUndefined(); // Should NOT find it
@@ -389,7 +440,7 @@ describe('ImageAtlasCache', () => {
389440
expect(newTexture).not.toBe(texture);
390441

391442
// Atlas should not be in cache anymore
392-
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null);
443+
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null, '');
393444
const cachedAtlas = cache.findCachedAtlas(descriptor);
394445
expect(cachedAtlas).toBeUndefined();
395446
});
@@ -414,7 +465,7 @@ describe('ImageAtlasCache', () => {
414465
cache.getOrCache(oldAtlas);
415466

416467
// New tile requests the same image name but with new (grey) params
417-
const newDescriptor = new AtlasContentDescriptor(newIcons, patterns, versions, null);
468+
const newDescriptor = new AtlasContentDescriptor(newIcons, patterns, versions, null, '');
418469
const result = cache.findCachedAtlas(newDescriptor);
419470

420471
expect(result).toBeUndefined();
@@ -432,7 +483,7 @@ describe('ImageAtlasCache', () => {
432483
cache.destroyTextures();
433484

434485
// Atlas should still be in cache
435-
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null);
486+
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null, '');
436487
const cachedAtlas = cache.findCachedAtlas(descriptor);
437488
expect(cachedAtlas).toBe(atlas);
438489

@@ -589,7 +640,7 @@ describe('ImageAtlasCache - version invalidation', () => {
589640
cache.getOrCache(atlas1);
590641

591642
// Try to find with different version
592-
const descriptor = new AtlasContentDescriptor(icons, patterns, versions2, null);
643+
const descriptor = new AtlasContentDescriptor(icons, patterns, versions2, null, '');
593644
const result = cache.findCachedAtlas(descriptor);
594645

595646
expect(result).toBeUndefined();
@@ -606,7 +657,7 @@ describe('ImageAtlasCache - version invalidation', () => {
606657
cache.getOrCache(atlas);
607658

608659
// Create new atlas with same version (shouldn't happen in practice, but tests cache behavior)
609-
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null);
660+
const descriptor = new AtlasContentDescriptor(icons, patterns, versions, null, '');
610661
const result = cache.findCachedAtlas(descriptor);
611662

612663
expect(result).toBe(atlas);

0 commit comments

Comments
 (0)