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

Skeleton WebGL vector support #749

Merged
merged 26 commits into from
Jun 17, 2013
Merged

Skeleton WebGL vector support #749

merged 26 commits into from
Jun 17, 2013

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented May 30, 2013

This PR adds initial skeleton support for rendering vector data with WebGL. There's an example, ten-thousand-points, for demonstration.

Some outlines:

  • Coordinates are stored in fixed-size flat arrays (ol.struct.Buffer)
  • These arrays have mechanisms for tracking which elements have been modified and which elements are actually used
  • ol.geom2.PointCollection and ol.geom2.LineStringCollection build on top of ol.structs.Buffer to manage geometries whose coordinates are stored in these fixed-size flat arrays
  • Vector sources contain zero or more collections of each type
  • A skeleton WebGL renderer renders points and lines in a fixed style

There's still a lot missing, including:

  • Adding support for features built from one or more geometries
  • Polygon geometries
  • Styling
  • ...

@ahocevar
Copy link
Member

ahocevar commented Jun 1, 2013

Nice example! @tschaub and I started to review this - plan is to finish the review on Monday.

@@ -12,10 +12,12 @@ goog.require('ol.css');
goog.require('ol.layer.ImageLayer');
goog.require('ol.layer.TileLayer');
goog.require('ol.layer.Vector');
goog.require('ol.layer.VectorLayer2');
Copy link
Member

Choose a reason for hiding this comment

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

I think this pull request would be leaner without the canvas skeletons for VectorLayer2 - @tschaub and I are planning to integrate the geom2 stuff into geom and canvas in upcoming pull requests when this one is merged. But maybe I am missing something and these skeletons are needed to make things work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I had to add this because the Travis CI example integration tests are canvas only which means that I got an assertion error (effectively "no layer renderer available for this type of layer") without this.

@tschaub
Copy link
Member

tschaub commented Jun 4, 2013

Nice work @twpayne. It looks like the direction you are going is to favor geometry collections and not to have individual constructors for Point, LineString, et. al. I like the idea of just dealing with arrays instead of geometry instances, but this makes it awkward to cache things like geometry bounds - unless this is done at the feature level. The structures also get a bit deeper when we're dealing with things like MultiPolygons - or MultiPolygonCollections following this trend (it's relatively easy to track "ranges" for linestrings in a shared array, but more involved to track ranges for all rings of all polygons in all multipolygons in a shared array). I'm interested to have some more conversation about this.

I'm also trying to imagine the parsing or building phase for feature based data. I don't think the most efficient way forward is to build things like Array.<ol.geom2.LineString>, call ol.geom2.LineStringCollection.pack, and then call getOffsets() (unimplemented) on the returned collection, assigning offsets to features so they can later access geometry coordinates. Feels like it would be more efficient to have something like add that would be used during the initial assembly of the collections (without having to look for free ranges). Probably easier to get a clear understanding when we talk.

@twpayne
Copy link
Contributor Author

twpayne commented Jun 6, 2013

I've added the comments as discussed in today's hangout.

@ahocevar
Copy link
Member

Regarding @tschaub's comment about Object.<number, number>, in case it is not clear (it wasn't to me): Object literal keys are always strings in JavaScript.

Setting the renderer of the ten-thousand-points example to
WebGL should remove the need for this stub. This is basically
the same approach we took for all vector examples in master,
where we set the renderer to Canvas.

it('throws an error when dimensions are inconsistent', function() {
expect(function() {
var lsc = ol.geom2.LineStringCollection.pack([[0, 1], [2, 3, 4]]);
Copy link
Member

Choose a reason for hiding this comment

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

Wrong argument type. Should probably be [[[0, 1], [2, 3, 4]]].

@elemoine
Copy link
Member

I've just reviewed @twpayne's commits. Nothing more to say. It looks good to me.

@twpayne
Copy link
Contributor Author

twpayne commented Jun 17, 2013

Thanks for the reviews all.

twpayne added a commit that referenced this pull request Jun 17, 2013
@twpayne twpayne merged commit 7b9ac53 into master Jun 17, 2013
@twpayne twpayne deleted the webgl-vector branch June 17, 2013 14:08
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.

4 participants