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.
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
Clear external tileset skeletons from tile tree to save memory usage #1107
Clear external tileset skeletons from tile tree to save memory usage #1107
Changes from 29 commits
dd27186
2aa36b3
d4c14db
c5a7fc4
e7d1ce5
3fb4007
2b35b26
101f71c
db707fd
ae37451
3816ef9
0b27e40
765e6db
7171545
38a9f08
7f8e023
7842279
7367c77
f705ac2
9c30fc5
21c03e2
140a7d9
9289398
8aa6cc0
b4dd32a
40f951f
b5d4637
489a541
598f49f
81e5311
7bc0896
c5de73a
7517543
160fa4d
3546686
4ea9154
44cf954
2009ee1
3cdf25e
9d757a7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's possible for any of these
children
to have loaded content at this point. I can't think of how they would, at least. Am I mistaken?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out it is possible - in
TilesetJsonLoader::createLoader
when we callcreateChildTiles
with the root tile fromparseTilesetJson
. WhenparseTilesetJson
is called with an implicit tileset, that root tile is created with external content which gives it theContentLoaded
state. Checking increateChildTiles
if any children have theContentLoaded
state and ticking up the_doNotUnloadSubtreeCount
for each one solves the issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting! Implicit tiling has been a bit of a blind spot for me in reviewing this so far. We should definitely make sure implicit tilesets are still working well in this PR, if you haven't already.
It doesn't need to be part of this PR, but we should eventually support unloading implicit tilesets, too. The rules there are a bit different, though. With an external tileset, once we're sure the entire thing is unused, we can unload all of it. It's all or nothing. However, with implicit tiling, we can recreate explicit tiles for any part of the implicit tree, so it's valid to unload any unused subtree.