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

Better handling of processing 3d tiles #6364

Merged
merged 7 commits into from
Mar 30, 2018
Merged

Better handling of processing 3d tiles #6364

merged 7 commits into from
Mar 30, 2018

Conversation

lilleyse
Copy link
Contributor

While running a Draco compressed tileset we noticed that tiles seems to be streaming in reverse order. The problem was we were iterating over the processing queue in reverse order, mainly as a convenience since elements could be removed during iteration. In this tileset downloading tiles is much quicker than processing so the processing queue was stuffed with tiles and kept on processing the low priority tiles first.

The fix was to iterate in forward order and afterwards filter out tiles that are done processing.

Before/after:
nyc
nyc-better

@cesium-concierge
Copy link

Signed CLA is on file.

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


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

🌍 🌎 🌏

@lilleyse
Copy link
Contributor Author

I'm not sure why 3 of the tests fail in travis but not locally... I'll keep looking.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 23, 2018

Update CHANGES.md and add a unit test if possible. Are you also porting this to your refactor?

@lilleyse
Copy link
Contributor Author

Fixed the tests. One of the files in the i3dm specs data folder was called Box.glb instead of box.glb. The output from 3d-tiles-tools/sampler-generator looks fine so I'm not sure how that happened. The code here must have exposed that problem in some way.

I also updated CHANGES.md but didn't write a new test because it was too dependent on load order and the test started to get complicated.

Are you also porting this to your refactor

Yeah these changes will fit in fine there.

@lilleyse lilleyse mentioned this pull request Mar 29, 2018
2 tasks
@lilleyse
Copy link
Contributor Author

Do we want this in before 1.44? It will help loading if there are any Draco tilesets out there.

Otherwise if it doesn't matter much I can target the request-performance branch instead.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 29, 2018

If we are going to message Draco support, we should merge this.

@hpinkos
Copy link
Contributor

hpinkos commented Mar 29, 2018

If this is making it in the release it needs to be merged tomorrow. @lilleyse who do you think should review this?

@lilleyse
Copy link
Contributor Author

@likangning93 given that you are slated to review some of the other 3D Tiles PRs maybe it makes sense to review this too?

tiles[i - removeCount] = tile;
}
}
tiles.length -= removeCount;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For whoever is reviewing, this code is based on a similar filter function in RequestScheduler

@lilleyse
Copy link
Contributor Author

Ah I forgot @likangning93 is out today - @bagnell could you review today?

@bagnell bagnell merged commit 6e7eb4f into master Mar 30, 2018
@bagnell bagnell deleted the processing-queue branch March 30, 2018 19:29
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