Model scope fixes (internal-8073)

GitOrigin-RevId: 1cd6807807f3e72e33590ec863e4f3b0b9b02ed9

Author: ibesora

3d-style/render/draw_model.ts

@@ -39,7 +39,6 @@ import type VertexBuffer from '../../src/gl/vertex_buffer';
 import type {CutoffParams} from '../../src/render/cutoff';
 import type {LUT} from "../../src/util/lut";
 import type {ModelUniformsType, ModelDepthUniformsType} from '../render/program/model_program';
-import type {Source} from '../../src/source/source';
 
 export default drawModels;
 
@@ -261,23 +260,19 @@ function drawMesh(sortedMesh: SortedMesh, painter: Painter, layer: ModelStyleLay
             undefined, dynamicBuffers);
 }
 
-function getScope(source: Source, layer: ModelStyleLayer, importedAsBasemap: boolean) {
-    // When using geojson layers, models are always set in the root scope to avoid model duplication
-    // But when the root style is defined as a fragment or has an schema, we use the well-known "basemap"
-    // scope to load the style so models will be set there.
-    return source.type === 'vector' ? layer.scope : importedAsBasemap ? "basemap" : "";
+function getScope(painter: Painter, layer: ModelStyleLayer) {
+    return painter.style._importedAsBasemap ? "basemap" : layer.scope;
 }
 
 export function prepare(layer: ModelStyleLayer, sourceCache: SourceCache, painter: Painter) {
     const modelSource = sourceCache.getSource();
     if (!modelSource.loaded()) return;
 
     if (modelSource.type === 'vector' || modelSource.type === 'geojson') {
-        const scope = getScope(modelSource, layer, painter.style._importedAsBasemap);
         if (painter.modelManager) {
             // Do it here, to prevent modelManager handling in Painter.
             // geojson models are always set in the root scope to avoid model duplication
-            painter.modelManager.upload(painter, scope);
+            painter.modelManager.upload(painter, getScope(painter, layer));
         }
         return;
     }
@@ -448,8 +443,7 @@ function drawModels(painter: Painter, sourceCache: SourceCache, layer: ModelStyl
     }
 
     if (modelSource.type === 'vector' || modelSource.type === 'geojson') {
-        const scope = getScope(modelSource, layer, painter.style._importedAsBasemap);
-        drawVectorLayerModels(painter, sourceCache, layer, coords, scope);
+        drawVectorLayerModels(painter, sourceCache, layer, coords, getScope(painter, layer));
         cleanup();
         return;
     }

src/style/style.ts

@@ -2044,8 +2044,12 @@ class Style extends Evented<MapEvents> {
         return this._availableImages.slice();
     }
 
+    getActualScope() {
+        return this._importedAsBasemap ? "basemap" : this.scope;
+    }
+
     addModelURLs(models: ModelsSpecification): this {
-        this.modelManager.addModelURLs(models, this.scope);
+        this.modelManager.addModelURLs(models, this.getActualScope());
         this._updateWorkerModels();
         this.fire(new Event('data', {dataType: 'style'}));
         return this;
@@ -2055,13 +2059,13 @@ class Style extends Evented<MapEvents> {
         this._checkLoaded();
         if (this._validate(validateModel, `models.${id}`, url, null, options)) return this;
 
-        this.modelManager.addModel(id, url, this.scope);
+        this.modelManager.addModel(id, url, this.getActualScope());
         this.fire(new Event('data', {dataType: 'style'}));
         return this;
     }
 
     hasModel(id: string): boolean {
-        return this.modelManager.hasModel(id, this.scope);
+        return this.modelManager.hasModel(id, this.getActualScope());
     }
 
     removeModel(id: string): this {
@@ -2070,14 +2074,14 @@ class Style extends Evented<MapEvents> {
         }
         const keepModelURI = false;
         const forceRemoval = true;
-        this.modelManager.removeModel(id, this.scope, keepModelURI, forceRemoval);
+        this.modelManager.removeModel(id, this.getActualScope(), keepModelURI, forceRemoval);
         this.fire(new Event('data', {dataType: 'style'}));
         return this;
     }
 
     listModels(): Array<string> {
         this._checkLoaded();
-        return this.modelManager.listModels(this.scope);
+        return this.modelManager.listModels(this.getActualScope());
     }
 
     addSource(id: string, source: (SourceSpecification | CustomSourceInterface<unknown>) & {collectResourceTiming?: boolean}, options: StyleSetterOptions = {}): void {

test/integration/render-tests/model-layer/geojson-source-with-schema-add-layer/expected.png

@@ -0,0 +1,54 @@
+{
+  "version": 8,
+  "metadata": {
+    "test": {
+      "height": 512,
+      "allowed": 0.0015,
+      "operations": [
+        ["addSource", 
+        "trees", 
+        {
+            "type": "geojson",
+            "data": "local://data/trees.geojson"
+        }],
+        ["addLayer", {
+            "id": "tree-layer-diffuse",
+            "type": "model",
+            "source": "trees",
+            "layout": {
+                "model-id": "tree-diffuse"
+            },
+            "paint": {
+                "model-scale": [ 70.0, 50.0, 50.0],
+                "model-translation": [0, 0, 100]
+            }
+            }
+        ],
+        ["addModel", "tree-diffuse", "local://models/tree-no-material.glb"],
+        ["wait"]
+      ]
+    }
+  },
+  "sources": {
+  },
+  "transition": {
+    "duration": 0
+  },
+  "models": {
+  },
+  "pitch": 60,
+  "bearing": 0,
+  "zoom": 15,
+  "schema": {},
+  "center": [
+    -122.40784,
+    37.78432
+  ],
+  "light": {
+    "intensity": 1,
+    "position": [1, 110, 90],
+    "anchor": "map"
+  },
+  "layers": [
+  ]
+}

test/integration/render-tests/model-layer/geojson-source-with-schema-add-layer/style.json

@@ -78,7 +78,7 @@ describe('ModelManager', () => {
     });
 
     // eslint-disable-next-line @typescript-eslint/require-await
-    test("#addModel with different ids but with same URL increases it's number of references but doesn't load it again", async () => {
+    test("#addModel with different ids but with same URL and scope increases it's number of references but doesn't load it again", async () => {
         const {modelManager, eventedParent} = createModelManager();
 
         eventedParent.on('error', ({error}) => {
@@ -96,6 +96,25 @@ describe('ModelManager', () => {
         expect(modelManager.models['basemap']['model'].numReferences).toBe(2);
     });
 
+    // eslint-disable-next-line @typescript-eslint/require-await
+    test("#addModel with different ids and scope but with same URL increases it's number of references but doesn't load it again", async () => {
+        const {modelManager, eventedParent} = createModelManager();
+
+        eventedParent.on('error', ({error}) => {
+            // eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-member-access
+            expect.unreachable(error.message);
+        });
+
+        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        modelManager.loadModel = vi.fn((id, url) => Promise.resolve({id, url}));
+
+        modelManager.addModel('model', 'https://www.example.com/', 'basemap');
+        modelManager.addModel('model2', 'https://www.example.com/', '');
+
+        expect(modelManager.loadModel).toHaveBeenCalledOnce();
+        expect(modelManager.models['basemap']['model'].numReferences).toBe(2);
+    });
+
     // eslint-disable-next-line @typescript-eslint/require-await
     test('#removeModel', async () => {
         const {modelManager, eventedParent} = createModelManager();
@@ -188,6 +207,28 @@ describe('ModelManager', () => {
         modelManager.addModelsFromBucket(['https://www.example.com/1', 'https://www.example.com/2', 'https://www.example.com/3'], 'basemap');
     });
 
+    // eslint-disable-next-line @typescript-eslint/require-await
+    test('#addModelsFromBucket doesn\t reload existing models', async () => {
+        const {modelManager, eventedParent} = createModelManager();
+
+        eventedParent.on('error', ({error}) => {
+            // eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-member-access
+            expect.unreachable(error.message);
+        });
+
+        vi.spyOn(modelManager, 'loadModel').mockImplementation(
+            (id, url) => url.startsWith('https://www.example.com/') ? Promise.resolve({id, url}) : Promise.reject(new Error('Not found'))
+        );
+
+        eventedParent.once('data', () => {
+            modelManager.addModelsFromBucket(['https://www.example.com/1'], 'basemap');
+            expect(modelManager.models['basemap']['https://www.example.com/1'].numReferences).toBe(2);
+        });
+
+        modelManager.addModelsFromBucket(['https://www.example.com/1', 'https://www.example.com/2', 'https://www.example.com/3'], 'basemap');
+
+    });
+
     test('#reloadModels', () => {
         const {modelManager, eventedParent} = createModelManager();