ol.source.RemoteVector #1744

Merged
merged 28 commits into from Apr 3, 2014

Conversation

Projects
None yet
7 participants
@twpayne
Contributor

twpayne commented Feb 21, 2014

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

This comment has been minimized.

Show comment
Hide comment
Member

fredj commented Feb 25, 2014

@jachym

This comment has been minimized.

Show comment
Hide comment
@jachym

jachym Feb 25, 2014

Contributor

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.

Contributor

jachym commented Feb 25, 2014

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 comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 4, 2014

Contributor

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.
Contributor

twpayne commented Mar 4, 2014

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.
@@ -83,9 +87,18 @@ var styles = {
}
};
-var vectorSource = new ol.source.OSMXML({
- projection: 'EPSG:3857',
- url: 'data/osm/map.osm'

This comment has been minimized.

@fredj

fredj Mar 4, 2014

Member

The file can be removed from the repository.

@fredj

fredj Mar 4, 2014

Member

The file can be removed from the repository.

This comment has been minimized.

@twpayne

twpayne Mar 4, 2014

Contributor

Thanks! Commit updated.

@twpayne

twpayne Mar 4, 2014

Contributor

Thanks! Commit updated.

@fredj fredj referenced this pull request Mar 5, 2014

Closed

Vector BBOX Strategy #617

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Mar 9, 2014

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?

Member

ahocevar commented Mar 9, 2014

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?

src/ol/source/remotevectorsource.js
+/**
+ * @inheritDoc
+ */
+ol.source.RemoteVector.prototype.loadFeatures = function(extent, resolution) {

This comment has been minimized.

@ahocevar

ahocevar Mar 11, 2014

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.

@ahocevar

ahocevar Mar 11, 2014

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.

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Added in 52523fc.

@twpayne

twpayne Mar 12, 2014

Contributor

Added in 52523fc.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Mar 11, 2014

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.

Member

ahocevar commented Mar 11, 2014

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

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 12, 2014

Contributor

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.

Contributor

twpayne commented Mar 12, 2014

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 comment has been minimized.

Show comment
Hide comment
@kousu

kousu Mar 18, 2014

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?

kousu commented Mar 18, 2014

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

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 18, 2014

Contributor

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.

Contributor

twpayne commented Mar 18, 2014

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

This comment has been minimized.

Show comment
Hide comment
@kousu

kousu Mar 19, 2014

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.

kousu commented Mar 19, 2014

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

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Mar 20, 2014

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.

Member

tschaub commented Mar 20, 2014

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

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 25, 2014

Contributor

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

ol3-tile-vector

Contributor

twpayne commented Mar 25, 2014

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

ol3-tile-vector

@twpayne

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 25, 2014

Contributor

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.

Contributor

twpayne commented Mar 25, 2014

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

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 26, 2014

Contributor

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

Contributor

twpayne commented Mar 26, 2014

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

@twpayne

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 26, 2014

Contributor

You can play with the tile vector demo here.

Contributor

twpayne commented Mar 26, 2014

You can play with the tile vector demo here.

@twpayne twpayne referenced this pull request Mar 27, 2014

Closed

Vector tile to-do list #1911

0 of 3 tasks complete
@twpayne

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 27, 2014

Contributor

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.

Contributor

twpayne commented Mar 27, 2014

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

This comment has been minimized.

Show comment
Hide comment
@fredj

fredj Mar 27, 2014

Member

+1 !

Member

fredj commented Mar 27, 2014

+1 !

@tschaub

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Mar 27, 2014

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.

Member

tschaub commented Mar 27, 2014

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

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Mar 27, 2014

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.

Member

tschaub commented Mar 27, 2014

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

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 27, 2014

Contributor

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

Contributor

twpayne commented Mar 27, 2014

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

@dreamind

This comment has been minimized.

Show comment
Hide comment
@dreamind

dreamind Mar 28, 2014

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();

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

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Apr 2, 2014

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.

Member

tschaub commented Apr 2, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Apr 3, 2014

Contributor

This has now been rebased, and Travis is passing.

Contributor

twpayne commented Apr 3, 2014

This has now been rebased, and Travis is passing.

twpayne added a commit that referenced this pull request Apr 3, 2014

@twpayne twpayne merged commit 0de380c into openlayers:master Apr 3, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@twpayne twpayne deleted the twpayne:remote-vector branch Apr 3, 2014

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