Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Fix collada textures not being loaded #7407

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

achim-k
Copy link
Contributor

@achim-k achim-k commented Jan 29, 2024

User-Facing Changes
Fix collada textures not being loaded

Description
We were revoking temporary object URLs to early, resulting in errors when loading these URLs. This PR ties the lifetime of these temporary object URLs to the lifetime of the ModelCache.

Copy link
Contributor

@snosenzo snosenzo left a comment

Choose a reason for hiding this comment

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

Seems fine, though I do want to make sure: there's no problem with these models only being cleaned up on panel unmount? They shouldn't grow over time otherwise?

@achim-k
Copy link
Contributor Author

achim-k commented Jan 30, 2024

Seems fine, though I do want to make sure: there's no problem with these models only being cleaned up on panel unmount? They shouldn't grow over time otherwise?

No I don't think that there is an issue, especially since models are cached, so temporary object URLs are created only once for every model URL

@achim-k achim-k merged commit a51933b into main Jan 30, 2024
15 checks passed
@achim-k achim-k deleted the achim/fg-6397-urdf-collada-texture-loading-error branch January 30, 2024 12:51
@@ -220,25 +219,21 @@ export class ModelCache {
const objectUrl = URL.createObjectURL(
new Blob([textureAsset.data], { type: textureAsset.mediaType }),
);
textureUrls.set(textureUrl, objectUrl);
this.#colladaTextureObjectUrls.set(textureUrl, objectUrl);
Copy link
Member

Choose a reason for hiding this comment

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

Should we check if one already exists before creating a new one here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good remark, I'll create a follow-up PR for this

achim-k added a commit that referenced this pull request Jan 30, 2024
amacneil pushed a commit that referenced this pull request Jan 31, 2024
**User-Facing Changes**
None

**Description**
Follow up of
#7407 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants