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

Entity ease of use #2315

Merged
merged 170 commits into from
Feb 1, 2015
Merged

Entity ease of use #2315

merged 170 commits into from
Feb 1, 2015

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Dec 10, 2014

This isn't ready yet, but I'm opening it up for early review.

The goal here is to further enhance the DataSource layer so that it can be easier to use for beginners and also become the primary high-level API for Cesium. This would eliminate the need for the "mid-level" api we've considered adding, such as the current Polygon primitive (which would eventually be made obsolete). While the overall goal is still a long way off, this PR gets it a lot closer.

This change looks bigger/scarier than it is, but it mainly does 4 things:

  1. For simple values, you no longer need to create an instance of ConstantProperty, ConstantPositionProperty, ColorMaterialProperty, or ImageMaterialProperty. We create a new property on the fly if needed, mirroring the behavior of C# implicit types. createPropertyDescriptor has been deleted and replaced with several PropertyHelper functions to achieve this goal.
  2. All of the constructor functions for Entity and the various xxxGraphics objects now take an options argument which mirrors their public properties.
  3. A new CustomDataSource object, which makes easy to add Entities to a viewer instances in very little code. This is in lieu of simply having an entities property on Viewer. It fits a little better in the existing architecture. viewer.entities isn't off the table, but I think it would require more rework for not a lot of benefit. I've also thought about renaming the existing DataSource interface to something else (DataSourceInterface?) and renaming CustomDatasource to DataSource. This would be a non-breaking change that only affects documentation.
  4. EntityCollection.add now takes either an Entity instance or an options object. The options themselves can either be xxxGraphics instances or options objects themselves.

Here's a complete Sandcastle example that puts the above changes to use.

var viewer = new Cesium.Viewer('cesiumContainer');

var entity = viewer.entities.add({
    name: 'example',
    description : '<b>HTML description!</b>',
    position : Cesium.Cartesian3.fromDegrees(3, -4, 50000),

    label : {
        text : 'A label!',
        verticalOrigin : Cesium.VerticalOrigin.BOTTOM
    },

    point : {
        pixelSize : 15,
        color : Cesium.Color.RED
    },

    polygon : {
        material : Cesium.Color.BLUE,
        extrudedHeight : 50000,
        outline: true,
        outlineColor : Cesium.Color.BLACK,
        hierarchy : Cesium.Cartesian3.fromDegreesArray([-2, -2, 3, -4, 2, -2, 2, 2, -2, 2])
    }
});

viewer.zoomTo(entity);

TODO

  • Update documentation
  • Update CHANGES
  • Finish tests
  • Fix circular dependency in PropertyHelper
  • Add Checkerboard material property.
  • Port relevant Sandcastle examples
  • Add PolylineVolumeGeometry support to entity
  • Rotation broken when tracking in 2D.
  • Model.boundingSphere is wrong with Model.scale is set.[Model.boundingSphere was not properly scaling its center. #2444]

TODO post-merge (will write issues when ready)

  • Detect and automatically use EllipsoidSurfaceAppearance
  • Port Materials Sandcastle example and add entity features needed to do so.
  • Geometry Showcase: Support for per-segment or per-vertex polyline color.
  • Billboards/Labels: Define entity position in another reference frame.
  • Write blog post/tutorial for 1.6 release.
  • Update all tutorials (especially G&A) to distinguish between high-level and low-level functionality.
  • BoundingSphere.fromBoundingSpheres does not always produce a good sphere for view.

@mramato
Copy link
Contributor Author

mramato commented Dec 10, 2014

FYI, I just noticed the "unknown" commits; switched to a new system and apparently git wasn't fully configured when I committed. I'll fix it in the morning.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 10, 2014

Excellent!

Is code like this possible?

var viewer = new Cesium.Viewer('cesiumContainer');

var entity = viewer.entities.add(new Cesium.Entity({
    name: 'example',
    description : '<b>HTML description!</b>',
    position : Cesium.Cartesian3.fromDegrees(3, -4, 50000),

    label : {
        text : 'A label!',
        verticalOrigin : Cesium.VerticalOrigin.BOTTOM
    },

    point : {
        pixelSize : 15,
        color : Cesium.Color.RED
    },

    polygon : {
        material : Cesium.Color.BLUE,
        extrudedHeight : 50000,
        outline: true,
        outlineColor : Cesium.Color.BLACK,
        positions : Cartesian3.fromDegreesArray([-2, -2, 3, -4, 2, -2, 2, 2, -2, 2])
    },

    viewFrom : new Cesium.Cartesian3(100000, 0, 100000)
}));

viewer.trackedEntity = entity;

This:

  • Implicitly does viewer.extend(Cesium.viewerEntityMixin);. Why wouldn't we want this as a sensible default?
  • Replaces the explicit custom datasource with viewer.entities as mentioned above to reduce the number of objects a new Cesium developer needs to work with. A custom datasource feels natural to us, but it is one more thing a beginner needs to get their head around.
  • Drops the explicit need for Cesium.LabelGraphics, Cesium.PointGraphics, Cesium.PolygonGraphics. Is this a good idea? (We could even drop the explicit new Cesium.Entity)
    • If not, I would rename them to Cesium.Label, Cesium.Point, Cesium.Polygon, and then rename the low-level stuff. Yes, that is a BIG breaking change. I thought we've talk about this before, but I can't find the issue.
  • Changed example to use Cartesian3.fromDegreesArray for no good reason.

More questions:

  • How closely should this track CZML vs. the low-level Cesium API? For example, should we allow hex colors instead of Cesium.Color and strings instead of Cesium.VerticalOrigin? I imagine it becomes somewhat painful and inefficient.
  • I should already know this, but what is the code example for picking an entity? And a 3D model?
  • How efficient is this when the scene is rendered? Do we loop through each property of each entity? Each entity? Or is this a thin layer that delegates into the primitives when properties change?
  • Can we do a quick memory/performance test by creating examples that:
    • Create 10,000 billboards using entities. Create 10,000 billboards using primitives.
    • Something similar for polygons.
    • Both examples again, but with dynamic properties instead of static.
  • The external Cesium API comes pretty clean. We promote Entity for drawing stuff and DataSource for loading stuff, right? Hardcore users might dip into Primitive, Geometry, or even the private Renderer, but most shouldn't have to.
    • It would be a separate API, but we could also provide access to terrain/imagery/buildings/stars/etc through Viewer (Less API indirection? #2183). When I compare Cesium to a game, our environment/level is the terrain/imagery/etc, and the characters are the entities.
  • Do we have too many layers internally? If I add a property to Model, how many other objects do I need to add it to? Does it make sense to remove/combine anything?
  • Do we care that an AMD user needs to pull types from Core, Scene, DataSources to create an object? I don't think it is an issue.
  • Can we change the Sandcastle examples using primitives to use this? This would also help us test the interfaces more. We'll also have to go back to the tutorials at some point.

To simplify code for constant values (and to make it easier for beginners
to learn), DataSource properties were modified to take raw values as input
(and turn them into a corresponding property instance).  For example,

```
var foo = new PolygonGraphics
foo.positions = new ConstantProperty([...]);
foo.material = new ConstantProperty('./src.png');
foo.extrudedHeight = new ConstantProperty(10000);
```

becomes

```
var foo = new PolygonGraphics
foo.positions = [...];
foo.material = './src.png';
foo.extrudedHeight = 10000;
```

Reading the property back will return a Property instance in each of
the above cases.  Most values become ConstantProperty instances, materials
because either a `ColorMaterialProperty` or a `ImageMaterialProperty`
depending on type.  Position becomes a `ConstantPositionProperty`.

Also added an options argument to most DataSource graphics objects and
Entity that enables their properties to be set at construction time.

Added a CustomDataSource to easily add entities programmatically.
Also sort requires.

Deprecated id argument to Entity contructor.
Deprecated `ColorMaterialProperty.fromColor`.
@mramato
Copy link
Contributor Author

mramato commented Dec 10, 2014

FYI, I did a force push in order to get rid of the unknown commits (turns out git-gui was pointing to a different .gitconfig than my command line because of home directory issues).

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 10, 2014

Another question: how good is batching under the hood, e.g., are we going to run into a lot of cases where it slows down adding new entities or modifying them? What is the strategy?

@mramato
Copy link
Contributor Author

mramato commented Dec 10, 2014

Implicitly does viewer.extend(Cesium.viewerEntityMixin);. Why wouldn't we want this as a sensible default?

Since the original CesiumWidget (I'm talking dojo, pre-knockout) suffered from kitchen-sink syndrome; we were paranoid about putting too much stuff directly into the Viewer widget. Now that we have a better handle on where Cesium is going, we've talked about deprecating viewerEntityMixin and rolling it into Viewer proper, this should be an easy non-breaking change. I'm all for it, but I would do it in a separate PR.

Drops the explicit need for Cesium.LabelGraphics, Cesium.PointGraphics, Cesium.PolygonGraphics. Is this a good idea?

It should be easy to do and would be no different than how the other "raw" properties work. i.e. if you do entity.label = { text : 'foo'}, then entity.label automatically becomes a LabelGraphics instance. The only oddness that could come from it is the fact that the entity.label instance would not be the object you set, so if you did something like

var label = { text  : 'foo'}; 
entity.label = label; 
label.text = 'bar'

The last line would have no affect (because entity.label is not the same instance as label). But I don't think it's that big of a deal, so I'm in support of this change. We might want to eventually rename the object types too; but I would hold off on that until we're happy with everything else (in order to prevent churn).

Changed example to use Cartesian3.fromDegreesArray for no good reason.

👍 Had I remembered it existed, I would have done the same.

How closely should this track CZML vs. the low-level Cesium API? For example, should we allow hex colors instead of Cesium.Color and strings instead of Cesium.VerticalOrigin?

It would be possible to automatically turn a hex string into a Color instance; but it would only work for constant values; same goes for VerticalOrigin and other basic types. I don't think it's worth it yet and would definitely add some overhead (how much it would affect performance I'm not sure). Unless someone feels strongly otherwise; I would leave this out for the first pass and we can add the functionality later (in a non-breaking way) if we decide it's worth it.

I should already know this, but what is the code example for picking an entity?

You just do a normal pick and the id property of the primitive will be the Entity instance. We could definitely add a viewer.pickEntity function to make this trivial for users.

And a 3D model?

model : {
    uri : 'path/to/model',
    scale : 2,
    minimumPixelSize : 128
}

How efficient is this when the scene is rendered? Do we loop through each property of each entity? Each entity? Or is this a thin layer that delegates into the primitives when properties change?

It depends on the type of primitive being mapped. For example, static Geometry with constant visibility and color has basically zero overhead once the primitive is created. It will only be looked at again if something changes. For non-geometryr primitives, (Point/Billboard/Label/Path/Model), we loop over each entity once per frame and there is currently overhead even if the data is static. However, we already have infrastructure in place to allow us to remove that overhead; I just haven't had time to refactor the visualizers to take advantage of it. In the long run, performance overhead should be fairly negligible but right now it's definitely there for large data-sets (for non-geometry).

Can we do a quick memory/performance test

We can, but it's going to show what I describe above. I personally wouldn't worry about it until we are prepared to spend time addressing any discrepancies; particularly because I think the overhead is small enough not to matter for a majority of our users.

It would be a separate API, but we could also provide access to terrain/imagery/buildings/stars/etc through Viewer

There's a good possibility that we might need a way to express some of these things through the Entity interface as well (particularly because of KML and people's desire to add imagery via CZML). @kring started looking into this at one point, but we still need to nail down the semantics of how things would work. If people are writing code, then they would want to use the Cesium API directly, but external Data Sources will likely always be going through the entity system. None of this is set in stone though; we'll know more once we get further along in KML.

Do we have too many layers internally? If I add a property to Model, how many other objects do I need to add it to? Does it make sense to remove/combine anything?

In the long run, maybe; but I would say it's too early to try and do it now. In my opinion, we need time for the API to mature and for us to understand how people use it before we start taking stuff out.

Do we care that an AMD user needs to pull types from Core, Scene, DataSources to create an object? I don't think it is an issue.

I agree, non-issue. I would go as far to call it a feature of AMD. Like my answer above; I think a year or two from now we can take a step back at the mature API and decide if we want to collapse modules or move stuff around.

Can we change the Sandcastle examples using primitives to use this? This would also help us test the interfaces more. We'll also have to go back to the tutorials at some point.

I assumed that was the plan; but we should probably flesh out how and when we want to do that. It will definitely expose the need for other changes to make sure we have feature parity at the entity level.

how good is batching under the hood, e.g., are we going to run into a lot of cases where it slows down adding new entities or modifying them? What is the strategy?

I assume you are talking about geometry, since adding/modifying Point/Billboard/Label/Path/Model shouldn't be a problem. Geometry is currently batched per data-source into as few primitives as possible as per the rules of the batching system (i.e. things like translucency, appearance, open/closed, outline, etc.. affect what can be batched). Time-dynamic geometry is not batched at all (since it has to change every frame anyway). I'm sure how we treat dynamic geometry will change once we have dynamic buffers. One drawback here is that if we define 10,000 similar geometries (say opaque colored static boxes); that gets batched into a single primitive. If later on we add a new colored static box, we would end up rebatching that into a single primitive with 10,001 geometries. However, this isn't as bad as it sounds and I'm not sure how common of a use case it is. If some use cases end up slow; we have a lot of ways to improve things behind the scenes with non-breaking changes. We can easily add new heuristics for batching as needed.

Also keep in mind that if you want to express data in a time-dynamic faction, you would not do it by setting the above example to new values every frame. That would have a large adverse affect on performance. Instead you would use the built-in Property types we already expose, such as SampledProperty, CallbackProperty and TimeIntervalCollection property in order to describe the data over time up front. If you are receiving real-time data, then you would simply be pushing new data into these properties, which is different than giving us new "constant" values.

Finally, one known issue we'll have to deal with is dynamic but non-simulation time driven changes. For example, interactive polygon drawing. This is a case where all of the data is static, but changes constantly as the user moves the mouse. My current best strategy for this is to simply have the user tell us up front that the data is going to be changing frequently; though we could get fancy and try and auto-detect it if we want to.

Replaces the explicit custom datasource with viewer.entities

I saved this for last because I have mixed feelings about it. Since Cesium is an SDK, doing this without weirding up the API presents it's own challenges. I could add viewer.entities easily; but I'm not sure what affect it will have on everything else. For example, right now all entity visualization is driven by DataSourceDisplay; which itself holds onto a DataSourceCollection. Adding viewer.entities creates a special case where we have entities that are outside of that system. One option may be to have an implicit DataSource that is part of DataSourceCollection, but I would have to experiment with it to see how that could work. I'm by no means against adding viewer.entities I think we just need to work out a good way to do it.

That's a wrap. This definitely sets the record for longest GitHub response I've ever written. Hopefully I didn't miss anything.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 10, 2014

Agreed on most accounts.

I should already know this, but what is the code example for picking an entity?

You just do a normal pick and the id property of the primitive will be the Entity instance. We could definitely add a viewer.pickEntity function to make this trivial for users.

Not now, but I think we should revisit the whole procedural pick paradigm compared to events. It might make sense from our implementation perspective, but I imagine it is atypical of most JavaScript APIs.

Can we do a quick memory/performance test

We can, but it's going to show what I describe above. I personally wouldn't worry about it until we are prepared to spend time addressing any discrepancies; particularly because I think the overhead is small enough not to matter for a majority of our users.

I suspect looping over a lot of static billboards is going to hurt, which is why the billboard collection is designed the way it is. Even if we're not able to improve performance now, it would be good to know what we are getting into. Perhaps it's only a few percent. Perhaps it is more.

If I add a property to Model, how many other objects do I need to add it to?

I'd still like a quick discussion here as it seems like there is at least three places.

Can we change the Sandcastle examples using primitives to use this? This would also help us test the interfaces more. We'll also have to go back to the tutorials at some point.

I assumed that was the plan; but we should probably flesh out how and when we want to do that.

I would say ASAP if we are going to promote this as the Cesium API to use for most developers.

We can easily add new heuristics for batching as needed.

I think we will need them for the add/remove case (even the billboard and label collections need work here), but we could see what users really need first.

Also keep in mind that if you want to express data in a time-dynamic faction, you would not do it by setting the above example to new values every frame. That would have a large adverse affect on performance. Instead you would use the built-in Property types we already expose, such as SampledProperty, CallbackProperty and TimeIntervalCollection property in order to describe the data over time up front. If you are receiving real-time data, then you would simply be pushing new data into these properties, which is different than giving us new "constant" values.

Do you have some code examples here? I get it, but how natural is this to users compared to, for example, how they use labels and billboards today?

One option may be to have an implicit DataSource that is part of DataSourceCollection, but I would have to experiment with it to see how that could work

I don't know all the implementation details here, but, if it helps, we treat the globe as a special case in Scene.

`EntityCollection.add` now takes either an `Entity` instance or an options
object. The options themselves can either be `xxxGraphics` instances or
options objects themselves.
@mramato
Copy link
Contributor Author

mramato commented Dec 11, 2014

@pjcozzi I made some of the changes you asked for, namely the ability to not have to instantiate the xxxGraphics objects or Entity. I updated the code sample to match.

mramato and others added 22 commits January 29, 2015 12:32
Change parameger name for `Viewer.zoomTo` and `flyTo`
Allow InfoBox default sanitizer to be set to undefined to disable it.
Same for CompositeEntityCollection.entities.

Since EntityCollection is normally exposed itself as an `entities` property,
the naming was creating lots of awkward places where we had `foo.entities.entities`,
`foo.entities.values` makes this more readable and less error prone.
@mramato
Copy link
Contributor Author

mramato commented Feb 1, 2015

Unless anyone has any other comments, this is ready to be merged up to master.

@mramato
Copy link
Contributor Author

mramato commented Feb 1, 2015

@kring and @chris-cooper, since I know you were interested in it before, this change also introduces a dev-only Sandcastle folder of examples that get filtered automatically in the release. Just be sure it has the Development label and is in the development subfolder of the gallery.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 1, 2015

No need to hold this up, but coverage for several classes is pretty weak:

image

pjcozzi added a commit that referenced this pull request Feb 1, 2015
@pjcozzi pjcozzi merged commit bad2815 into master Feb 1, 2015
@pjcozzi pjcozzi deleted the implicit-properties branch February 1, 2015 23:02
@mramato
Copy link
Contributor Author

mramato commented Feb 1, 2015

I'm pretty sure coverage is the same (if not a little better) than master. I agree it needs to be improved, but this branch isn't the cause. Either way, I'll add improved coverage to the roadmap issue.

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.

3 participants