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

Adding CZML Examples #3050

Merged
merged 36 commits into from
Oct 9, 2015
Merged

Adding CZML Examples #3050

merged 36 commits into from
Oct 9, 2015

Conversation

sen-lu
Copy link
Contributor

@sen-lu sen-lu commented Sep 29, 2015

in reference to #3027

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 29, 2015

Thanks @TiffanyLu and @adamdavidcole. We'll do a quick review now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 29, 2015

Were you able to push the build directory to your gh-pages branch? If so, can you provide a link to Sandcastle so we can share the new CZML examples for community feedback?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 29, 2015

There is one JSHint error:

$ ./Tools/apache-ant-1.8.2/bin/ant build jsHint
Buildfile: /Users/pcozzi/source/cesium3/cesium/build.xml

build:

build:

jsHint:
[runJsHint] Checked 50 files
[runJsHint] Checked 100 files
[runJsHint] Checked 150 files
[runJsHint] Checked 200 files
[runJsHint] Checked 250 files
[runJsHint] Checked 300 files
[runJsHint] Checked 350 files
[runJsHint] Checked 400 files
[runJsHint] Checked 450 files
[runJsHint] Checked 500 files
[runJsHint] Checked 550 files
[runJsHint] Checked 600 files
[runJsHint] Checked 650 files
[runJsHint] Checked 700 files
[runJsHint] Checked 750 files
[runJsHint] Checked 800 files
[runJsHint] Checked 850 files
[runJsHint] Checked 900 files
[runJsHint] Errors:
[runJsHint] Apps/Sandcastle/gallery/CZML Points.html: line 29, col 14, Extra comma. (it breaks older versions of IE)

This is why Travis CI failed above: https://travis-ci.org/AnalyticalGraphicsInc/cesium/builds/82754199

@@ -0,0 +1,80 @@
<!DOCTYPE html>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update CZML Billboard and Label.jpg so the label is not missing the "A" in "AGI?" Just use Firefox to take the screenshot.

},
{
"id" : "Green circle at height",
name: "Green circle at height",
Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout all examples, please use strict JSON so name should be "name". In general, all CZML property names should be strings in double quotes so it is easy to store them as JSON.

Choose a reason for hiding this comment

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

Ok, I will go through and fix those.

Once I do, how do I update the pull request?

Thanks,
Adam

On Tue, Sep 29, 2015 at 11:09 AM, Patrick Cozzi [email protected]
wrote:

In Apps/Sandcastle/gallery/CZML Circles and Ellipses.html
#3050 (comment)
:

+

Loading...


+

+
+<script id="cesium_sandcastle_script">

  • function startup(Cesium) {
  •    "use strict";
    
  •    //Sandcastle_Begin
    
  •    var czml = [
    
  •        {
    
  •            "id" : "document",
    
  •            "name" : "CZML Geometries: Circles and Ellipses",
    
  •            "version" : "1.0"
    
  •        },
    
  •        {
    
  •            "id" : "Green circle at height",
    
  •            name: "Green circle at height",
    

Throughout all examples, please use strict JSON so name should be "name".
In general, all CZML property names should be strings in double quotes so
it is easy to store them as JSON.


Reply to this email directly or view it on GitHub
https://github.com/AnalyticalGraphicsInc/cesium/pull/3050/files#r40684026
.

@emackey
Copy link
Contributor

emackey commented Oct 5, 2015

Hey, this looks pretty cool. Here's a suggestion for code at the bottom of the "CZML Path" demo with the paraglider. This turns on terrain and tracks the entity.

var terrainSources = Cesium.createDefaultTerrainProviderViewModels();
var viewer = new Cesium.Viewer('cesiumContainer', {
    terrainProviderViewModels : terrainSources,
    selectedTerrainProviderViewModel : terrainSources[1]
});
viewer.dataSources.add(Cesium.CzmlDataSource.load(czml)).then(function(ds) {
    viewer.trackedEntity = ds.entities.getById('path');
});

"name" : "rectangle with image, above surface",
"rectangle" : {
"show" : true,
"coordinates" : { "wsenDegrees" : [-75, 40, -65, 50] },
Copy link
Contributor

Choose a reason for hiding this comment

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

The aspect ratio of the Cesium logo is off by a lot here, can you fix? Try this:

        "coordinates" : { "wsenDegrees" : [-75, 40, -50, 45] },

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 5, 2015

@emackey good feedback, thanks. @TiffanyLu @adamdavidcole let's make these changes.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 6, 2015

@TiffanyLu @adamdavidcole please merge Cesium master into this branch so GitHub will be able to auto merge this pull request.

Right now there is a merge conflict. It is most likely in CHANGES.md and will be easy to resolve. Let me know if you need any help and when this pull request is ready to merge (all feedback addressed and no merge conflict). Thanks!

"fill" : true,
"material" : {
"image" : {
"image" : { "uri" : "https://raw.githubusercontent.com/AnalyticalGraphicsInc/cesium/master/Apps/Sandcastle/images/Cesium_Logo_Color.jpg" }
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use a relative URL to the local Sandcastle file, not a full URL to GitHub.

@pjcozzi pjcozzi mentioned this pull request Oct 7, 2015
"fill" : true,
"material" : {
"image" : {
"image" : { "uri" : "/Apps/Sandcastle/images/Cesium_Logo_Color.jpg" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I don't mean to nit-pick, but this path is still absolute, not relative. Please use a relative path, like this:

            "image" : { "uri" : "../images/Cesium_Logo_Color.jpg" }

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll note the absolute path is not working on tiffanylu.com anyway, because it's deployed to a subfolder. The relative path should fix it there too.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 8, 2015

I removed the extrudedHeight property—do you still get the flicker?

Thanks, I will test tomorrow morning when I have access to Windows. In the meantime, is there anything else this pull request needs before we can merge?

@sen-lu
Copy link
Contributor Author

sen-lu commented Oct 8, 2015

@pjcozzi nope, we're ready!

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 9, 2015

I removed the extrudedHeight property—do you still get the flicker?

No, it looks good.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 9, 2015

Thanks again for this contribution, @TiffanyLu and @adamdavidcole!

pjcozzi added a commit that referenced this pull request Oct 9, 2015
@pjcozzi pjcozzi merged commit 249c038 into CesiumGS:master Oct 9, 2015
@pjcozzi pjcozzi deleted the czml-examples branch October 9, 2015 13:05
@mramato
Copy link
Contributor

mramato commented Oct 9, 2015

@TiffanyLu and @adamdavidcole, thanks a lot. These are awesome! I'm sure they will save a lot of users a lot of time.

I know I'm late to the party so I'm going to write up another issue with some suggestions, but it's mostly minor stuff. I'll also include a model example.

@mramato mramato mentioned this pull request Oct 9, 2015
11 tasks
@mramato
Copy link
Contributor

mramato commented Oct 9, 2015

See #3063

EDIT: I linked to the wrong issue

@sen-lu sen-lu mentioned this pull request Oct 14, 2015
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