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

Share textures across models in a limited but useful case #5051

Closed
wants to merge 5 commits into from

Conversation

kring
Copy link
Member

@kring kring commented Mar 1, 2017

This PR shares textures across models in the limited case of textures that come from regular old images. Textures that come from arraybuffers (e.g. compressed textures. textures embedded in a binary glTF) or from images with really long URLs (e,g, data URIs) are not supported here because determining a suitable cache key would be tricky.

My use case is a 3D Tiles tileset with lots of overlap in the textures used by different tiles. The images are all external to the b3dm files. Before this PR, heaps of duplicate textures would be created and eventually WebGL context would be lost after exceeding GPU memory. After this PR, it works great.

It's safe to say we'll want to support sharing textures (and probably other resources) in more scenarios eventually, but this incremental improvement is pretty useful.

One possible criticism of this PR is that loadImage is still called even for textures that do not need to be created because they're already in the cache. I didn't think it was worth the extra complexity that would be necessary in parseTextures to fix this, since the browser should already do a pretty good job of making this fast.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Thanks @kring this is also helpful for some of our tilesets.

@pjcozzi / @bagnell would either of you like to review too?

@@ -2234,6 +2234,8 @@ define([

///////////////////////////////////////////////////////////////////////////

var resourcesCachedAcrossModels = {};
Copy link
Contributor

@lilleyse lilleyse Mar 7, 2017

Choose a reason for hiding this comment

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

For consistency with other cached resources, should this instead be stored somewhere like context.cache.modelTextureCache?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 7, 2017

Yes, I would like to review. Please give me a week or two.

@pjcozzi pjcozzi mentioned this pull request Mar 28, 2017
@lilleyse lilleyse mentioned this pull request May 10, 2017
23 tasks
@mramato mramato changed the base branch from 3d-tiles to master June 19, 2017 17:09
@hpinkos
Copy link
Contributor

hpinkos commented Jan 11, 2018

@lilleyse @pjcozzi do we want these changes?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 20, 2018

@hpinkos we'll want something like this. We could start with this and get it over the finish line when it is a priority.

@cesium-concierge
Copy link

Thanks again for your contribution @kring!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Dec 12, 2018

@cesium-concierge stop

@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2019

I'm going to close this PR due to inactivity, but keep the branch around for when we want to revisit this.

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.

5 participants