Skip to content
This repository

UTFGrid Layer, Tile and Controls #244

Closed
wants to merge 8 commits into from

5 participants

Matthew Perry Tim Schaub Éric Lemoine Peter Robins Andreas Hocevar
Matthew Perry

See the related email to dev list at:
http://osgeo-org.1560.n6.nabble.com/preliminary-UTFGrid-support-in-OpenLayers-td4477224.html

More info here:
http://www.perrygeo.net/wordpress/?p=160

I'd love to see support for UTFGrids included in the next release. Let me know if there's anything I can do to clean things up to make this happen...

Tim Schaub
Owner

I've added a few commits that I'm hoping you'll consider. These are included in a pull request. I wanted to make some changes that were based on recent work in master. The bulk of the commits in that pull request will go away if you merge master with your branch.

The first change (34a9c46) just adds the new UTFGrid component to the debug loader so examples can work without a build. The next change (6284e44) updates the examples so they fit in a bit better with the rest (and run without the missing ./OpenLayers.js).

My understanding of the UTFGrid spec is that the structure of property values in the data member may be of any type. The existing implementation assumes that the property values will be objects with keys representing feature attribute names and values representing these attribute values. The UTFGrid spec examples show a simple data structure with strings that wouldn't work with this implementation. I've updated things (e1dffac) so that the user provided callback receives a data lookup - keys corresponding to layer index and values straight from the data member.

This change (e1dffac) also gives the layer a getData method. I think this is a bit more flexible/sensible than having the control extract data from the layer's tiles.

I'd be interested to here if others have comments on this. That's it for now. More review later.

Tim Schaub
Owner

I'm adding commits in my fork while reviewing.

  • 73e6973 - Removed image stuff from UTFGrid tile.
  • e6f0aa0 - Tweaked use of Script protocol. Open question: doesn't the GeoJSON format throw when parsing the UTFGrid data? Good candidate for a test.
  • c18f6a2 - I changed the JSON parsing to use the JSON format. This is essentially for IE7 support. I'm open for pushback here. I know there's a push to minimize our build size, and folks are scoffing at legacy IE support. But I've sat in some fairly big agencies (USGS, NOAA) and seen first hand the value in having OpenLayers be a bit more concerned about cross-browser compatibility than others.
  • 12fa9b5 - Since loadstart is fired, it makes sense to fire loadend as well.
  • faeb691 - Initial tests for UTFGrid tiles.
  • 551c582 - Updated layer and tile API so tile is responsible for getting feature id/data given pixel offsets and layer does the same for map locations.
Matthew Perry

This is all looking great from my end. I like the new API a lot and seems like the most sensible way to do it.

What else is left?

Tim Schaub
Owner

We should put together tests for the layer and control as well. I've grabbed the UTFGrid demo.json to validate the implementation and am finding that I can't get tests to pass (see this issue).

Tim Schaub
Owner

In 4d31a3e, I've changed the layer's getFeatureData method to getFeatureInfo. The old getFeatureData method was already retrieving feature ids, and it doesn't make sense to force the user to do two requests for the complete bundle of information.

In experimenting with the UTFGrid highlighting, I've noticed a few other issues with the layer. Will be good to write up tests for these.

Tim Schaub
Owner

I think it would be great to get this into 2.12. There are a few more changes I'd like to make. Matt, I'm interested to know how these sound to you.

Single arg layer constructor

I think single arg constructors are the way forward for the library. Details upon request. For this layer, here's the difference.

Instead of this

var layer = new OpenLayers.Layer.UTFGrid(
    null, 
    "utfgrids/${z}/${x}/${y}.json",
    {utfgridResolution: 4, displayInLayerSwitcher: false}
);

A user would write this

var layer = new OpenLayers.Layer.UTFGrid({
    url: "utfgrids/${z}/${x}/${y}.json",
    utfgridResolution: 4, 
    displayInLayerSwitcher: false
});

The difference gets more pronounced when you start assigning more layer properties in the constructor. The three arg constructor is particularly awkward when you have a bunch of options (highlightStyle, useJSONP, zoomOffset, etc.)

Tests

The layer and control still need tests.

I'm also interested to work out what is going on with character codes between 55296 and 57343 (mapbox/utfgrid-spec#1). I'm fairly confident there is nothing wrong with this implementation. It's either an encoding issue with the example/tests or a real consideration that needs to be accounted for in the spec. So, while I don't think it should block inclusion of these components, I'd like to figure out why the demo.js tests won't pass.

Examples

I'd like to see the JSONP functionality in action. Are there reliable servers that we could use in an example? I think it also makes sense to have examples with pan/zoom navigation. I understand it's tough to load up a bunch of static UTFGrids. But at least being able to pan a bit or zoom once would be nice.

Tweaks

Maybe persnickety, but I'm inclined to change utfgridResolution to gridResolution. While a bit ambiguous, I find the latter easier to type (the spec refers to this as factor - even tidier, but less descriptive).

I'd like to clean up the examples a bit more. Minor stuff: separate js file, some name/structure changes.

Matthew Perry

Single Arg constructor

+1

Tests

I'm a bit unfamiliar with javascript unit testing and I probably won't have time to really delve into them until later next week unfortunately.

JSONP

The problem is that OpenLayers script protocol uses a global callback registry so the returned JSONP file must dynamically wrap the json in that function. At the moment, the jsonp support is not very practical since the two main utfgrid servers don't support dynamic jsonp callbacks.

Tilestream serves up jsonp files with a hardcoded grid(..) callback and, for reasons of caching and CDN storage, probably will continue to do so.

I've added dynamic jsonp callbacks to my Tilestache fork at https://github.com/perrygeo/tilestache but that hasn't been pulled into the upstream master yet nor are there really any reliable servers out there running it.

Pan/Zoom in examples

I will update the examples and generate another zoom level of tiles if it's OK to add them to the repository.

Tweaks

-1 on gridResolution .. its a bit too ambiguous since we refer to the tile layout as the "grid" ... I would be in favor of utfgridFactor or gridFactor to match the specs more closely.

Tim Schaub
Owner

Thanks for the feedback. All sounds good. I'm happy to stick with your utfgridResolution - it is the most descriptive. If you can pile in a few more utfgrids, I think that would be great. You'll likely notice some dateline wrapping issues - not specific to this code I think, but something I want to check out. Might be easiest to have a multiple zoom level example with a restricted extent & set of resolutions.

I'll try to find a bit more time for the tests.

Tim Schaub
Owner

Making additional changes discussed above.

  • 2feb860 - example restructuring
  • 36d22bb - single arg constructor
  • 268b842 - test for layer and control (passing in IE6, Chrome 17, etc.)
  • 32db586 - providing more detail to the callback
  • c6aa996 - example with multiple zoom levels
Tim Schaub tschaub referenced this pull request March 03, 2012
Merged

UTFGrid support #274

Tim Schaub
Owner

As mentioned on the other pull request, I only opened that in case Matt doesn't get a chance to pull in my additional commits. I'd like to get some additional eyes on this, but I think it's in good shape. I still want to have an example that allows for some navigation. I've pulled down three zoom levels of grids from MapBox's geography-class layer. I'll try to put together an updated example with that before too long.

Tim Schaub
Owner

Complete batch of commits merged w/ 1a44458. Thanks @perrygeo for the collaboration on this.

Tim Schaub tschaub closed this March 09, 2012
Matthew Perry

I just got back from vacation and was psyched to see it merged already. Thanks for all the hard work on this - it looks great! Anything else to do to tie up loose ends before the release?

Éric Lemoine

Using imageSize for the frame size does not make sense to me. The tileSize should be used for the frame size. imageSize should be for the image inside the frame.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
2  examples/gutter.html
@@ -45,7 +45,7 @@ <h1 id="title">Gutter Example</h1>
45 45
           "http://demo.opengeo.org/geoserver/wms",
46 46
           {layers: 'topp:states'},
47 47
           {gutter: 15});
48  
-      var states = new OpenLayers.Layer.WMS( "Roads (no gutter)",
  48
+      var states = new OpenLayers.Layer.WMS( "States (no gutter)",
49 49
           "http://demo.opengeo.org/geoserver/wms",
50 50
           {layers: 'topp:states'});
51 51
       map.addLayers([states, states15]);
11  lib/OpenLayers/Layer.js
@@ -159,13 +159,6 @@ OpenLayers.Layer = OpenLayers.Class({
159 159
      */
160 160
     imageSize: null,
161 161
     
162  
-    /**
163  
-     * Property: imageOffset
164  
-     * {<OpenLayers.Pixel>} For layers with a gutter, the image offset 
165  
-     *     represents displacement due to the gutter.
166  
-     */
167  
-    imageOffset: null,
168  
-
169 162
   // OPTIONS
170 163
 
171 164
     /** 
@@ -693,7 +686,7 @@ OpenLayers.Layer = OpenLayers.Class({
693 686
     /**
694 687
      * APIMethod: setTileSize
695 688
      * Set the tile size based on the map size.  This also sets layer.imageSize
696  
-     *     and layer.imageOffset for use by Tile.Image.
  689
+     *     or use by Tile.Image.
697 690
      * 
698 691
      * Parameters:
699 692
      * size - {<OpenLayers.Size>}
@@ -710,8 +703,6 @@ OpenLayers.Layer = OpenLayers.Class({
710 703
           //                              this.name + ": layers with " +
711 704
           //                              "gutters need non-null tile sizes");
712 705
           //}
713  
-            this.imageOffset = new OpenLayers.Pixel(-this.gutter, 
714  
-                                                    -this.gutter); 
715 706
             this.imageSize = new OpenLayers.Size(tileSize.w + (2*this.gutter), 
716 707
                                                  tileSize.h + (2*this.gutter)); 
717 708
         }
29  lib/OpenLayers/Protocol/HTTP.js
@@ -62,13 +62,28 @@ OpenLayers.Protocol.HTTP = OpenLayers.Class(OpenLayers.Protocol, {
62 62
     scope: null,
63 63
 
64 64
     /**
65  
-     * Property: readWithPOST
  65
+     * APIProperty: readWithPOST
66 66
      * {Boolean} true if read operations are done with POST requests
67 67
      *     instead of GET, defaults to false.
68 68
      */
69 69
     readWithPOST: false,
70 70
 
71 71
     /**
  72
+     * APIProperty: updateWithPOST
  73
+     * {Boolean} true if update operations are done with POST requests
  74
+     *     defaults to false.
  75
+     */
  76
+    updateWithPOST: false,
  77
+    
  78
+    /**
  79
+     * APIProperty: deleteWithPOST
  80
+     * {Boolean} true if delete operations are done with POST requests
  81
+     *     defaults to false.
  82
+     *     if true, POST data is set to output of format.write().
  83
+     */
  84
+    deleteWithPOST: false,
  85
+
  86
+    /**
72 87
      * Property: wildcarded.
73 88
      * {Boolean} If true percent signs are added around values
74 89
      *     read from LIKE filters, for example if the protocol
@@ -293,7 +308,8 @@ OpenLayers.Protocol.HTTP = OpenLayers.Class(OpenLayers.Protocol, {
293 308
             requestType: "update"
294 309
         });
295 310
 
296  
-        resp.priv = OpenLayers.Request.PUT({
  311
+        var method = this.updateWithPOST ? "POST" : "PUT";
  312
+        resp.priv = OpenLayers.Request[method]({
297 313
             url: url,
298 314
             callback: this.createCallback(this.handleUpdate, resp, options),
299 315
             headers: options.headers,
@@ -344,11 +360,16 @@ OpenLayers.Protocol.HTTP = OpenLayers.Class(OpenLayers.Protocol, {
344 360
             requestType: "delete"
345 361
         });
346 362
 
347  
-        resp.priv = OpenLayers.Request.DELETE({
  363
+        var method = this.deleteWithPOST ? "POST" : "DELETE";
  364
+        var requestOptions = {
348 365
             url: url,
349 366
             callback: this.createCallback(this.handleDelete, resp, options),
350 367
             headers: options.headers
351  
-        });
  368
+        };
  369
+        if (this.deleteWithPOST) {
  370
+            requestOptions.data = this.format.write(feature);
  371
+        }
  372
+        resp.priv = OpenLayers.Request[method](requestOptions);
352 373
 
353 374
         return resp;
354 375
     },
12  lib/OpenLayers/Tile/Image.js
@@ -208,11 +208,12 @@ OpenLayers.Tile.Image = OpenLayers.Class(OpenLayers.Tile, {
208 208
      * code.
209 209
      */
210 210
     positionTile: function() {
211  
-        var style = this.getTile().style;
  211
+        var style = this.getTile().style,
  212
+            size = this.layer.getImageSize(this.bounds);
212 213
         style.left = this.position.x + "%";
213 214
         style.top = this.position.y + "%";
214  
-        style.width = this.size.w + "%";
215  
-        style.height = this.size.h + "%";
  215
+        style.width = size.w + "%";
  216
+        style.height = size.h + "%";
216 217
     },
217 218
 
218 219
     /** 
@@ -256,11 +257,6 @@ OpenLayers.Tile.Image = OpenLayers.Class(OpenLayers.Tile, {
256 257
                 var top = this.layer.gutter / this.layer.tileSize.h * 100;
257 258
                 style.left = -left + "%";
258 259
                 style.top = -top + "%";
259  
-                style.width = (2 * left + 100) + "%";
260  
-                style.height = (2 * top + 100) + "%";
261  
-            } else {
262  
-                style.width = "100%";
263  
-                style.height = "100%";
264 260
             }
265 261
             style.visibility = "hidden";
266 262
             style.opacity = 0;
5  tests/Layer.html
@@ -764,7 +764,7 @@
764 764
     }
765 765
       
766 766
     function test_layer_setTileSize(t) {
767  
-        t.plan(6);
  767
+        t.plan(4);
768 768
 
769 769
         layer = new OpenLayers.Layer();
770 770
         
@@ -784,7 +784,6 @@
784 784
         var size = new OpenLayers.Size(2,2);
785 785
         layer.setTileSize(size);
786 786
         t.ok(layer.tileSize.equals(size), "size paramater set correctly to layer's tile size");
787  
-        t.ok(layer.imageOffset == null, "imageOffset and imageSize null when no gutters")
788 787
       
789 788
       //set on layer
790 789
         layer.tileSize = layerTileSize;
@@ -803,10 +802,8 @@
803 802
         size = new OpenLayers.Size(10,100);
804 803
         layer.setTileSize(size);
805 804
 
806  
-        var desiredImageOffset = new OpenLayers.Pixel(-15, -15); 
807 805
         var desiredImageSize = new OpenLayers.Size(40, 130); 
808 806
 
809  
-        t.ok(layer.imageOffset.equals(desiredImageOffset), "image offset correctly calculated");
810 807
         t.ok(layer.imageSize.equals(desiredImageSize), "image size correctly calculated");
811 808
     }
812 809
     
11  tests/Tile/Image.html
@@ -295,9 +295,6 @@
295 295
         t.ok(tile.layer.imageSize == null,
296 296
              "zero size gutter doesn't set image size"); 
297 297
 
298  
-        t.ok(tile.layer.imageOffset == null,
299  
-             "zero size gutter doesn't set image offset");
300  
-        
301 298
         var zero_gutter_bounds = tile.bounds;
302 299
         
303 300
         map.destroy();
@@ -312,8 +309,12 @@
312 309
                                                              tile.size.h + (2 * gutter))),
313 310
              "gutter properly changes image size"); 
314 311
 
315  
-        t.ok(tile.layer.imageOffset.equals(new OpenLayers.Pixel(-gutter, -gutter)),
316  
-             "gutter properly sets image offset");
  312
+        var offsetLeft = -(gutter / layer.tileSize.w * 100) | 0;
  313
+        var offsetTop = -(gutter / layer.tileSize.h * 100) | 0;
  314
+        t.eq(parseInt(tile.imgDiv.style.left, 10), offsetLeft,
  315
+             "gutter properly sets image left style");
  316
+        t.eq(parseInt(tile.imgDiv.style.top, 10), offsetTop,
  317
+             "gutter properly sets image top style");
317 318
         t.ok(tile.bounds.equals(zero_gutter_bounds),
318 319
              "gutter doesn't affect tile bounds");
319 320
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.