3d tiles, partial loading of tile trees #3237

e-andersson · 2015-11-23T08:55:48Z

As discussed with @pjcozzi and @lilleyse here are some changes that implement partial loading of tile trees and fix the problem of empty tiles (tiles.json-pointing tiles) being rendered.

…ement

…esium into 3d-tiles

pjcozzi · 2015-11-23T13:38:03Z

@lilleyse can you please do a first round of review on this? Then I'll do a quick look when you think it is ready.

lilleyse · 2015-11-23T13:48:36Z

@e-andersson I would like to make some minor changes to the code, could you give me commit access to your fork?

e-andersson · 2015-11-23T14:01:47Z

@lilleyse Sure, done!

lilleyse · 2015-11-23T14:05:56Z

Thanks!

lilleyse · 2015-11-23T17:05:42Z

Easier to view my comments inline with the code here

lilleyse · 2015-11-23T17:09:41Z

Overall most of the changes are organization related, with some miscellaneous cleanup. Please let me know if I overlooked something.

mramato · 2015-11-23T17:15:39Z

Source/Scene/Cesium3DTile.js

@@ -136,7 +150,10 @@ define([
            var type = url.substring(url.lastIndexOf('.') + 1);
            var contentFactory = Cesium3DTileContentProviderFactory[type];

-            if (defined(contentFactory)) {
+            if (type === 'json') {


This may be outside the scope of this PR, but would /type/i.test('json') be better here, or is lowercase guaranteed? Also, this looks like it is grabbing the type based on the file extension in the url. That's generally bad practice and we should be using content types. Though I imagine that's a spec-related decision.

We removed content.type: CesiumGS/3d-tiles#5

We can add it back, but it is redundant, except for the cases where the file wouldn't have an extension. Can you point me to something that showing this is the right practice in this context - as opposed to a generic web request? It feels like overkill.

I was referring to the actual web response content-type header. However, looking at this some more I think it's okay as long as the spec states that urls must end in the correct extension. Terrain does something similar and requires all tiles end in .terrain.

Yeap, thanks for looking into it.

pjcozzi · 2015-11-24T15:41:51Z

@lilleyse is this ready for me to look at?

lilleyse · 2015-11-24T17:54:17Z

@pjcozzi Yes this is ready.

pjcozzi · 2015-11-24T20:43:54Z

Source/Scene/Cesium3DTile.js

@@ -124,6 +124,20 @@ define([
        /**
         * DOC_TBA
         *
+         * @readonly
+         */
+        this.hasSubtree = false;


The hasSubtree and subtreeLoading terminology may be confusing since all non-leaf tiles have sub-trees. Maybe these should be isTileset and tilesetLoading?

pjcozzi · 2015-11-24T20:47:36Z

Sorry, more terminology, please call 3D tile tree a tileset throughout.

pjcozzi · 2015-11-24T20:50:28Z

@e-andersson if you can give me commit access to your fork, I will make trivial changes without bothering you?

pjcozzi · 2015-11-24T20:57:16Z

@e-andersson this looks good, thanks again! Just those minor terminology changes. I will also reformat a few things to make it closer to the Cesium conventions, but I can do that after we merge.

@lilleyse please cover all this possible cases (e.g., additive and replacement refinement) when you add tests.

e-andersson · 2015-11-24T21:19:34Z

Thanks @pjcozzi. I've given you access to my fork now, feel free to make those changes.
Just a note on isTileset -- doesn't isTileset make it look like we're talking about an actual Cesium3DTileset? Agreed though that subtree is more confusing.

Naming tweaks

pjcozzi · 2015-11-24T22:05:10Z

Source/Scene/Cesium3DTile.js

@@ -124,6 +124,20 @@ define([
        /**
         * DOC_TBA
         *
+         * @readonly
+         */
+        this.hasTilesetContent = false;


These names are perhaps slightly better.

Yes, looks a bit clearer to me!

pjcozzi · 2015-11-24T22:09:03Z

Just those comments. The approach is all good, we just need to finalize the corner case details for the spec.

…-tile-tests Added instanced tile specs

…array

e-andersson · 2015-12-01T08:57:42Z

@pjcozzi I wasn't very clear about the state of this. I'm working on a few more things at the moment, will get back soon when it's ready for review.

pjcozzi · 2015-12-01T13:14:56Z

OK, sounds good.

e-andersson · 2015-12-02T08:13:48Z

@pjcozzi Now when we've decided to not allow tileset content tiles with children, it seems nested tileset content tiles don't make sense anymore. Any tiles.json-in-tiles.json chain will be equivalent to just one tiles.json chain.

However, thinking about tileset loading strategies, I can see a use case where there is an all-encompassing root, that has as its children tilesets covering a large part of the earth. We wouldn't want to iterate over thousands of children. If we were to let tileset content tiles have children (in addition to its external root), it would cover this use case, but not in a very clean way.
Other possible options to deal with this use case:

Separate tree structure that helps decide when to load Cesium3DTilesets
Allow tiles.json that specify multiple children in top object, instead of a single root. A tile with this type of content wouldn't map well to a regular tile though.

pjcozzi · 2015-12-02T15:56:48Z

@e-andersson are you talking about the "tileset of tilesets" case where, for example, we may have a tileset for each city, but we don't want to load lots of tilesets just to meaningless loop over most of them since their projected screen-space error would be so small?

I think the right approach is to build a spatial data structure between each tileset (think of them as leaf nodes) and the root "tileset of tilesets" using basically empty interior nodes (with external tiles.json as desired). This is the same, for example, as a quadtree that only has content in leaf nodes - using the quad tree is more efficient than just making all the leaf nodes a child of the root.

e-andersson · 2015-12-03T08:03:28Z

@pjcozzi Yes, that's what I mean.

The approach you describe is pretty much what we over here have so far talked about as the most promising solution. In order to make a tileset of tilesets tree (quad or other), it seems that tiles with empty content need to be allowed by the specification. Currently the tiles schema says that a content.url is required, but perhaps that could be optional and if not given it would indicate an empty tile whose purpose is to let the tree branch out (thanks to its children).

I made a simple sketch of how I imagine such a tileset of tilesets would look, assuming empty content tiles are allowed. This is perhaps getting slightly off-topic for this pull request, but it would be nice to know this case will be supported within the current tileset/tiles specification before finishing.

pjcozzi · 2015-12-03T13:14:40Z

Wow, nice diagram. content.url is required, but content is optional. From the spec:

content is optional. When it is not defined, the tile's bounding volume is still used for culling (see Grids).

(Also, there is no "required" : true in the schema).

I believe this works for your use case (I have the same exact use case in mind). Do you agree?

I'm pretty sure I tested this in Cesium and I also added it to our list of unit tests to add (thanks for the reminder). Let me know if you run into an issue.

e-andersson · 2015-12-03T13:51:53Z

Thanks :-)

Ah, I looked in the wrong place, didn't occur to me to check whether content itself was required or not. Great, then I don't see any remaining issue. Will just change a few things back and then ping you when this PR is ready. Thanks for clarifying things!

pjcozzi · 2015-12-03T13:57:37Z

Sounds great!

e-andersson · 2015-12-07T08:11:49Z

@pjcozzi This is ready for review now. Sorry it took a while -- I started exploring other ways of structuring the code and the algorithm but in the end it didn't turn out well.

pjcozzi · 2015-12-07T12:56:44Z

Thanks @e-andersson! I am in a conference all day so I will look at this tomorrow.

pjcozzi · 2015-12-08T20:36:54Z

@lilleyse any comments?

pjcozzi · 2015-12-08T21:00:33Z

Source/Scene/Cesium3DTileset.js

@@ -381,7 +406,9 @@ define([
        }

        if (root.isContentUnloaded()) {
-            if (outOfCore) {
+            if (root.hasTilesetContent) {


Are you sure this shouldn't be:

if (outOfCore) { if (root.hasTilesetContent) { selectTileWithTilesetContent(tiles3D, selectedTiles, root, fullyVisible, frameState, true); } else { requestContent(tiles3D, root); } }

When outOfCore is false, the tileset is rendered for picking and we want to render the same exact commands as the previous pass without making/processing any requests.

You're right, submitted new pull request to fix this.

lilleyse · 2015-12-08T21:10:55Z

Nothing major to add, but I do wonder if Tileset3DTileContentProvider can be fleshed out more to include some of logic that happens during the traversal. However I'm not completely sure what this would look like.

pjcozzi · 2015-12-08T21:21:21Z

@lilleyse thanks, perhaps that is something that will become obvious to you when you write unit tests.

pjcozzi · 2015-12-08T21:30:38Z

Thanks again @e-andersson! I made a few style tweaks to make things consistent with the Cesium codebase (we are working on a guide in #3312 ). My only outstanding question is #3237 (comment).

I am going to merge this since it will help @lilleyse write unit tests. Open another PR if we need to tweak this.

3d tiles, partial loading of tile trees

e-andersson · 2015-12-09T09:42:50Z

@lilleyse I was thinking about that too, and tried a few things in that direction but didn't find a good, clean way of doing it. I'm available for discussion of other options if needed!

@pjcozzi Question addressed, new PR submitted. Hadn't seen that coding guide until now, will use from now on!

pjcozzi · 2015-12-09T12:35:22Z

@e-andersson you haven't seen the coding guide yet because we just finished the draft yesterday. :)

We also just wrote a Testing Guide. Once we finish the initial 3D Tiles tests (#3177), we'll ask that all new pull requests include tests.

e-andersson · 2015-12-09T13:11:52Z

That explains it :)
Looking forward to seeing the 3D tileset tests.

e-andersson added 7 commits November 20, 2015 16:25

partial tileset tree can now be loaded

d7ca786

clean-up, slight improvements in code organization

9307bcd

added partial tile tree loading support for tiles with additive refin…

2fcbdb7

…ement

Merge branch '3d-tiles' of https://github.com/AnalyticalGraphicsInc/c…

0b104dc

…esium into 3d-tiles

added company+name to contributors

f91f763

added company+name to contributors

d4cd7d3

fixed company address

853dc34

Review and organization changes

b5af3f9

mramato reviewed Nov 23, 2015
View reviewed changes

pjcozzi mentioned this pull request Nov 24, 2015

3D Tiles Roadmap #3241

Open

pjcozzi reviewed Nov 24, 2015
View reviewed changes

pjcozzi added 2 commits November 24, 2015 16:59

Naming tweaks

8e10d7b

Merge pull request #1 from e-andersson/tmp

6602734

Naming tweaks

pjcozzi reviewed Nov 24, 2015
View reviewed changes

Merge pull request CesiumGS#3239 from AnalyticalGraphicsInc/instanced…

d6a5942

…-tile-tests Added instanced tile specs

e-andersson added 2 commits December 1, 2015 09:18

the root is appended to children array, so pick the last item in the …

ac81e37

…array

removed TODO, changed comment

d2acff5

pjcozzi mentioned this pull request Dec 3, 2015

External tileset CesiumGS/3d-tiles#45

Merged

minor changes and comments

39ae7a7

Style tweak

050614b

pjcozzi reviewed Dec 8, 2015
View reviewed changes

Style tweaks

42aa267

Style tweak

0f67265

pjcozzi added a commit that referenced this pull request Dec 8, 2015

Merge pull request #3237 from e-andersson/3d-tiles

34ae325

3d tiles, partial loading of tile trees

pjcozzi merged commit 34ae325 into CesiumGS:3d-tiles Dec 8, 2015

pjcozzi deleted the 3d-tiles branch December 8, 2015 21:30

e-andersson mentioned this pull request Dec 9, 2015

Fix handling outOfCore -- no content requests if outOfCore is false #3317

Merged

lilleyse mentioned this pull request Dec 10, 2015

Rework external tiles.json #3328

Merged

3d tiles, partial loading of tile trees #3237

3d tiles, partial loading of tile trees #3237

Conversation

e-andersson commented Nov 23, 2015

pjcozzi commented Nov 23, 2015

lilleyse commented Nov 23, 2015

e-andersson commented Nov 23, 2015

lilleyse commented Nov 23, 2015

lilleyse commented Nov 23, 2015

lilleyse commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Nov 24, 2015

lilleyse commented Nov 24, 2015

Choose a reason for hiding this comment

pjcozzi commented Nov 24, 2015

pjcozzi commented Nov 24, 2015

pjcozzi commented Nov 24, 2015

e-andersson commented Nov 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Nov 24, 2015

e-andersson commented Dec 1, 2015

pjcozzi commented Dec 1, 2015

e-andersson commented Dec 2, 2015

pjcozzi commented Dec 2, 2015

e-andersson commented Dec 3, 2015

pjcozzi commented Dec 3, 2015

e-andersson commented Dec 3, 2015

pjcozzi commented Dec 3, 2015

e-andersson commented Dec 7, 2015

pjcozzi commented Dec 7, 2015

pjcozzi commented Dec 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Dec 8, 2015

pjcozzi commented Dec 8, 2015

pjcozzi commented Dec 8, 2015

e-andersson commented Dec 9, 2015

pjcozzi commented Dec 9, 2015

e-andersson commented Dec 9, 2015