Merge e04df47fc6 into ee2b3813b2

fix(textureatlas): Apply internal padding between images

CHANGES.md

@@ -7,7 +7,8 @@
 #### Fixes :wrench:
 
 - Billboards using `imageSubRegion` now render as expected. [#12585](https://github.com/CesiumGS/cesium/issues/12585)
-- Improved scaling of SVGs in billboards [#TODO](https://github.com/CesiumGS/cesium/issues/TODO)
+- Improved scaling of SVGs in billboards [#13020](https://github.com/CesiumGS/cesium/issues/13020)
+- Fixed unexpected outline artifacts around billboards [#13038](https://github.com/CesiumGS/cesium/issues/13038)
 
 #### Additions :tada:
 

packages/engine/Source/Core/TexturePacker.js

@@ -122,6 +122,8 @@ TexturePacker.prototype._findNode = function (node, { width, height }) {
       return node;
     }
 
+    const borderPadding = this._borderPadding;
+
     // Vertical split (childNode1 = left half, childNode2 = right half).
     if (widthDifference > heightDifference) {
       node.childNode1 = new TextureNode({
@@ -130,12 +132,18 @@ TexturePacker.prototype._findNode = function (node, { width, height }) {
         width,
         height: nodeHeight,
       });
-      node.childNode2 = new TextureNode({
-        x: rectangle.x + width,
-        y: rectangle.y,
-        width: widthDifference,
-        height: nodeHeight,
-      });
+
+      // Apply padding only along the vertical "cut".
+      const widthDifferencePadded = widthDifference - borderPadding;
+
+      if (widthDifferencePadded > 0) {
+        node.childNode2 = new TextureNode({
+          x: rectangle.x + width + borderPadding,
+          y: rectangle.y,
+          width: widthDifferencePadded,
+          height: nodeHeight,
+        });
+      }
 
       return this._findNode(node.childNode1, { width, height });
     }
@@ -147,12 +155,19 @@ TexturePacker.prototype._findNode = function (node, { width, height }) {
       width: nodeWidth,
       height,
     });
-    node.childNode2 = new TextureNode({
-      x: rectangle.x,
-      y: rectangle.y + height,
-      width: nodeWidth,
-      height: heightDifference,
-    });
+
+    // Apply padding only along the horizontal "cut".
+    const heightDifferencePadded = heightDifference - borderPadding;
+
+    if (heightDifferencePadded > 0) {
+      node.childNode2 = new TextureNode({
+        x: rectangle.x,
+        y: rectangle.y + height + borderPadding,
+        width: nodeWidth,
+        height: heightDifferencePadded,
+      });
+    }
+
     return this._findNode(node.childNode1, { width, height });
   }
 

packages/engine/Source/DataSources/StaticGeometryColorBatch.js

@@ -47,23 +47,8 @@ function Batch(
   this.subscriptions = new AssociativeArray();
   this.showsUpdated = new AssociativeArray();
   this.itemsToRemove = [];
-  this.invalidated = false;
-
-  let removeMaterialSubscription;
-  if (defined(depthFailMaterialProperty)) {
-    removeMaterialSubscription =
-      depthFailMaterialProperty.definitionChanged.addEventListener(
-        Batch.prototype.onMaterialChanged,
-        this,
-      );
-  }
-  this.removeMaterialSubscription = removeMaterialSubscription;
 }
 
-Batch.prototype.onMaterialChanged = function () {
-  this.invalidated = true;
-};
-
 Batch.prototype.isMaterial = function (updater) {
   const material = this.depthFailMaterialProperty;
   const updaterMaterial = updater.depthFailMaterialProperty;
@@ -391,9 +376,6 @@ Batch.prototype.destroy = function () {
   if (defined(oldPrimitive)) {
     primitives.remove(oldPrimitive);
   }
-  if (defined(this.removeMaterialSubscription)) {
-    this.removeMaterialSubscription();
-  }
 };
 
 /**
@@ -451,12 +433,9 @@ StaticGeometryColorBatch.prototype.add = function (time, updater) {
 function removeItem(items, updater) {
   const length = items.length;
   for (let i = length - 1; i >= 0; i--) {
-    const item = items[i];
-    if (item.remove(updater)) {
-      if (item.updaters.length === 0) {
-        items.splice(i, 1);
-        item.destroy();
-      }
+    if (items[i].remove(updater)) {
+      // If the item is now empty, delete it (deferred until the next update,
+      // in case a new updater is added to the same item first).
       return true;
     }
   }
@@ -489,26 +468,18 @@ function moveItems(batch, items, time) {
 }
 
 function updateItems(batch, items, time, isUpdated) {
-  let length = items.length;
-  let i;
-  for (i = length - 1; i >= 0; i--) {
+  for (let i = items.length - 1; i >= 0; i--) {
     const item = items[i];
-    if (item.invalidated) {
+    if (item.updaters.length === 0) {
       items.splice(i, 1);
-      const updaters = item.updaters.values;
-      const updatersLength = updaters.length;
-      for (let h = 0; h < updatersLength; h++) {
-        batch.add(time, updaters[h]);
-      }
       item.destroy();
     }
   }
 
-  length = items.length;
-  for (i = 0; i < length; ++i) {
-    isUpdated = items[i].update(time) && isUpdated;
-  }
-  return isUpdated;
+  return items.reduce(
+    (isUpdated, item) => isUpdated && item.update(time),
+    isUpdated,
+  );
 }
 
 StaticGeometryColorBatch.prototype.update = function (time) {

packages/engine/Source/DataSources/StaticGeometryPerMaterialBatch.js

@@ -43,20 +43,10 @@ function Batch(
   this.depthFailMaterial = undefined;
   this.updatersWithAttributes = new AssociativeArray();
   this.attributes = new AssociativeArray();
-  this.invalidated = false;
-  this.removeMaterialSubscription =
-    materialProperty.definitionChanged.addEventListener(
-      Batch.prototype.onMaterialChanged,
-      this,
-    );
   this.subscriptions = new AssociativeArray();
   this.showsUpdated = new AssociativeArray();
 }
 
-Batch.prototype.onMaterialChanged = function () {
-  this.invalidated = true;
-};
-
 Batch.prototype.isMaterial = function (updater) {
   const material = this.materialProperty;
   const updaterMaterial = updater.fillMaterialProperty;
@@ -378,7 +368,6 @@ Batch.prototype.destroy = function () {
   if (defined(oldPrimitive)) {
     primitives.remove(oldPrimitive);
   }
-  this.removeMaterialSubscription();
 };
 
 /**
@@ -426,40 +415,30 @@ StaticGeometryPerMaterialBatch.prototype.remove = function (updater) {
   const items = this._items;
   const length = items.length;
   for (let i = length - 1; i >= 0; i--) {
-    const item = items[i];
-    if (item.remove(updater)) {
-      if (item.updaters.length === 0) {
-        items.splice(i, 1);
-        item.destroy();
-      }
+    if (items[i].remove(updater)) {
+      // If the item is now empty, delete it (deferred until the next update,
+      // in case a new updater is added to the same item first).
       break;
     }
   }
 };
 
 StaticGeometryPerMaterialBatch.prototype.update = function (time) {
-  let i;
   const items = this._items;
   const length = items.length;
 
-  for (i = length - 1; i >= 0; i--) {
+  for (let i = length - 1; i >= 0; i--) {
     const item = items[i];
-    if (item.invalidated) {
+    if (item.updaters.length === 0) {
       items.splice(i, 1);
-      const updaters = item.updaters.values;
-      const updatersLength = updaters.length;
-      for (let h = 0; h < updatersLength; h++) {
-        this.add(time, updaters[h]);
-      }
       item.destroy();
     }
   }
 
-  let isUpdated = true;
-  for (i = 0; i < items.length; i++) {
-    isUpdated = items[i].update(time) && isUpdated;
-  }
-  return isUpdated;
+  return items.reduce(
+    (isUpdated, item) => isUpdated && item.update(time),
+    false,
+  );
 };
 
 StaticGeometryPerMaterialBatch.prototype.getBoundingSphere = function (

packages/engine/Source/DataSources/StaticGroundPolylinePerMaterialBatch.js

@@ -44,12 +44,6 @@ function Batch(
   this.material = undefined;
   this.updatersWithAttributes = new AssociativeArray();
   this.attributes = new AssociativeArray();
-  this.invalidated = false;
-  this.removeMaterialSubscription =
-    materialProperty.definitionChanged.addEventListener(
-      Batch.prototype.onMaterialChanged,
-      this,
-    );
   this.subscriptions = new AssociativeArray();
   this.showsUpdated = new AssociativeArray();
   this.zIndex = zIndex;
@@ -57,10 +51,6 @@ function Batch(
   this._asynchronous = asynchronous;
 }
 
-Batch.prototype.onMaterialChanged = function () {
-  this.invalidated = true;
-};
-
 // Check if the given updater's material is compatible with this batch
 Batch.prototype.isMaterial = function (updater) {
   const material = this.materialProperty;
@@ -325,7 +315,6 @@ Batch.prototype.destroy = function () {
   if (defined(oldPrimitive)) {
     orderedGroundPrimitives.remove(oldPrimitive);
   }
-  this.removeMaterialSubscription();
 };
 
 /**
@@ -371,12 +360,9 @@ StaticGroundPolylinePerMaterialBatch.prototype.remove = function (updater) {
   const items = this._items;
   const length = items.length;
   for (let i = length - 1; i >= 0; i--) {
-    const item = items[i];
-    if (item.remove(updater)) {
-      if (item.updaters.length === 0) {
-        items.splice(i, 1);
-        item.destroy();
-      }
+    if (items[i].remove(updater)) {
+      // If the item is now empty, delete it (deferred until the next update,
+      // in case a new updater is added to the same item first).
       break;
     }
   }
@@ -389,22 +375,16 @@ StaticGroundPolylinePerMaterialBatch.prototype.update = function (time) {
 
   for (i = length - 1; i >= 0; i--) {
     const item = items[i];
-    if (item.invalidated) {
+    if (item.updaters.length === 0) {
       items.splice(i, 1);
-      const updaters = item.updaters.values;
-      const updatersLength = updaters.length;
-      for (let h = 0; h < updatersLength; h++) {
-        this.add(time, updaters[h]);
-      }
       item.destroy();
     }
   }
 
-  let isUpdated = true;
-  for (i = 0; i < items.length; i++) {
-    isUpdated = items[i].update(time) && isUpdated;
-  }
-  return isUpdated;
+  return items.reduce(
+    (isUpdated, item) => isUpdated && item.update(time),
+    false,
+  );
 };
 
 StaticGroundPolylinePerMaterialBatch.prototype.getBoundingSphere = function (

packages/engine/Specs/Renderer/TextureAtlasSpec.js

@@ -852,16 +852,16 @@ describe("Scene/TextureAtlas", function () {
 .2222222222222222...............
 .2222222222222222...............
 .2222222222222222...............
-.22222222222222223333333333.....
-.22222222222222223333333333.....
-.22222222222222223333333333.....
-.22222222222222223333333333.....
-.22222222222222223333333333.....
-.22222222222222223333333333.....
-.22222222222222223333333333.....
-.22222222222222223333333333.....
-.22222222222222223333333333.....
-.2222222222222222333333333301...
+.2222222222222222.3333333333....
+.2222222222222222.3333333333....
+.2222222222222222.3333333333....
+.2222222222222222.3333333333....
+.2222222222222222.3333333333....
+.2222222222222222.3333333333....
+.2222222222222222.3333333333....
+.2222222222222222.3333333333.1..
+.2222222222222222.3333333333....
+.2222222222222222.3333333333.0..
 ................................
           `.trim(),
         );
@@ -926,9 +926,9 @@ describe("Scene/TextureAtlas", function () {
 .2222222222...
 .2222222222...
 .2222222222...
+.2222222222.1.
 .2222222222...
-.2222222222...
-.222222222201.
+.2222222222.0.
 ..............
           `.trim(),
         );
@@ -976,16 +976,16 @@ describe("Scene/TextureAtlas", function () {
 .3333333333333333...............
 .3333333333333333...............
 .3333333333333333...............
-.33333333333333332222222222.....
-.33333333333333332222222222.....
-.33333333333333332222222222.....
-.33333333333333332222222222.....
-.33333333333333332222222222.....
-.33333333333333332222222222.....
-.33333333333333332222222222.....
-.33333333333333332222222222.....
-.33333333333333332222222222.....
-.3333333333333333222222222201...
+.3333333333333333.2222222222....
+.3333333333333333.2222222222....
+.3333333333333333.2222222222....
+.3333333333333333.2222222222....
+.3333333333333333.2222222222....
+.3333333333333333.2222222222....
+.3333333333333333.2222222222....
+.3333333333333333.2222222222.1..
+.3333333333333333.2222222222....
+.3333333333333333.2222222222.0..
 ................................
           `.trim(),
         );
@@ -1337,6 +1337,108 @@ describe("Scene/TextureAtlas", function () {
         ).contextToRender([0, 255, 0, 255]);
       });
 
+      it("adds custom padding with borderWidthInPixels", async function () {
+        atlas = new TextureAtlas({ borderWidthInPixels: 0 });
+        let indices = await addImages();
+
+        expect(drawAtlas(atlas, indices)).toBe(
+          `
+................
+................
+................
+................
+................
+................
+2222222222......
+2222222222......
+2222222222......
+2222222222......
+2222222222......
+2222222222......
+22222222220.....
+22222222220.....
+22222222220.....
+222222222201....
+          `.trim(),
+        );
+
+        atlas = new TextureAtlas({ borderWidthInPixels: 2 });
+        indices = await addImages();
+
+        expect(drawAtlas(atlas, indices)).toBe(
+          `
+................................
+................................
+................................
+................................
+..2222222222....................
+..2222222222....................
+..2222222222....................
+..2222222222..1.................
+..2222222222....................
+..2222222222....................
+..2222222222..0.................
+..2222222222..0.................
+..2222222222..0.................
+..2222222222..0.................
+................................
+................................
+          `.trim(),
+        );
+
+        atlas = new TextureAtlas({ borderWidthInPixels: 5 });
+        indices = await addImages();
+
+        expect(drawAtlas(atlas, indices)).toBe(
+          `
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+................................
+.....2222222222.................
+.....2222222222.................
+.....2222222222.................
+.....2222222222.................
+.....2222222222.................
+.....2222222222.................
+.....2222222222.....0...........
+.....2222222222.....0...........
+.....2222222222.....0...........
+.....2222222222.....0.....1.....
+................................
+................................
+................................
+................................
+................................
+          `.trim(),
+        );
+
+        async function addImages() {
+          const promise = Promise.all([
+            atlas.addImage(tallGreenGuid, tallGreenImage),
+            atlas.addImage(blueGuid, blueImage),
+            atlas.addImage(bigBlueGuid, bigBlueImage),
+          ]);
+
+          return pollWhilePromise(promise, () => {
+            atlas.update(scene.frameState.context);
+          });
+        }
+      });
+
       it("GUID changes when atlas texure is modified", async function () {
         atlas = new TextureAtlas();