-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix billboard subregion not displayed #12958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0c9a523
3363ee8
981e206
cae6a02
452dd19
08ecab0
5d00fca
6f1349b
f73f2f2
1d6d5e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -680,13 +680,13 @@ TextureAtlas.prototype.addImage = function (id, image) { | |
| }; | ||
|
|
||
| /** | ||
| * Add a sub-region of an existing atlas image as additional image indices. | ||
| * Get 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 | undefined} The existing subRegion index, or undefined if not yet added. | ||
| */ | ||
| TextureAtlas.prototype.addImageSubRegion = function (id, subRegion) { | ||
| TextureAtlas.prototype.getImageSubRegion = function (id, subRegion) { | ||
| //>>includeStart('debug', pragmas.debug); | ||
| Check.typeOf.string("id", id); | ||
| Check.defined("subRegion", subRegion); | ||
|
|
@@ -697,28 +697,36 @@ TextureAtlas.prototype.addImageSubRegion = function (id, subRegion) { | |
| 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; | ||
| }); | ||
| return index; | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const index = this._nextIndex++; | ||
| /** | ||
| * 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. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also just return a number now, right? If the image subregion already exists, it just returns the index of the subregion directly. |
||
| */ | ||
| TextureAtlas.prototype.addImageSubRegion = function (id, subRegion) { | ||
| let index = this.getImageSubRegion(id, subRegion); | ||
| if (index) { | ||
| return index; | ||
| } | ||
| const imageIndex = this._indexById.get(id); | ||
|
|
||
| index = this._nextIndex++; | ||
| this._subRegions.set(index, imageIndex); | ||
| this._rectangles[index] = subRegion.clone(); | ||
|
|
||
| const indexPromise = this._indexPromiseById.get(id); | ||
| return indexPromise.then((imageIndex) => { | ||
| if (imageIndex === -1) { | ||
| // The atlas has been destroyed | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -251,15 +251,33 @@ 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; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const atlas = this._billboardCollection.textureAtlas; | ||||||||||||||||||||||||
| const index = atlas.getImageSubRegion(id, subRegion); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (index) { | ||||||||||||||||||||||||
| this.setImageSubRegion(index, subRegion); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| this.loadImageSubRegion(id, subRegion); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+262
to
+266
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Just a style preference |
||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * @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. | ||||||||||||||||||||||||
| * @returns {Promise<number | undefined>} | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| BillboardTexture.prototype.loadImageSubRegion = async function (id, subRegion) { | ||||||||||||||||||||||||
| let index; | ||||||||||||||||||||||||
| const atlas = this._billboardCollection.textureAtlas; | ||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| this._loadState = BillboardLoadState.LOADING; | ||||||||||||||||||||||||
| index = await atlas.addImageSubRegion(id, subRegion); | ||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||
| // There was an error loading the referenced image | ||||||||||||||||||||||||
|
|
@@ -268,6 +286,27 @@ BillboardTexture.prototype.addImageSubRegion = async function (id, subRegion) { | |||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (this._id !== id) { | ||||||||||||||||||||||||
| // Another load was initiated and resolved resolved before this one. This operation is cancelled. | ||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||
|
|
@@ -280,7 +319,6 @@ BillboardTexture.prototype.addImageSubRegion = async function (id, subRegion) { | |||||||||||||||||||||||
| this._height = subRegion.height; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| this._index = index; | ||||||||||||||||||||||||
| this._loadState = BillboardLoadState.LOADED; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| this.dirty = true; | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do note that unlike my previous implementation, if there are many (thousands+) of subregions then this could get pretty slow, since this is checked by all billboards with subRegions every frame. This could be solved by creating a new
BidirectionalMapcore class (which would store an additional inverseMap), and using it instead of the regular map for this_subRegionsmap. I don't think this is needed for this bug fix, but is a possible future improvement if this implementation is chosen over my previous one.