Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 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
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

- Improved performance of `scene.drillPick`. [#12916](https://github.com/CesiumGS/cesium/pull/12916)
- Improved performance when removing primitives. [#3018](https://github.com/CesiumGS/cesium/pull/3018)
- Billboards using `imageSubRegion` now render as expected. [#12585](https://github.com/CesiumGS/cesium/issues/12585)
- Improved performance of terrain Quadtree handling of custom data [#12907](https://github.com/CesiumGS/cesium/pull/12907)
- Fixed vertical exaggeration of ellipsoid-shaped voxels. [#12811](https://github.com/CesiumGS/cesium/issues/12811)
- Fixed parsing content bounding volumes contained in 3D Tiles 1.1 subtree files. [#12972](https://github.com/CesiumGS/cesium/pull/12972)
Expand Down
74 changes: 52 additions & 22 deletions packages/engine/Source/Renderer/TextureAtlas.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,16 @@ function TextureAtlas(options) {
this._initialSize = initialSize;

this._texturePacker = undefined;
/** @type {BoundingRectangle[]} */
this._rectangles = [];
/** @type {Map<number, number} */
this._subRegions = new Map();
this._guid = createGuid();

this._imagesToAddQueue = [];
/** @type {Map<string, number} */
this._indexById = new Map();
/** @type {Map<string, Promise<number>} */
this._indexPromiseById = new Map();
this._nextIndex = 0;
}
Expand Down Expand Up @@ -653,12 +657,16 @@ TextureAtlas.prototype.addImage = function (id, image) {
//>>includeEnd('debug');

let promise = this._indexPromiseById.get(id);
let index = this._indexById.get(id);
if (defined(promise)) {
// This image has already been added
// This image is already being added
return promise;
} else if (defined(index)) {
// This image has already been added and resolved
return index;
}

const index = this._nextIndex++;
index = this._nextIndex++;
this._indexById.set(id, index);

const resolveAndAddImage = async () => {
Expand All @@ -671,54 +679,76 @@ TextureAtlas.prototype.addImage = function (id, image) {
return -1;
}

return this._addImage(index, image);
return this._addImage(index, image).then((index) => {
this._indexPromiseById.delete(id);
return index;
});
};

promise = resolveAndAddImage();
this._indexPromiseById.set(id, promise);
return promise;
};

/**
* Get an existing sub-region of an existing atlas image as additional image indices.
* @private
* @param {string} id The identifier of the existing image.
* @param {BoundingRectangle} subRegion An {@link BoundingRectangle} defining a region of an existing image, measured in pixels from the bottom-left of the image.
* @param {number} imageIndex The index of the image.
* @returns {Promise<number> | number | undefined} The existing subRegion index, or undefined if not yet added.
*/
TextureAtlas.prototype.getCachedImageSubRegion = function (
id,
subRegion,
imageIndex,
) {
const imagePromise = this._indexPromiseById.get(id);
for (const [index, parentIndex] of this._subRegions.entries()) {
if (imageIndex === parentIndex) {
const boundingRegion = this._rectangles[index];
if (boundingRegion.equals(subRegion)) {
// The subregion is already being tracked
if (imagePromise) {
return imagePromise.then((resolvedImageIndex) =>
resolvedImageIndex === -1 ? -1 : index,
);
}
return index;
}
}
}
};

/**
* Add a sub-region of an existing atlas image as additional image indices.
* @private
* @param {string} id The identifier of the existing image.
* @param {BoundingRectangle} subRegion An {@link BoundingRectangle} defining a region of an existing image, measured in pixels from the bottom-left of the image.
* @returns {Promise<number>} A Promise that resolves to the image region index. -1 is returned if resouces are in the process of being destroyed.
* @returns {number | Promise<number>} The resolved image region index, or a Promise that resolves to it. -1 is returned if resources are in the process of being destroyed.
*/
TextureAtlas.prototype.addImageSubRegion = function (id, subRegion) {
//>>includeStart('debug', pragmas.debug);
Check.typeOf.string("id", id);
Check.defined("subRegion", subRegion);
//>>includeEnd('debug');

const imageIndex = this._indexById.get(id);
if (!defined(imageIndex)) {
throw new RuntimeError(`image with id "${id}" not found in the atlas.`);
}

const indexPromise = this._indexPromiseById.get(id);
for (const [index, parentIndex] of this._subRegions.entries()) {
if (imageIndex === parentIndex) {
const boundingRegion = this._rectangles[index];
if (boundingRegion.equals(subRegion)) {
// The subregion is already being tracked
return indexPromise.then((resolvedImageIndex) => {
if (resolvedImageIndex === -1) {
// The atlas has been destroyed
return -1;
}

return index;
});
}
}
let index = this.getCachedImageSubRegion(id, subRegion, imageIndex);
if (defined(index)) {
return index;
}

const index = this._nextIndex++;
index = this._nextIndex++;
this._subRegions.set(index, imageIndex);
this._rectangles[index] = subRegion.clone();

const indexPromise =
this._indexPromiseById.get(id) ?? Promise.resolve(imageIndex);

return indexPromise.then((imageIndex) => {
if (imageIndex === -1) {
// The atlas has been destroyed
Expand Down
2 changes: 0 additions & 2 deletions packages/engine/Source/Scene/Billboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,6 @@ function Billboard(options, billboardCollection) {
this._batchIndex = undefined; // Used only by Vector3DTilePoints and BillboardCollection

this._imageTexture = new BillboardTexture(billboardCollection);
this._imageWidth = undefined;
this._imageHeight = undefined;

this._labelDimensions = undefined;
this._labelHorizontalOrigin = undefined;
Expand Down
53 changes: 48 additions & 5 deletions packages/engine/Source/Scene/BillboardTexture.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,23 +251,67 @@ BillboardTexture.prototype.loadImage = async function (id, image) {
* @param {string} id An identifier to detect whether the image already exists in the atlas.
* @param {BoundingRectangle} subRegion An {@link BoundingRectangle} defining a region of an existing image, measured in pixels from the bottom-left of the image.
*/
BillboardTexture.prototype.addImageSubRegion = async function (id, subRegion) {
BillboardTexture.prototype.addImageSubRegion = function (id, subRegion) {
this._id = id;
this._loadState = BillboardLoadState.LOADING;
this._loadError = undefined;
this._hasSubregion = true;

let index;
const atlas = this._billboardCollection.textureAtlas;
const indexOrPromise = atlas.addImageSubRegion(id, subRegion);

if (typeof indexOrPromise === "number") {
this.setImageSubRegion(indexOrPromise, subRegion);
return;
}

this.loadImageSubRegion(id, subRegion, indexOrPromise);
};

/**
* @see {TextureAtlas#addImageSubRegion}
* @private
* @param {string} id An identifier to detect whether the image already exists in the atlas.
* @param {BoundingRectangle} subRegion An {@link BoundingRectangle} defining a region of an existing image, measured in pixels from the bottom-left of the image.
* @param {Promise<number>} indexPromise A promise that resolves to the image region index.
* @returns {Promise<number | undefined>}
*/
BillboardTexture.prototype.loadImageSubRegion = async function (
id,
subRegion,
indexPromise,
) {
let index;
try {
index = await atlas.addImageSubRegion(id, subRegion);
this._loadState = BillboardLoadState.LOADING;
index = await indexPromise;
} catch (error) {
// There was an error loading the referenced image
this._loadState = BillboardLoadState.ERROR;
this._loadError = error;
return;
}

if (this._id !== id) {
// Another load was initiated and resolved resolved before this one. This operation is cancelled.
return;
}

Comment on lines +293 to +313
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a race condition that is handled by loadImage but wasn't handled by addImageSubRegion before

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate? So this is a separate bug from what's originally being fixed in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I caught this from just comparing the two functions it's unrelated to the original issue

this._loadState = BillboardLoadState.LOADED;

this.setImageSubRegion(index, subRegion);
};

/**
* @see {TextureAtlas#addImageSubRegion}
* @private
* @param {number | undefined} index The resolved index in the {@link TextureAtlas}
* @param {BoundingRectangle} subRegion An {@link BoundingRectangle} defining a region of an existing image, measured in pixels from the bottom-left of the image.
*/
BillboardTexture.prototype.setImageSubRegion = function (index, subRegion) {
if (index && this._index === index) {
return;
}

if (!defined(index) || index === -1) {
this._loadState = BillboardLoadState.FAILED;
this._index = -1;
Expand All @@ -280,7 +324,6 @@ BillboardTexture.prototype.addImageSubRegion = async function (id, subRegion) {
this._height = subRegion.height;

this._index = index;
this._loadState = BillboardLoadState.LOADED;

this.dirty = true;
};
Expand Down
3 changes: 2 additions & 1 deletion packages/engine/Specs/DataSources/BillboardVisualizerSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,10 @@ describe(
const bb = billboardCollection.get(0);

await pollToPromise(function () {
entityCluster.update(scene.frameState);
scene.renderForSpecs();
visualizer.update(time);
return bb.show;
return bb.ready;
});

expect(bb.position).toEqual(testObject.position.getValue(time));
Expand Down
Loading