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 Sandcastle demo #3463

Merged
merged 14 commits into from
Jan 26, 2016
Merged

3D Tiles Sandcastle demo #3463

merged 14 commits into from
Jan 26, 2016

Conversation

lilleyse
Copy link
Contributor

For #3177

I took most of the code from the Cities.html Sandcastle demo and made it into 3D Tiles.html. I added a dropdown to switch between data sets. Right now they are pulling from the Specs/Data folder, but I'm happy to copy them to the Apps/SampleData folder if that makes more sense.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 25, 2016

This didn't require any updates to server.js? I would expect that we should add something like

'application/octet-stream' : ['b3dm', /* other extensions */],

to here: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/server.js#L41-L46

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 25, 2016

Right now they are pulling from the Specs/Data folder, but I'm happy to copy them to the Apps/SampleData folder if that makes more sense.

Can you add a TODO to #3241, then we'll do this before merging to master to avoid headaches when regenerating the dataset.

var canaryWharfUrl = 'http://localhost:8002/tilesets/CanaryWharf/';
var seattleUrl = 'http://localhost:8002/tilesets/Seattle/';

var city;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but can you rename city to tileset? I started using tileset as the canonical variable name.

@lilleyse
Copy link
Contributor Author

This didn't require any updates to server.js? I would expect that we should add something like

Ah thanks for catching that, I made the change to my cleanup branch but not here.

@@ -405,7 +405,7 @@ define([

if (owner.debugShowContentBoundingVolume && hasContentBoundingVolume && workaround2657(tile._header.content.boundingVolume)) {
if (!defined(tile._debugContentBoundingVolume)) {
tile._debugContentBoundingVolume = tile._boundingVolume.createDebugVolume(Color.BLUE);
tile._debugContentBoundingVolume = tile._contentBoundingVolume.createDebugVolume(Color.BLUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 25, 2016

Also, please add a TODO to #3241 to integrate the sample CC3D data into this Sandcastle example and to generate the thumbnail.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 25, 2016

Any idea why this isn't 11.3? Is this a new issue with Cesium labels? Or something in this branch?

image

var cartesian = scene.pickPosition(movement.position);
var cartographic = Cesium.Cartographic.fromCartesian(cartesian);
var height = cartographic.height.toFixed(2) + ' m';

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 25, 2016

Any idea why this isn't 11.3?

Or does this have something to do with the 3D tileset?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 25, 2016

For the composite example, is the instance and batched tile in the composite tile? Looks like we need to move the debug color selection logic into the composite:

image

var instancedUrl = 'http://localhost:8080/Specs/Data/Cesium3DTiles/Instanced/InstancedWithBatchTable/';
var compositeUrl = 'http://localhost:8080/Specs/Data/Cesium3DTiles/Composite/Composite/';
var pointsUrl = 'http://localhost:8080/Specs/Data/Cesium3DTiles/Points/Points/';
var washingtonDCUrl = 'http://localhost:8002/tilesets/WashingtonDC/';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this and Canary Wharf and Seattle for now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 25, 2016

Just those comments. This is great; it will be soooooo useful for testing!

@lilleyse
Copy link
Contributor Author

Any idea why this isn't 11.3?

Not sure.. the console prints the correct values. I'll have to look around some more.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 26, 2016

This didn't require any updates to server.js? I would expect that we should add something like...

Working on this now. Will open a PR into this branch shortly.

@lilleyse
Copy link
Contributor Author

This is updated. I changed the organization for debug coloring so that most of the work is now in Cesium3DTile and each content provider implements their method of handling the debug update.

I'm still not sure why the labeling is weird, I've tried different string combinations and sometimes it works fine, sometimes nothing.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 26, 2016

Do the palette, Change id style, and Change Height style options still work for you?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 26, 2016

Just that question. This looks good!

@lilleyse
Copy link
Contributor Author

Result of a bad merge, fixed now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 26, 2016

Looks good!

pjcozzi added a commit that referenced this pull request Jan 26, 2016
@pjcozzi pjcozzi merged commit 3158fb6 into 3d-tiles Jan 26, 2016
@pjcozzi pjcozzi deleted the 3d-tiles-sandcastle branch January 26, 2016 22:18
scene.debugShowFramesPerSecond = true;
//viewer.extend(Cesium.viewerCesiumInspectorMixin);

var tilesetUrl = 'http://localhost:8080/Specs/Data/Cesium3DTiles/Tilesets/Tileset/';
Copy link
Contributor

Choose a reason for hiding this comment

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

These urls should not be hardcoded and they should not be absolutely. They need to be relative in order to work in all cases and on the Cesium website.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is important. Please use a relative path.

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.

4 participants