SLD maps minScaleDenominator and maxScaleDenominator to rule minScale and maxScale

Replying to tschaub:

I could be off, but something looks fishy here. The SLD format parses values for minScaleDenominator and maxScaleDenominator and maps them directly to rule.minScale and rule.maxScale. In Style.js, when rules are evaluated, the map scale is compared to rule.minScale and rule.maxScale. The style is displayed if map.getScale() > rule.minScale or map.getScale() < rule.maxScale (for the last applied rule with a minScale or maxScale property).

The code in Style.js means that the feature is displayed if scale > rule.minScale AND scale < maxScale (at least it is supposed to, and maybe I am missing something here). And the SE spec says that the feature is to be displayed if scale_denom >= minScaleDenominator && scale_denom < maxScaleDenominator. So the check to determine if we are in range is correct, but the way it is applied is wrong (see below).

Maybe the wording is misleading. In OpenLayers, layer.min/maxScale can be either scale or scale denominator. In SLD, and also for style.min/maxScale, they have to be denominators. And map.getScale also returns the denominator, so there should be nothing wrong with the calculations in OpenLayers.Style.

In SE, when a scale denominator >= minScaleDenominator, the rule applies. In OL when the map scale (denominator) <= minScale, a layer is in range. Similarly, in SE, when a scale denominator < maxScaleDenominator (not inclusive), the rule applies. In OL, when map scale >= maxScale (inclusive), a layer is in range.

Rule.min/maxScale and Layer.min/maxScale were designed to be different things, because of the following analogy: for server-side SLD (WMS layers), the min/maxScaleDenominators define which features should be rendered and which not. Still, in OpenLayers, a user can define a minScale/maxScale range at which the layer should be displayed at all. So the min/maxScale of the feature style can be seen independently of the min/maxScale of the layer -- layer overrides feature.

Finally, in SE, the scale denominator is relative to the "standardized rendering pixel" - that is, a 0.28mm x 0.28mm pixel (1 px / 90.714 in). In OL, scale is relative to DOTS_PER_INCH (typically 72).

This is a good point. That conversion has to be added to Format.SLD.

So, it looks to me like there are three problems here: 1) minScaleDenominator should map to rule.maxScale (and same for ...)

IMHO no, because rule.maxScale actually is a scale denominator. I will add more tests for OL.Style to see if this works correctly.

2) scales should be converted to reflect the 0.28mm pixel

Yes.

3) one of minScale or maxScale should not be inclusive

minScale should be inclusive.

The third problem is an issue with layer.calculateInRange. I imagine it will be seen as breaking the API to change that.

IMO, the current behaviour is correct, because it is analog to WMS layers.

Also, I'm a bit confused by the handling of rule min/maxScale. Currently, a style is extended with symbolizers from all rules that evaluate to true. The min/maxScale check comes after the evaluate test. So, if 10 rules apply (evaluate to true) but only one is in "range" given the current scale, doesn't the style still get extended with all 10 symbolizers? And, isn't the style.display property set based only on the last applied rule that has minScale or maxScale?

You are right, this behaviour is terribly wrong. I can't believe that I overlooked that. According to the SE spec, min/maxScaleDenominator determines if the rule should apply at all, so the scale check would have to be done before every rule to see if it applies. It also means that a feature is only to be rendered hidden if no rule applies at a given scale.

I know that this second bit deserves a separate ticket if it represents a real issue. I was just hoping that Andreas might tell me why it is not and we could dismiss it.

I am sorry, but this is a real issue. But those things are all related, so maybe this ticket should just get a different name and cover all of the issues.

Replying to ahocevar:

The code in Style.js means that the feature is displayed if scale > rule.minScale AND scale < maxScale (at least it is supposed to, and maybe I am missing something here). And the SE spec says that the feature is to be displayed if scale_denom >= minScaleDenominator && scale_denom < maxScaleDenominator. So the check to determine if we are in range is correct, but the way it is applied is wrong (see below).

Sorry, the SE spec says that the rule applies if scale_denom >= minScaleDenominator && scale_denom < maxScaleDenominator.

This patch makes the minScale check inclusive and changes the behavior how rules are applied wrt min/maxScale. The diff of the test also shows that the SE spec was completely misinterpreted previously. Problem 2) is not yet addressed with this patch.

After reading the SE spec again, I would say we do not need to recalculate scale denominators:

If the map-rendering software has information available about the actual pixel size of the final display device, then an extra processing step will be needed (if the actual pixel size is different from the standard pixel size) to adjust the actual rendering scale to calculate the standard rendering scale [...]

OpenLayers has no information available about the pixel size, it only makes an assumption. So we do not need to adjust the scales. Also, it would be quite unintuitive to the user to have scale denominators in SLD that would not match the scales retrieved from OL.Map.getScale().

Replying to ahocevar:

Replying to tschaub:

So, it looks to me like there are three problems here: 1) minScaleDenominator should map to rule.maxScale (and same for ...)

IMHO no, because rule.maxScale actually is a scale denominator. I will add more tests for OL.Style to see if this works correctly.

Ok, I'll state it another way. I agree that there is not a problem with the rule code now (I only glanced at the patch). The "issue" I was getting at is that in OL, minScale is greater than maxScale. Yes, the values are scale denominators, but the min/max semantics refer to the scale fraction. So, rule.minScale is different than layer.minScale - the "min" prefix in the rule property corresponds to a small scale denominator and the "min" prefix in the layer property corresponds to a small scale fraction.

I think the rule property names are more intuitive. Just think we should acknowledge that the names should not be confused (but are going to be confused) with the identically named layer and map properties. If we want to avoid confusion, rule properties should be named minScaleDenominator and maxScaleDenominator. Even though the values of both refer to the scale denominator, at least this would distinguish rule properties from layer/map properties.

I know this is sticky business.

Replying to ahocevar:

After reading the SE spec again, I would say we do not need to recalculate scale denominators:

If the map-rendering software has information available about the actual pixel size of the final display device, then an extra processing step will be needed (if the actual pixel size is different from the standard pixel size) to adjust the actual rendering scale to calculate the standard rendering scale [...]

OpenLayers has no information available about the pixel size, it only makes an assumption. So we do not need to adjust the scales. Also, it would be quite unintuitive to the user to have scale denominators in SLD that would not match the scales retrieved from OL.Map.getScale().

I won't belabor this, because I think the SE spec makes it more confusing than it needs to be.

But, just as example, if a casual user has SLD with a minScaleDenominator of 100000 (for example) and another layer with a maxScale of 100000, the feature will not be diplayed (or the rule not applied) and the layer will not be in range for scales (denominators) below 100000. This is good. It makes sense.

If a picky user was strictly interpreting the SE spec and set OpenLayers.DOTS_PER_INCH to 120 (for example) - or say they just knew that OpenLayers assumed a default value - then they would assume that the minScaleDenominator in their SLD was going to be corrected by a strict client.

Anyway, I think this is a trivial issue.

Replying to tschaub:

I know this is sticky business. Replying to ahocevar:

After reading the SE spec again, I would say we do not need to recalculate scale denominators:

If the map-rendering software has information available about the actual pixel size of the final display device, then an extra processing step will be needed (if the actual pixel size is different from the standard pixel size) to adjust the actual rendering scale to calculate the standard rendering scale [...]

The fact is that if map-rendering software is going to deal with scale they *have* to assume something about pixel size, whether or not they know the "final display device" pixel size.

Replying to tschaub:

I think the rule property names are more intuitive. Just think we should acknowledge that the names should not be confused (but are going to be confused) with the identically named layer and map properties. If we want to avoid confusion, rule properties should be named minScaleDenominator and maxScaleDenominator. Even though the values of both refer to the scale denominator, at least this would distinguish rule properties from layer/map properties.

This is a very good proposal. Actually, I had this wording in earlier versions of the Style code, but changed it just for shortness. Will upload a patch with the reverted wording shortly.

I am setting this to review, noting the following:

• the proposed patch changes the API properties min/maxScale to min/maxScaleDenominator, but those properties were introduced after 2.5, so it should be possible to do that. At least, it would be highly desireable (see Tims comment above).
• the scale is not re-computed, we stick with the assumption of DPI from OL to stay consistent with map.getScale().

I would be greatful for any quick comment or review. It would be good to have this corrected before 2.6, because then it will not be possible anymore to resolve the min/maxScale(Denominator) confusion.

New patch with additional tests for minScale/maxScale, plus introduced ElseFilter in Format.SLD. With the corrected rule processing logic, the ElseFilter is crucial. Also renamed minScale and maxScale rule properties to minScaleDenominator and maxScaleDenominator for clarity and destinction from layer.maxScale and layer.minScale

Thanks for the work on this. I could use a bit of explanation on the ElseFilter bit. My understanding is that an else filter *only* applies if none of the other filters apply. If I read this patch correctly, an else filter is treated the same way as a rule with no filter - that is it will always apply.

Now, there are plenty of ways we don't follow the SE spec (we extend styles instead of painting one style on top of another), but I think this is one way that we should conform. If someone goes to the trouble of putting an else filter in their sld (paint a big red dot if no other filters apply), they might be able to get a big read dot extended with symolizers from other rules that apply.

Am I misreading something?

At some point, I'd like to see features get a style map with keys that mapped to arrays of styles. I'm sure this will get crschmidt's hackles up, but I'd rather have simple constructors and complex objects than just simple objects (I'm talking here about a more complex style class that has a nice simple constructor for people who only want orange lines).

If anybody else thinks this is worthwhile (the ability to have multiple styles painted on top of one another - say a wide dark orange line with a lighter orange line over it to symbolize a highway or a shadow under a graphic), we will likely start mapping rules to an array of "styles" (symbolizers) instead of extending styles with symbolizers from all rules that apply.

All this makes me want to talk more real-time instead of just commenting on patches.

One other minor comment, I'd suggest

userStyle.rules.unshift(rule)

instead of

userStyle.rules = [rule].concat(userStyle.rules)

Replying to tschaub:

Thanks for the work on this. I could use a bit of explanation on the ElseFilter bit. My understanding is that an else filter *only* applies if none of the other filters apply. If I read this patch correctly, an else filter is treated the same way as a rule with no filter - that is it will always apply.

It is kind of a simplification, like that we extend styles instead of painting a feature for every rule that applies. The ElseFilter rule is added at the beginning of the array of rules, so it will be parsed first. If no other rules apply, the ElseFilter will apply in its full beauty. If other rules apply, those will extend the ElseFilter. This conforms to the SE spec as long as all properties used by the ElseFilter exist in at least one of the other rules. If not, then the ElseFilter will have remainders that are not overwritten by the other rules. In that cases, it is not conform to the SE spec.

Now, there are plenty of ways we don't follow the SE spec (we extend styles instead of painting one style on top of another), but I think this is one way that we should conform. If someone goes to the trouble of putting an else filter in their sld (paint a big red dot if no other filters apply), they might be able to get a big read dot extended with symolizers from other rules that apply.

Yes, on the rare occasion that no other rules overwrite "big", "red" or "dot". But you are right, we can do better on that.

At some point, I'd like to see features get a style map with keys that mapped to arrays of styles. I'm sure this will get crschmidt's hackles up, but I'd rather have simple constructors and complex objects than just simple objects (I'm talking here about a more complex style class that has a nice simple constructor for people who only want orange lines).

This is actually something that I wanted to address in the solution to #1120, by using a hash of styles with rendering intent names as keys in OL.Feature.Vector.

If anybody else thinks this is worthwhile (the ability to have multiple styles painted on top of one another - say a wide dark orange line with a lighter orange line over it to symbolize a highway or a shadow under a graphic), we will likely start mapping rules to an array of "styles" (symbolizers) instead of extending styles with symbolizers from all rules that apply.

With the consequence that vector features would have attached features (or at least geometries) to paint the stacked styles. This could maybe be done in a way similar to the editing vertices of the ModifyFeature control.

All this makes me want to talk more real-time instead of just commenting on patches.

And I'm glad that we will have an opportunity to do so soon.

One other minor comment, I'd suggest {{{ userStyle.rules.unshift(rule) }}} instead of {{{ userStyle.rules = [rule].concat(userStyle.rules) }}}

Thanks, I knew that there was something better than mine, but I did not find it.

Should we pull Format.SLD out of the trunk until we come up with a re-design, or should we apply the patch for this ticket as a first step and see that we can accomplish the rest of the task before the 2.6 release cycle?

In the new patch, I made two improvements:

• ElseFilter is now what it should be
• OL.Style.createStyle has now an options array as second parameter. If we pass extend: false with this parameter, an array of symbolizers that applied for the passed feature will be returned, instead of a single style that is extended with every added style.

So now everything should be prepared for complex style rendering (i.e. features with more than one style, with each one drawn above the other).

New patch that represents the current state of the discussion

All tests pass in FF2 and IE6.

current patch doesn't apply for me, this might

Same patch as the one from Chris, but parseable by trac.

Added a new patch that does not introduce the options array to Style.createStyle, and removes the baseStyle property of this method (because it is not really needed).

This patch should contain the portions of the previous patch that Tim was in favour of.

I am raising the priority of this ticket to major. The reason is that the more people out there are using SLD and Style, the less it will be accepted to change API behaviour and properties (although those were not in a released version yet).

The proposed patch works, tests pass in FF and IE. I think that we neither make anything worse, nor do we make it harder to do further work towards a StyleMap object by applying this patch.

Hey Andreas - Thanks for the continued work on this. All looks good to me with one small exception (that you are probably aware of). I'll raise that in another ticket.

I agree that this improves things, augments your totally cool style work so far, and sets us up for style maps on layers.

(In [5964]) SLD maps minScaleDenominator and maxScaleDenominator to rule minScale and maxScale. r=tschaub (closes #1297)