Skip to content

Loading…

ol.source.RemoteVector #1744

Merged
merged 28 commits into from

7 participants

@twpayne

This PR is opened for discussion, it is not yet ready to be merged.

This PR experiments with an ol.source.RemoteVector designed for the case where the local ol.source.Vector is a cache of a subset of the features on a remote server.

The key changes are:

  • ol.source.VectorFile is split into two classes: ol.source.FormatVector and ol.source.StaticVector.

  • ol.source.FormatVector handles reading and writing features both synchronously and asynchronously in a given format.

  • ol.source.StaticVector loads all features in a given format from a variable or URL.

  • ol.source.RemoteVector inherits from ol.source.FormatVector and is configured with a couple of functions.

  • loadingFunction represents the loading strategy, skeleton implementations are in src/ol/loading.js.

  • extentUrlFunction takes an extent and returns the URL to request.

There is an example in examples/remote-vector.js but it is not yet functional because the demo.mapfish.org server does not support cross-origin requests.

@fredj, any chance of finishing off your OSM XML API parser so we can use it for testing this approach?

There's still more to be done, for example a strategy for expiring features from the ol.source.Remote.

Comments welcome.

@fredj
OpenLayers member
@jachym

Looks good to me. I would only suggest to add policy for on-server-generalized features (which are loaded later for bigger scales and would rewrite already loaded features.

@twpayne

This is ready for review based on the comments from last week's hangout and some help from @fredj.

Key changes:

  • The vector-osm example now dynamically loads vector data on demand using a tiling strategy.
  • Feature loading is explicit (ol.source.Vector#loadFeatures(extent, resolution)).
  • A feature id index is maintained; newly loaded features are only added if their id is not already present in the index.
@fredj fredj commented on the diff
examples/vector-osm.js
@@ -83,9 +87,18 @@ var styles = {
}
};
-var vectorSource = new ol.source.OSMXML({
- projection: 'EPSG:3857',
- url: 'data/osm/map.osm'
@fredj OpenLayers member
fredj added a note

The file can be removed from the repository.

@twpayne
twpayne added a note

Thanks! Commit updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fredj fredj referenced this pull request
Closed

Vector BBOX Strategy #617

@ahocevar
OpenLayers member

Thanks @twpayne for your continued effort on this. I'll give it a closer look tomorrow or on Tuesday. @tschaub, it would be great if you could also take another look. Maybe something to look at together in our Monday call?

@ahocevar ahocevar commented on an outdated diff
src/ol/source/remotevectorsource.js
((62 lines not shown))
+ for (i = 0, ii = features.length; i < ii; ++i) {
+ var feature = features[i];
+ var featureId = feature.getId();
+ if (!(featureId in this.loadedFeatures_)) {
+ notLoadedFeatures.push(feature);
+ this.loadedFeatures_[featureId] = true;
+ }
+ }
+ goog.base(this, 'addFeaturesInternal', notLoadedFeatures);
+};
+
+
+/**
+ * @inheritDoc
+ */
+ol.source.RemoteVector.prototype.loadFeatures = function(extent, resolution) {
@ahocevar OpenLayers member

I think this should get the projection as 3rd argument. The method would then check whether the requested projection is the same as the one that was used for previous requests. If it is, we can just move on. Otherwise, at least for now, the cache could be deleted.

Adding this would allow us to get rid of the projection configuration setting on the source.

If this feels like more than a trivial change, we can also handle it in a separate pull request.

@twpayne
twpayne added a note

Added in 52523fc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ahocevar
OpenLayers member

To me this looks good, but please wait until @tschaub is able to take another look, because he has thought loading strategies through more thoroughly than me.

One thing this does not currently handle is request methods other than GET. But I think this can also be handled in a separate pull request.

What remains is that I find the names FormatVector and RemoteVector confusing. I think FormatVector should be RemoteVector. RemoteVector could then be renamed to VectorSubset, and StaticVector could become VectorFile again - but it's hard to come up with good names for those two.

@twpayne

Thanks for the review @ahocevar. I've added a projection argument to loadFeatures.

For the naming, I agree that it's not easy. Maybe it would be best to brainstorm this in the hangout.

@kousu

This is amazing and we are a very interested party to this. I wish I knew ol better and could comment intelligently. I take it this has the ability to load different features at different zoom levels?

We plan in our project to not just load views dynamically but to update them too. I was going to push the vectors over ajax but giving each region of vectors a URL is much better. Is the API for topo/geojson updating thought out yet?

@twpayne

I take it this has the ability to load different features at different zoom levels?

This is being addressed in #1812 (also awaiting review).

Is the API for topo/geojson updating thought out yet?

This is entirely up to the application and server. The MapFish protocol is a simple REST-ful protocol for CRUD operations on features, and is a good choice if you don't need the complexity of the various OGC specifications.

@kousu

Wonderful! I was expecting to have to design something like that myself. I might bug you on the mailing list if I can't figure out how to get MapFish and OL to do what we want.

@tschaub
OpenLayers member

I really like the direction that ol.source.ServerVector is going. Thanks for the work on this @twpayne. I have a number of comments that I look forward to discussing on our call - some minor and some bigger. Basically, I think we can collapse the class hierarchy a bit and provide a lot of flexibility with a sensibly named ol.source.Vector class.

Two minor naming comments:

  • I prefer loader to loadingFunction - see ba3245f
  • I think we should say strategies are just about loading, and rename loadingStrategy to strategy - see 0abb2d6

The current ol.source.StaticVector provides seven different options for specifying how data is provided. These are: arrayBuffer, doc, node, object, text, url, and urls. I would collapse these to three: data, url, and urls (really I would ditch urls, but I'll leave that for now). All the data-type options get passed directly to the readFeatures method already. I think having a single data option is a more straightforward way to handle this.

My remaining comments are basically about where this loading functionality lives. As mentioned elsewhere, we could avoid having to specify the view's projection twice if all vector sources were lazily loaded. To support this, I would create factories like this:

  • ol.vector.createUrlLoader
  • ol.vector.createDataLoader

Both of these would create loader functions that trigger data loading once (and then become a noop). If a vector source is constructed with either data or url (or urls), the loader would be created with one of these factories. This would get rid of the current ol.source.StaticVector source.

I also think we could support a very sensibly named ol.source.Vector with this functionality. This would take the following options:

  • format - {ol.format.Feature} Used to parse feature data.
  • data - {ArrayBuffer|Document|Node|Object|string} Feature data (this is passed to readFeatures - which already has this signature)
  • url - {string} As an alternative to providing data at construction, a URL can be provided that will be used to fetch data.
  • loader - {function(this: ol.source.Vector, ol.Extent, number, ol.proj.Projection): string} A loader function. As an alternative to providing data or url, a custom loader function can be provided. If data or url is provided, a loader function will be created for you.
  • strategy - {function(ol.Extent, number): Array.<ol.Extent>} A function that determines the loading strategy (defaults to ol.vector.allStrategy).

And the projection option for vector sources could be removed (I think) if readFeatures took a second projection argument. Since this would be called by the loader, the projection is known and the user doesn't need to specify it again.

If we want to have a vector source class that doesn't load data, I would name it ol.source.BaseVector or something (this would have goog.nullFunction for loadFeatures as is the case with ol.source.Vector now).

Finally, we also have to talk about how cached features are expired. We can't just keep loading and caching features indefinitely.

@twpayne

I've updated this branch to add a vector tiles example:

ol3-tile-vector

@twpayne

At the hangout last week, a couple of suggestions were made:

  1. That ol.source.Vector be renamed to ol.source.VectorBase.

  2. That ol.source.FormatVector and ol.source.StaticVector be merged.

It turns out that neither of these are desirable. Here's why.

  1. ol.source.Vector is used extensively in the examples. Renaming it to ol.source.VectorBase would make them ugly.

  2. Merging ol.source.FormatVector and ol.source.StaticVector, and then ol.loadingstrategy.all to implement loading everything (what ol.source.StaticVector does) turns out to be not such a good idea because it hardcodes that all queries are done with bounding boxes into the library. Therefore, this approach would prevent (or at least make very difficult) the implementation of other ways of requesting extent-limited data from a server, such as the pure GeoJSON vector tiles demonstrated in the tile-vector example.

There are still a few FIXMEs in the code, these need the code from #1812.

@twpayne

I've rebased this on the latest master. Here's a screen shot of the tile-vector example:
vectortiles

@twpayne

You can play with the tile vector demo here.

@twpayne twpayne referenced this pull request
Closed

Vector tile to-do list #1911

0 of 3 tasks complete
@twpayne

Can this PR be merged? Although I don't think the code here is final, I do think it is a solid base to build on.

@fredj
OpenLayers member

+1 !

@tschaub
OpenLayers member

Of course there are still things I'd like to see changed. And I think having more eyes on it would be good. @elemoine have you done a review?

I'll bring up my comments on the call today.

ol.source.Vector is used extensively in the examples. Renaming it to ol.source.VectorBase would make them ugly.

ol.source.Vector would still be used in the examples. It would have the same functionality.

it hardcodes that all queries are done with bounding boxes into the library

This is another one I don't agree with. But I'll admit I haven't yet put my ideas into code.

@tschaub
OpenLayers member

In ol.source.TileVector the tile cache is append only (the clear method is never called). The tile vector example becomes unresponsive fairly often in my testing. This may not be related, but it makes me concerned about merging as is.

As with the append-only nature of ol.source.ServerVector, the tile cache in ol.source.TileVector behaves like a memory leak. I think both of these deserve more discussion (on pruning the cache).

I hope these comments are taken as constructive. I appreciate all this work, but I think that if we merge this as is, we will be less likely to rethink things later.

Update: I see that cache expiration is mentioned on #1911.

@twpayne

OK, but this branch is likely to be unmaintained after 16:00 UTC today...

@dreamind

Thanks for working on this feature. I've tried to use change handler for ol.source.TileVector to get the loaded features, but I received empty array from getFeatures(). It only works if I put some delay (like 3 seconds) before calling getFeatures()

source.once('change', function() {
  switch (source.getState()) {
    case ol.source.State.READY:
      var features = source.getFeatures();
@tschaub
OpenLayers member

@twpayne if you want to update this and merge, I'm ok with that.

As discussed on our calls, I do want to revisit the structure a bit and avoid passing the projection to static sources (and we clearly need to handle cache expiry) - but if we promote this as "experimental" it will be good to get people trying it out.

twpayne and others added some commits
@twpayne twpayne Rename ol.source.VectorFile to ol.source.StaticVector 77933d0
@twpayne twpayne Factor out ol.source.FormatVector 073f83c
@twpayne twpayne Rename ol.source.FormatVector#loadFeatures to loadFeaturesFromURL b008dd1
@twpayne twpayne Load features before rendering 8c984cc
@twpayne twpayne Add ol.loading 2842d1b
@twpayne twpayne Add ol.source.RemoteVector 50c4961
@twpayne twpayne Load features on demand in vector-osm example 10bddea
@twpayne twpayne Export ol.format.OSMXML cbbcab9
@twpayne twpayne Add projection argument when loading features b24e122
@twpayne twpayne Rename ol.source.RemoteVector to ServerVector de4a17b
@twpayne twpayne Rename ol.loading to ol.loadingstrategy b886980
@twpayne twpayne Replace extentUrlFunction with generic loadingFunction 9e75684
@twpayne twpayne Use a BBOX loading strategy by default db1a06a
@tschaub tschaub Rename loadingFunction to loader ce8d805
@tschaub tschaub Rename loadingStrategy to strategy 86c5a58
@twpayne twpayne Make tile URL functions more general a31ad69
@twpayne twpayne Separate out feature reading and feature adding 4a484a7
@twpayne twpayne Add ol.source.TileVector 683483f
@twpayne twpayne Add ol.source.Vector#forEachFeatureInExtentAtResolution 3a8504b
@twpayne twpayne Add tile-vector example 8986ea1
@fredj fredj Better vector style in tile-vector example fe216e3
@twpayne twpayne Use forEachFeatureInExtentAtResolution in ol.renderer.canvas.VectorLayer ed586ba
@twpayne twpayne Use forEachFeatureInExtentAtResolution in ol.source.ImageVector 96b7700
@twpayne twpayne Load vector tiles for current resolution f13debb
@twpayne twpayne Use openstreetmap.us vector tiles in tile-vector example a68d9f2
@twpayne twpayne Use TopoJSON in tile-vector example d3a13a2
@twpayne twpayne Add a polygon layer and tweak the style of the tile-vector example 3a462f7
@twpayne twpayne Export ol.source.ServerVector#readFeatures c155b70
@twpayne

This has now been rebased, and Travis is passing.

@twpayne twpayne merged commit 0de380c into openlayers:master

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@twpayne twpayne deleted the twpayne:remote-vector branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 3, 2014
  1. @twpayne
  2. @twpayne
  3. @twpayne
  4. @twpayne

    Load features before rendering

    twpayne committed
  5. @twpayne

    Add ol.loading

    twpayne committed
  6. @twpayne

    Add ol.source.RemoteVector

    twpayne committed
  7. @twpayne
  8. @twpayne

    Export ol.format.OSMXML

    twpayne committed
  9. @twpayne
  10. @twpayne
  11. @twpayne
  12. @twpayne
  13. @twpayne
  14. @tschaub @twpayne

    Rename loadingFunction to loader

    tschaub committed with twpayne
  15. @tschaub @twpayne

    Rename loadingStrategy to strategy

    tschaub committed with twpayne
  16. @twpayne
  17. @twpayne
  18. @twpayne

    Add ol.source.TileVector

    twpayne committed
  19. @twpayne
  20. @twpayne

    Add tile-vector example

    twpayne committed
  21. @fredj @twpayne
  22. @twpayne
  23. @twpayne
  24. @twpayne
  25. @twpayne
  26. @twpayne
  27. @twpayne
  28. @twpayne
Something went wrong with that request. Please try again.