Allow styles to override feature geometries

[ahocevar]

With this change, application developers are able to define styles that
render a different geometry than the feature geometry. This can e.g. be
used to render an interior point of a polygon instead of the polygon, or
to render symbols like arrows along lines.

[tsauerwein]

What about using a function instead of a concrete geometry (or both)? Something like:

/**
 * @type {ol.geom.Geometry|function(ol.geom.Geometry):ol.geom.Geometry|undefined}
 */
olx.style.StyleOptions.prototype.geometry;

The function would receive the original geometry and can return a new geometry that will be used for the rendering.

This would make it even more powerful.

[tsauerwein]

... or to render symbols like arrows along lines.

I would be interested in how you would do that with this approach. From the original linestring you would somehow calculate the positions of the arrows. So instead of a linestring you now have a MultiPoint. But to render the arrows, you would also need the rotation (for each arrow). How would you do that?

[ahocevar]

I'm working on a different approach now. Similar to what @tsauerwein suggested, but the function will not be a property of the style, but of ol.layer.Vector and ol.FeatureOverlay, just like the styleFunction. It will be called with feature, resolution and style.

[ahocevar]

@tsauerwein To get an arrow for each segment of a linestring, you could configure your layer with a styleFunction like this:

function(feature, resolution) {
  var styles = [];
  var pointsArray = feature.getGeometry().getCoordinates();
  for (var i = pointsArray.length - 1; i >= 1; --i) {
    styles.push(new ol.style.Style({
      geometry: new ol.geom.Point(/* calculate coordinates from segment */ ),
      image: new ol.style.Icon({
        rotation: /* calculate rotation from segment's gradient */
        // other style properties here
      });
    }));
  }
  return styles;
}

[elemoine]

If the styles are always the same for a given feature it would be a shame to recalculate them each time. One option is to cache the styles in the feature:

function(feature, resolution) {
  var styles = [];
  var pointsArray = feature.getGeometry().getCoordinates();
  for (var i = pointsArray.length - 1; i >= 1; --i) {
    styles.push(new ol.style.Style({
      geometry: new ol.geom.Point(/* calculate coordinates from segment */ ),
      image: new ol.style.Icon({
        rotation: /* calculate rotation from segment's gradient */
        // other style properties here
      });
    }));
  }
  // cache the styles in the feature
  feature.setStyle(styles);
  return styles;
}

feature.setStyles(styles) will schedule a re-render but it may be ok.

[elemoine]

@ahocevar I agree that this PR brings more flexibility than #3022.

[gberaudo]

There is already the method feature.setGeometryName for using an alternative geometry.
Are we going to deprecate it?

[ahocevar]

@gberaudo feature.setGeometryName is still useful - in the context of WFS transacations as well as in the case where you just want to set a specific geometry for rendering, and use static styles.

[ahocevar]

If the styles are always the same for a given feature it would be a shame to recalculate them each time. One option is to cache the styles in the feature

Another option would be to configure a createFeatureFunction on the source's format, which sets the style on each feature right before rendering.

I think I'll consider @tsauerwein's suggestion to use a function instead or in addition to an ol.geom.Geometry. Then it will be easier to use geometries that are stored on the feature already, e.g. feature.get('geom2') of feature.get('features')[0].getGeometry(). The latter would work for clusters. The former could even be simplified if the new geometry property can also be a string pointing to a feature property. Then the former would just be 'geom2'.

Ok, and now on to the examples.

[gberaudo]

For describing arrows positions along a multiline or each segments, this approach will require computations from the developer and storing the world coordinates (eg lonlat coordinates) and derivatives for each point.

Though this approach is straightforward and flexible, it is also very inefficient for long/many multilines, creating tens of thousands of styles, one for each arrow point.

On the contrary, a declarative approach would be compact, and use a single static style.
I would rather introduce a small set of style attributes for describing the positioning of an image along a multiline or it segments. For example, the following could describe the positioning of an arrow based on an image, on the middle of each segment, oriented according to derivative between the segments points:

{
  position: ('50%',
  image: new ol.style.Circle(),
  orientation: ' derivative'
}

A declarative approach would also make interaction with external libraries or services like ol3-cesium and mapfish print much easier.

[ahocevar]

@gberaudo Your suggestion above would definitely make sense for arrows along lines. The arrow use case is just something that came to my mind, I'm not going to create an example for it. There are other use cases that are perfectly covered by the changes this pull request introduces.

[tsauerwein]

orientation: ' derivative'

See also how this is handled with CartoCSS:

marker-placement: point line interior

Attempt to place markers on a point, in the center of a polygon, or if markers-placement:line, then multiple times along a line. 'interior' placement can be used to ensure that points placed on polygons are forced to be inside the polygon interior

[ahocevar]

This is now ready for review, with example and unit tests. @tschaub, I think you wanted to see an example, so it would be great if you could take a look at it.

[ahocevar]

Any final review would be much appreciated. Thanks!

[ahocevar]

Thanks for the thorough review @elemoine. I have addressed all your comments. Couple good catches among them.

[tschaub]

I think this adds a lot of flexibility in a way that fits in pretty naturally with the current ol.style.Style object. Given the ongoing discussion about making style getters exportable, I think it would make sense to decide on how someone would get a geometry given a style.

Alternative 1

Make getGeometryFunction() exportable. Example usage:

var func = style.getGeometryFunction();
var geom = func(feature);

Alternative 2

Add a getGeometry() function. Example usage:

var geom = style.getGeometry(feature);

The first would be a minor change but gives us yet another function named *Function (a personal peeve). The second is (subjectively) more natural to use (I mean who really is going to do anything with the return from getGeometryFunction except for calling it?) but is arguably surprising because it lacks symmetry with setGeometry.

My preference would be to add style.getGeometry(feature), make it exportable, and use it internally as well (instead of style.getGeometryFunction()(feature).

[tschaub]

I think you've lulled the compiler to sleep here. I mean how can it possibly know that this function returns a geometry?

[ahocevar]

Oh, I see your point @tschaub. The function's body should be

var result = feature.get(geometry);
return goog.isDef(result) ? result : null;

Really strange that the compiler does not catch this.

[ahocevar]

Or I should accept that the function may return undefined and change the types accordingly.

[ahocevar]

@tschaub, is af30b88 what you had in mind?

[tschaub]

Oh I was meaning that feature.get(geometry) could return any type (assuming geometry is a user provided property name and the value could be anything. If the compiler were more strict about this, we'd want an assertion in there that the return was ol.geom.Geometry (instead of anything else that is possible to store in a feature).

Sorry for the confusion. I know you've already pushed ahocevar@af30b88 - but I think it isn't necessary.

This is what I was talking about:

var feature = new ol.Feature({foo: 'bar'});
var style = new ol.style.Style({
  geometry: 'foo',
  ...
});

[ahocevar]

No worries, I can remove that commit again - but now I need some sleep 😄

[ahocevar]

Ok @elemoine, but don't you agree that there is not much use in maintaining a style_ member only to be able to have a getStyle() getter that returns exactly that? This is what @tsauerwein meant in his comment.

Both @tschaub and I agree that having a get/set* pair which is not a real getter/setter is not elegant because it lacks symmetry. So would everyone be at least somewhat happy if I just rename getGeometry to geometryFunction?. Then we still have Function in the name, but it's not a method but a property of ol.style.Style. And we don't have to do something weird like getGeometryFunction().call(style, feature), but can just do geometryFunction(feature).

[ahocevar]

Another way to make everyone happy would be to just rename setGeometry to setGetGeometry or setGeometryFunction.

[ahocevar]

I'm happy now.

[elemoine]

I am sorry but I still don't understand why we would not have setGeometry/getGeometry/getGeometryFunction as for ol.Feature. @ahocevar, would you remove ol.Feature#getStyle if it was not @api stable? Are you saying getStyle is useless?

[ahocevar]

I think what application developers do with an ol.layer.Vector or ol.Feature is different from what they do with an ol.style.Style. For the former two, you'll mostly want to set a new style to change the appearance of the layer. For the latter, you'll need to get the styling information when you want some other component to render or synchronize (e.g. MapFish Print or ol3-cesium). So in my opinion, a different API would be ok for working with geometries from ol.style.Style, because the use cases are different than for working with styles from ol.Feature or ol.layer.Vector.

Regarding the question whether ol.Feature#getStyle should be removed: all I'm saying is that I removed ol.style.Style#getGeometry because I agreed with @tsauerwein's comment that it is not needed any more. And then I saw that none of our examples use ol.Feature#getStyle or ol.layer.Vector#getStyle. And that the geometry_ member is not used in any of these classes.

[tschaub]

If possible, it would be great to see some code examples of what people think makes for a nice API.

Here is mine:

// create a style with a point geometry
var style = new ol.style.Style({
  geometry: new ol.geom.Point([1, 2])
});
var point = style.getGeometry();

// create a style with a function that creates a geometry given a feature
var style = new ol.style.Style({
  geometry: function(feature) {
    // return a geometry based on the feature
  }
});
var geometry = style.getGeometry(feature);

[elemoine]

I think what application developers do with an ol.layer.Vector or ol.Feature is different from what they do with an ol.style.Style. For the former two, you'll mostly want to set a new style to change the appearance of the layer. For the latter, you'll need to get the styling information when you want some other component to render or synchronize (e.g. MapFish Print or ol3-cesium). So in my opinion, a different API would be ok for working with geometries from ol.style.Style, because the use cases are different than for workino with styles from ol.Feature or ol.layer.Vector.

I am not convinced :-) If I set a geometry into a style I want to be able to call getGeometry to get a reference to that geometry and change it (style.getGeometry().setCoordinates(coords)). So I don't see much difference with getStyle really, and do not understand why we would not make this consistent. And the fact that we have no example using setStyle does not mean it is useless, IMO.

This is just opinion @ahocevar and I certainly don't want to block this PR if I'm the only one with this opinion.

[ahocevar]

@elemoine:

If I set a geometry into a style I want to be able to call getGeometry to get a reference to that geometry and change it (style.getGeometry().setCoordinates(coords)

var style = new ol.style.Style({
  geometry: new ol.geom.Point([1, 2]);
});
// change the geometry
style.getGeometry().setCoordinates([3, 4]);
// The geometry you configured with your style now has the coordinates [3, 4]

Note that the above is with the pull request as it is now.

[tschaub]

@elemoine can you provide some example code that you think would make for a good API?

[elemoine]

Note that the above is with the pull request as it is now.

Not really. I can't do style.getGeometry() if I compile my code because the @param type is ol.Feature. The param is not optional.

My API:

// create a style with a point geometry
var style = new ol.style.Style({
  geometry: new ol.geom.Point([1, 2])
});
var point = style.getGeometry();

// create a style with a point geometry
var style = new ol.style.Style({
  geometry: new ol.geom.Point([1, 2])
});
var geometry = style.getGeometryFunction()(feature);

// create a style with a function that creates a geometry given a feature
var style = new ol.style.Style({
  geometry: function(feature) {
    // return a geometry based on the feature
  }
});
var geometry = style.getGeometry()(feature);

// create a style with a function that creates a geometry given a feature
var style = new ol.style.Style({
  geometry: function(feature) {
    // return a geometry based on the feature
  }
});
var geometry = style.getGeometryFunction()(feature);

I think this is consistent with setStyle/getStyle/getStyleFunction.

[elemoine]

I just updated my code snippets. There were mistakes.

[ahocevar]

Not really. I can't do style.getGeometry() if I compile my code because the @param type is ol.Feature. The param is not optional.

FWIW, if you configured your style with an ol.geom.Geometry as geometry, then you can, because the generated geometry function does not require any argument:

function() {
  return geometry;
};

[tschaub]

// create a style with a function that creates a geometry given a feature
var style = new ol.style.Style({
geometry: function(feature) {
// return a geometry based on the feature
}
});
var geometry = style.getGeometry()(feature);

The thing I don't like about this sort of thing is that it is awkward for anybody except the setGeometry caller.

This forces anybody who wants something more predictable to use the even more awkward

style.getGeometryFunction()(feature)

or even weirder

// if the geometry function expects `this` to have special meaning
style.getGeometryFunction().call(style, feature);

[tschaub]

I think this is apparent, but what I'm hoping is that we can reserve the "nice" or "sensible" method names for the most common/useful/flexible purposes. Having sat with this for a while (and revisiting #1690), I'm also compelled by the consistency argument.

In the end, I think both style.getGeometry() and style.getGeometryFunction() will see little use outside the library. So I hope I won't have to cringe very often when explaining why someone has to call style.getGeometryFunction().call(style, feature). If this were going to be a more common thing to do, I hope we could consider a more "ergonomic" API.

So would be fine by me to remove a few commits here and make it so:

• the geometry option is {undefined|string|ol.geom.Geometry|ol.style.GeometryFunction},
• the setGeometry method accepts the same,
• the getGeometry method returns what was passed as an option or provided to setGeometry,
• the getGeometryFunction method returns the function we use internally, and
• the above three methods are exportable.

@ahocevar whaddya think?

[elemoine]

or even weirder
// if the geometry function expects this to have special meaning
style.getGeometryFunction().call(style, feature);

How about not calling call internally? If the user wants a specific this for her "geometry function" then she should use bind to pre-bind her function to the object.

For example:

var style = new ol.style.Style({
  // ...
});
style.setGeometry(function(feature) {
  // "this" is a reference to the style object
  // ...
}.bind(style));

This would also make the application code more explicit.

[ahocevar]

Ok, I'm going to make these changes and merge.

[elemoine]

Are you going to remove the call to call as well? I think we should. And we should also remove it when calling the feature style function.

[ahocevar]

Yes, I'm going to remove all that.