Polyline geometry #1115

bagnell · 2013-09-04T19:07:27Z

Adds PolylineGeometry which creates polylines like those of PolylineCollection.
Adds PolylineColorAppearance like PerInstanceColorAppearance but for polylines.
Adds PolylineMaterialAppearance.

…to be projected to 2D and split for RTE.

…and adjacent positions.

pjcozzi · 2013-09-05T12:01:27Z

Sounds cool. Update the tutorial.

pjcozzi · 2013-09-05T12:12:09Z

Since polyline performance is such a hot topic, once we merge this, it is worth emailing the forum to tell folks to use this for fast static polylines, and that fast dynamic polylines are a little ways out, and to watch #932 for updates.

pjcozzi · 2013-09-05T12:16:53Z

In the Sandcastle example, is this Mac only?

pjcozzi · 2013-09-05T12:18:30Z

Source/Core/GeometryPipeline.js

     */
-    GeometryPipeline.projectTo2D = function(geometry, projection) {
+    GeometryPipeline.projectTo2D = function(geometry, attributeName, attributeName3D, attributeName2D, projection) {


This is a breaking change, but I suppose no need to add the noise to CHANGES.md since it is very unlikely that other folks are using this.

pjcozzi · 2013-09-05T12:36:59Z

Since polyline performance is such a hot topic, once we merge this, it is worth emailing the forum to tell folks to use this for fast static polylines, and that fast dynamic polylines are a little ways out, and to watch #932 for updates.

This also allows for the highly sought-after per-vertex color. It would be good to show how to do this and perhaps build it directly into PolylineGeometry. We'd need to add color to VertexFormat, which I am OK with. It should just work with PolylineColorAppearance.

pjcozzi · 2013-09-05T12:38:32Z

Source/Scene/PolylineCollection.js

@@ -1037,10 +1039,11 @@ define([
            return;
        }

+        var vsSource = PolylineCommon + '\n\n' + PolylineVS;


This is a trivial case, but I would still use createShaderSource to keep the shader pipeline consistent. CC @gbeatty

pjcozzi · 2013-09-05T12:47:04Z

Source/Scene/PolylineMaterialAppearance.js

+         *
+         * @see <a href='https://github.com/AnalyticalGraphicsInc/cesium/wiki/Fabric'>Fabric</a>
+         */
+        this.material = (defined(options.material)) ? options.material : Material.fromType(undefined, Material.ColorType);


Drop the parentheses.

pjcozzi · 2013-09-05T13:16:45Z

This looks pretty good. All the tests pass on Chrome and Firefox.

One final comment - how does a user interop between the outline geometries and the polyline geometry? Let's make it easy to construct a polyline geometry from an outline geometry, and add an example to Sandcastle.

Conflicts: CHANGES.md

…ted tests. Updated geometry Sandcastle example.

bagnell · 2013-09-07T01:03:44Z

@pjcozzi This is ready for another review.

For the screen shot you posted, I think that is just the polyline disappearing over the horizon.

For your comment about using polylines as outlines, I'll do that in a separate pull request. The outlines for extruded geometry may look strange.

pjcozzi · 2013-09-07T17:37:27Z

For your comment about using polylines as outlines, I'll do that in a separate pull request. The outlines for extruded geometry may look strange.

OK, let's have a look then we'll make the call. It should be trivial to code.

For the screen shot you posted, I think that is just the polyline disappearing over the horizon.

Err, OK, but it looks much different in Firefox and Chrome.

pjcozzi · 2013-09-07T17:44:56Z

Source/Core/Cartesian3.js

+     * @exception {DeveloperError} index must be a number greater than or equal to 0.
+     * @exception {DeveloperError} index + 3 is greater than the length of the array.
+     */
+    Cartesian3.writeElements = function(cartesian, cartesianArray, index) {


We should add this to all the Cartesian types and to CHANGES.md. Sorry, I guess I wasn't clear enough. An @example would be good to.

All this is pretty standard every time I add something to these types.

At first glance, this function looks identical to the pack function that @mramato already added.

We'll take either, but it looks like unpack is duplicate with the previously existing fromArray (we misnamed writeElements to not mirror this). I'll let you guys sort out what is what, but it would be nice to stay with the from convention.

Keep in mind that the Packable and PackableForInterpolation interfaces are implemented by many different Core types. I'm fine with tweaking names, but we need to make sure things stay consistent for all types and that the names in the Packable interface make sense in relation to each other. I personally would just leave the Packable stuff as is and if we want to add a fromArray that just calls unpack I'm fine with that too. writeElements can just go away.

pjcozzi · 2013-09-07T17:51:14Z

Tests and Sandcastle example are good. This is ready pending those trivial comments.

bagnell · 2013-09-09T18:11:03Z

@pjcozzi This is ready for another review.

pjcozzi · 2013-09-10T12:13:38Z

Looks good. Merging. Also, as I said before:

Since polyline performance is such a hot topic, once we merge this, it is worth emailing the forum to tell folks to use this for fast static polylines, and that fast dynamic polylines are a little ways out, and to watch #932 for updates.

Sounds cool. Update the tutorial.

CC @hpinkos

Polyline geometry

bagnell added 11 commits September 3, 2013 16:30

Modify projectTo2D to take attribute names.

f78c630

Assume that any attributes whose component data type is double needs …

13757a0

…to be projected to 2D and split for RTE.

Add PolylineGeometry.

e327f60

Add polyline color and material appearances.

6ec1bf1

Generate compute functions based on scene mode for polyline position …

474bfa4

…and adjacent positions.

Modify PolylineCollection to use common polyline shader code.

3b45cb8

Create polyline geometry on a web worker.

cf8ade8

Add polyline geometry to Sandcastle example.

e833b9f

Update doc.

e81d7d8

Add polyline geometry and appearances tests.

b6e97c5

Update CHANGES.md.

8847bef

pjcozzi reviewed Sep 5, 2013
View reviewed changes

bagnell added 5 commits September 6, 2013 14:58

Updates based on review.

4948653

Merge branch 'master' into polylineGeometry

dbe2cdf

Conflicts: CHANGES.md

Add per vertex and per segment color to polyline geometry.

dd443b0

Add polyline color tests.

1491c1a

Add per vertex/segment colors to simple polyline geometry. Added/upda…

0d9da9e

…ted tests. Updated geometry Sandcastle example.

pjcozzi reviewed Sep 7, 2013
View reviewed changes

bagnell added 2 commits September 9, 2013 13:43

Merge branch 'master' into polylineGeometry

867e12b

Updates based on review.

3498a5b

pjcozzi added a commit that referenced this pull request Sep 10, 2013

Merge pull request #1115 from AnalyticalGraphicsInc/polylineGeometry

b66679c

Polyline geometry

pjcozzi merged commit b66679c into master Sep 10, 2013

pjcozzi deleted the polylineGeometry branch September 10, 2013 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polyline geometry #1115

Polyline geometry #1115

bagnell commented Sep 4, 2013

pjcozzi commented Sep 5, 2013

pjcozzi commented Sep 5, 2013

pjcozzi commented Sep 5, 2013

pjcozzi Sep 5, 2013

pjcozzi commented Sep 5, 2013

pjcozzi Sep 5, 2013

pjcozzi Sep 5, 2013

pjcozzi commented Sep 5, 2013

bagnell commented Sep 7, 2013

pjcozzi commented Sep 7, 2013

pjcozzi Sep 7, 2013

shunter Sep 7, 2013

pjcozzi Sep 7, 2013

mramato Sep 7, 2013

pjcozzi commented Sep 7, 2013

bagnell commented Sep 9, 2013

pjcozzi commented Sep 10, 2013

Polyline geometry #1115

Polyline geometry #1115

Conversation

bagnell commented Sep 4, 2013

pjcozzi commented Sep 5, 2013

pjcozzi commented Sep 5, 2013

pjcozzi commented Sep 5, 2013

pjcozzi Sep 5, 2013

Choose a reason for hiding this comment

pjcozzi commented Sep 5, 2013

pjcozzi Sep 5, 2013

Choose a reason for hiding this comment

pjcozzi Sep 5, 2013

Choose a reason for hiding this comment

pjcozzi commented Sep 5, 2013

bagnell commented Sep 7, 2013

pjcozzi commented Sep 7, 2013

pjcozzi Sep 7, 2013

Choose a reason for hiding this comment

shunter Sep 7, 2013

Choose a reason for hiding this comment

pjcozzi Sep 7, 2013

Choose a reason for hiding this comment

mramato Sep 7, 2013

Choose a reason for hiding this comment

pjcozzi commented Sep 7, 2013

bagnell commented Sep 9, 2013

pjcozzi commented Sep 10, 2013