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

3D Tiles #5308

Merged
merged 2,238 commits into from
Jun 19, 2017
Merged

3D Tiles #5308

merged 2,238 commits into from
Jun 19, 2017

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented May 10, 2017

For #3241

With the 3D Tiles spec converging to a 1.0 draft, the 3d-tiles branch in Cesium has been moving in parallel and is close to being production-ready. I'm opening this PR for some early feedback as we continue to go down the todo list.

To do:

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 13, 2017

@lilleyse pushing all those sound fine. Please write individual issues for each since some are good beginner issues.

Targeted Sandcastle demos.

Please do this before the next release.

Diagrams showing the 3D Tiles architecture.

The main point of this was to aid in reviewing this PR. Let's get some more value out of this by turning it into a short blog post for the Cesium / 3D Tiles community to encourage other 3D Tiles renderers.

@@ -1127,18 +1124,6 @@ defineSuite([
});
});

it('replacement and additive refinement', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a duplicate of another test.

@lilleyse
Copy link
Contributor Author

Merged master back into here and fixed up the RequestScheduler usage. For reviewing look at these commits:

02ba0eb
7df1e30

There is some functionality from #5317 that is still missing, like time slicing tiles that are processing and explicitly cancelling requests that go out of view. What's in here is the same behavior that 3d-tiles has always had, with some minor tweaks for using the improved request scheduler. I'll leave the other functionality for a later PR.

@lilleyse
Copy link
Contributor Author

I want to take a short break from this branch and then do a once-over for everything.

Thanks a lot @pjcozzi and @mramato for all the reviews here.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 13, 2017

like time slicing tiles that are processing and explicitly cancelling requests that go out of view

Both of these are important. Please submit a separate issue for each.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 13, 2017

If anyone wants to review or test this again, please do so ASAP so we can merge during the code sprint.

* @type {Cesium3DTileContent}
*
* @readonly
* @private
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,
we would like to have api access to the original batchTableJson. For this it would be good if the @Private here and at https://github.com/AnalyticalGraphicsInc/cesium/blob/3d-tiles/Source/Scene/Cesium3DTileContent.js#L208
could be removed. Or maybe a direct accessor to the BatchTableJson in the Cesium3DTileContent.js?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps push this until after this PR to control the scope so we can merge this soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah after the 3d Tiles merge in master i can create a small PR and get this topic back in the discussion

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be much appreciated, thanks!

var showExpression = defaultValue(styleJson.show, DEFAULT_JSON_BOOLEAN_EXPRESSION);
var pointSizeExpression = defaultValue(styleJson.pointSize, DEFAULT_JSON_NUMBER_EXPRESSION);

var expressions = styleJson.expressions;

Choose a reason for hiding this comment

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

I think this should be renamed from expressions to defines before this 3DTile will become in stable branch to prevent a spec update in next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll address this after merging this PR and before the next release.

@pjcozzi pjcozzi removed the bim label Jun 16, 2017
@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 18, 2017

Bump. Just a reminder that let's aim to merge this on Monday morning.

@lilleyse
Copy link
Contributor Author

I tested out a variety of tilesets and I believe this is ready to go. Other Sandcastle example look good too.

@mramato
Copy link
Contributor

mramato commented Jun 19, 2017

@lilleyse is writing up issues for any remaining task items that we agreed to do post merge.

There are a bunch of eslint errors when merging master, so I'll fix them in this branch and then merge up.

@mramato
Copy link
Contributor

mramato commented Jun 19, 2017

Congratulations, everyone!

@mramato mramato merged commit 1a28815 into master Jun 19, 2017
@mramato mramato deleted the 3d-tiles branch June 19, 2017 17:12
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.

None yet