Model caching fix #7796
Merged
Model caching fix #7796
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request @lilleyse!
Reviewers, don't forget to make sure that:
|
OmarShehata
approved these changes
Apr 30, 2019
| @@ -4438,7 +4437,7 @@ define([ | |||
| ModelUtility.updateForwardAxis(this); | |||
|
|
|||
| // glTF pipeline updates, not needed if loading from cache | |||
| if (!this._loadRendererResourcesFromCache) { | |||
| if (!defined(this.gltf.extras.sourceVersion)) { | |||
There was a problem hiding this comment.
Was this the lighting issue fix? Are you using sourceVersion as a way to know if this gltf object has already gone through this code-path so it doesn't go through when it's loaded in other viewers?
|
@lilleyse reminder that I don't have commit access here. |
@lilleyse please write up a new issue for this with any ideas you think are relevant and details of the problems with the current approach. Thanks and thanks for the fix! |
|
Opened #7801 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7688
This fixes both Sandcastle examples in #7688 by not removing the pipeline extras since it contains buffer data which models on different contexts need to access. This does increase memory usage since now the buffer needs to stay in memory, but it's the right thing to do. It doesn't affect 3D Tiles which avoids caching altogether and releases the glTF json.
The last version that worked, 1.49, also does not remove pipeline extras.
Also includes a fix where models on different contexts would have different lighting because
gltf.extras.sourceKHRTechniquesWebGLwould betruefor the second model and cause it to apply gamma correction unnecessarily.The comments in #7688 are good and I think we need to rethink caching to make it simpler once we can get around to it.