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

CZML Example improvements #3063

Closed
mramato opened this issue Oct 9, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@mramato
Copy link
Member

commented Oct 9, 2015

Since I missed the boat on #3050, here's some feedback I have.

General

  • Most var viewer and viewer.dataSources.add lines are unecessarily indented and should just start at column 1.
  • I'm guessing 99% of use cases want cartographicDegrees, so we should use that in all examples. I love the CZML Position Definitions example, and I think that's the only place we need to have different formats.
  • Similarly for colors, we seem to flop back and forth between rgba and rgbaf. For beginner examples, always usingrgbamight be easier and we can have aCZML Colorsexample showcases different formats.
  • We don't need to explicitly state show: true, which most examples do. I think it's confusing and makes it look required. show should only be used in examples where it's time-dynamic or if you want it off by default for some reason.
  • Screen shot sizes seem to vary, for example CZML Polygon - Interpolating example is wider than most others. We should resnap all images to be the same size (using the view as Thumbnail button so they are consistent.
  • Add viewer.zoomTo(dataSource) to the end of the following examples CZML Point, CZML Circles and Ellipses, CZML Polygon, CZML Polyline, CZML Rectangle, CZML Spheres and Ellipsoids and CZML Wall

CZML Billboard and Label

  • The description doesn't need the or \r\n tags. It will always be treated as HTML. Let's change it to something like
<p><a href='http://www.agi.com' target='_blank'>Analytical Graphics, Inc.</a> (AGI) founded Cesium.</p>

CZML Path

  • createDefaultTerrainProviderViewModels is not part of the public API. This should just create and use the CesiumTerrainProvider.
  • Might be cool to also add a billboard, model or something to this demo to both illustrate that it's possible but also make it easier to follow the path visually, especially since there is a lead time. For a model, we'll need orientation (which is currently harder than it should be since it's all quaternions).
  • The trail time seems too long and it's hard to notice it by just looking at the demo.

CZML Wall

  • Perhaps a wall with per-position heights? Maybe just mirror the Geometry example.

Here's a complete model example. scale and minimumPixelSize are both optional.

var czml = [
    {
        "id" : "document",
        "name" : "Basic CZML billboard and label",
        "version" : "1.0"
    }, {
        "id" : "aircraft model",
        "name" : "Cesium Air",
        "position" : {
          "cartographicDegrees" : [
            -77, 37, 10000
          ]
        },
        "model": {
            "gltf" : "../../SampleData/models/CesiumAir/Cesium_Air.bgltf",
            "scale" : 2.0,
            "minimumPixelSize": 128
        }
      }
];

var viewer = new Cesium.Viewer('cesiumContainer');
var promise = viewer.dataSources.add(Cesium.CzmlDataSource.load(czml));

promise.then(function(dataSource){
    viewer.trackedEntity = dataSource.entities.getById('aircraft model');
}).otherwise(function(error){
    window.alert(error);
});

@mramato mramato added the type - bug label Oct 9, 2015

This was referenced Oct 9, 2015

@tiffanylu

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2015

Completed Task List

  • CZML Billboard and Label
  • CZML Circles and Ellipses
  • CZML Colors
  • CZML Model
  • CZML Path
  • CZML Point
  • CZML Point - Time Dynamic
  • CZML Polygon
  • CZML Polygon - Interpolating References
  • CZML Polygon - Intervals, Availability
  • CZML Polyline
  • CZML Position Definitions
  • CZML Rectangle
  • CZML Spheres and Ellipsoids
  • CZML Wall

@pjcozzi @adamdavidcole

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Oct 13, 2015

Work is being done in the czml-examples-tweaks branch.

We discussed offline:

  • Reference property example showing a billboard and label at the same position (and with a pixel offset for the label).
  • Will open pull request ASAP
  • Will start on #3063 once pull request is open.
@adamdavidcole

This comment has been minimized.

Copy link

commented Oct 14, 2015

@mramato @pjcozzi I believe there is a bug in the processing of the minimumHeights and maximumHeights array in the CZML update function. The error comes from line 463 in https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/DataSources/CzmlDataSource.js#L443 where I get a TypeError: Cannot read property 'length' of undefined since the 'unwrappedInterval' variable is undefined going into this line.

I believe the error is limited to calling the function processPacketData with type Array (which currently only occurs for wall maximum and minimum heights). More specifically, I believe the error occurs in line 557 where processProperty is called for each element in the packetData array but the type passed is Array. Therefore, when it reaches the switch statement on the data's type in the unwrapInterval function (line 344), it attempts to return czmlInterval.array (which is undefined because the passed date is a single value from the array).

I'm not sure exactly how the logic in parsing the array works, but I think either the entire array needs to be passed to processProperty in line 557, or processProperty needs to be called with a different type than Array when it is called for each element of the array.

For now, I will make a simpler example of wall for the CZML sandcastle and keep following this issue in the meantime.

@mramato

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2015

Thanks @adamdavidcole, you might be right about there being a bug here. Do you have the CZML that triggered it handy? I'll debug it and take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.