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

Geometry sandcastle examples #1151

Merged
merged 9 commits into from
Sep 24, 2013
Merged

Geometry sandcastle examples #1151

merged 9 commits into from
Sep 24, 2013

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Sep 16, 2013

I added a sandcastle example for each geometry type. In order to keep these from cluttering the other demos, I put the existing demos on a 'Showcase' tab, and put the new geometry examples on their own tab. I moved the 'All' tab to the end of the tab list.
I also enabled horizontal scrolling with the mouse wheel to all tabs, previously it only worked on the first tab.

This is to accompany the geometry and appearances tutorial I'm working on.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 16, 2013

Thanks @hpinkos. I want to look at this before merging, but it probably won't be until Wednesday.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2013

I like the new Sandcastle label; this will go a long way to separate demos from code snippets.

Let's rename

  • Showcase -> Showcases
  • Geometry Examples -> Geometries
  • Tutorial -> Tutorials

How hard is it to add vertical separators between the labels like our toolbar at the top?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2013

Drop all of these comments:

/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/

I get that it separates out the Sandcastle.finishedLoading(); but that shouldn't even be in the example at all IMO, but it will require more Sandcastle restructuring.

Also, use C++ style comments throughout, not C.

var positionOnEllipsoid = ellipsoid.cartographicToCartesian(Cesium.Cartographic.fromDegrees(-105.0, 45.0));
var boxModelMatrix = Cesium.Matrix4.multiplyByTranslation(
Cesium.Transforms.eastNorthUpToFixedFrame(positionOnEllipsoid),
new Cesium.Cartesian3(0.0, 0.0, dimensions.z/2));
Copy link
Contributor

Choose a reason for hiding this comment

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

dimensions.z / 2.0. Use .0 throughout for whole numbers if your intention is floating-point.

In this, case * 0.5 is better for performance or at least it was years ago.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2013

Drop the "Geometry" suffix from each example, e.g., "Circle Outline Geometry" -> "Circle Outline." These are already in the "Geometries" label. Make people read less. People hate to read. Really.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2013

Why do all the examples use opaque geometry? Most applications will use translucency. We should show some of both and our earliest examples in the tutorial should show whatever is the least amount of code to get started (translucent I believe).


var positions = [];
for (var i = 0; i <= 10; ++i) {
positions.push(ellipsoid.cartographicToCartesian(Cesium.Cartographic.fromDegrees(-100.0 + i, 30.0 + 3*i/2*(i%2))));
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace please.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2013

We might want to put the walls in the Wall Geometry example at a higher latitude so they look better for the default camera view.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2013

This is a nice well organized set of examples. I can see adding a few more as the tutorial covers appearances (e.g., using materials instead of just solid colors), but these are a good to start.

function registerScroll(demoContainer){
if (typeof document.onmousewheel !== 'undefined') {
demoContainer.addEventListener('mousewheel', function(e) {
if (typeof e.wheelDelta !== 'undefined' && e.wheelDelta) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use defined here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the defined function isn't part of Sandcastle

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we require in it? What is different about Sandcastle than our other apps?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2013

Let's update CHANGES.md.

Then this is ready.

@hpinkos
Copy link
Contributor Author

hpinkos commented Sep 18, 2013

How hard is it to add vertical separators between the labels like our toolbar at the top?

I looked into trying that with no success. Dojo sub-tabs don't seem to be very flexible.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 18, 2013

I looked into trying that with no success. Dojo sub-tabs don't seem to be very flexible.

Ugh. It's not a problem yet, but it will be as we scale this to more labels for more fine-grained examples.

@hpinkos
Copy link
Contributor Author

hpinkos commented Sep 18, 2013

@pjcozzi Did I get everything?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 23, 2013

FYI - these examples were really useful for the hackathon. Adding fine-grained examples, in addition to the current demo-ish examples, to Sandcastle is definitely the right move.

@hpinkos merge in master and I'll get this moving.

Conflicts:
	Apps/Sandcastle/CesiumSandcastle.js
	CHANGES.md
@hpinkos
Copy link
Contributor Author

hpinkos commented Sep 23, 2013

@pjcozzi sweet, I'm glad it was helpful. I merged in master.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 24, 2013

Love it. Add polyline volume examples next time you merge master in for #1125.

pjcozzi added a commit that referenced this pull request Sep 24, 2013
@pjcozzi pjcozzi merged commit 9cf8dad into master Sep 24, 2013
@pjcozzi pjcozzi deleted the geoTutorial branch September 24, 2013 14:49
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.

2 participants