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

Fixed model cache bug #5728

Merged
merged 2 commits into from
Aug 8, 2017
Merged

Fixed model cache bug #5728

merged 2 commits into from
Aug 8, 2017

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Aug 7, 2017

Fixes #5720

While the discussion in #5720 is all valid, the fix was simpler. This prevents the pipeline extra's source buffer from being removed when loading completes. This allows any future models to reference that same buffer.

I had to restructure one test which was doing a clone on the .gltf property, since our clone function doesn't work for typed arrays.

@mramato
Copy link
Contributor

mramato commented Aug 7, 2017

Are you sure this completely fixes the bug? Won't this still result in some model resource being loaded twice for the reason I discussed in the linked issue?

@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 7, 2017

This fixes the crash in the Sandcastle code and should fix possible other cases. Pre-2.0 we did something similar and held onto the bgltf buffer directly inside the gltf cache. Now the buffer data is just referenced by the gltf json.

Render resources do get created twice, but I'm pretty sure that's expected since the 2nd model is updated after the first is destroyed. If the 1st model is not deleted, both the gltf cache and renderer resources are created once.

@mramato
Copy link
Contributor

mramato commented Aug 8, 2017

I'll take your work for it, but I still think the model caching code overall could use a lot of cleanup.

@moneimne can you please review this? @hpinkos please merge when happy.

@moneimne
Copy link
Contributor

moneimne commented Aug 8, 2017

Seems fine to me!

@hpinkos
Copy link
Contributor

hpinkos commented Aug 8, 2017

Yeah, this doesn't seem like the best solution, but it will work for now

@hpinkos hpinkos merged commit be83a12 into master Aug 8, 2017
@hpinkos hpinkos deleted the model-cache-fix branch August 8, 2017 14:35
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.

Model cache bug introduced in 1.36
4 participants