Skip to content
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

Cache Source instead of Texture instances in material system #5455

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

mrxz
Copy link
Contributor

@mrxz mrxz commented Feb 10, 2024

Description:
Final part of #5449, resolving #5441, #5120, #1372. Instead of caching textures and trying to do reference counting, the underlying Sources are cached instead. Actual reference counting is done by Three.js internally, which allows the same texture source to be shared with multiple textures, even if they differ in offset or repeat (which before would result in uploads).

This PR also improves the disposing behaviour, which previously only disposed when the material unregistered itself from the material system, missing any textures that were no longer assigned to that material. This also means that since Texture instances aren't shared, they can be changed by users without affecting other textures, while still using the same underlying image.

Changes proposed:

  • Let systems/material cache Source instances instead of Textures
  • Let helpers in utils/material request these Source instances and create/update the material's Texture instance


// Dispose old texture if present
if (material[materialName]) {
material[materialName].dispose();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this PR, the texture wouldn't be disposed when switching, effectively 'leaking' them. This did have the benefit of keeping it on the GPU in case the texture would be used later on. For example when a user repeatedly switches between two textures.

With this change, upon disposal the texture will be freed from the GPU (unless it's still being used elsewhere). So when reintroducing the texture in the scene, an upload takes place. The repeatedly switching would then incur a texture upload every time it switches.

The new behaviour is definitely more correct since A-Frame has no other way to track the (intended) lifecycle of a texture. But we might need to provide users with ways to keep textures 'alive'. Do note, however, that it's about the specific "texture source + texture properties". One such way might be to keep materials alive, see #5457

@dmarcos
Copy link
Member

dmarcos commented Feb 13, 2024

This need rebase

@mrxz
Copy link
Contributor Author

mrxz commented Feb 14, 2024

@dmarcos Rebased

@dmarcos
Copy link
Member

dmarcos commented Feb 20, 2024

Thanks. So much good work.

@dmarcos dmarcos merged commit fecd166 into aframevr:master Feb 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants