Hide
added a comment -
Looked a bit into the patch.
First a clarification about the dynamic styles: yes, as far as I know they are not used anymore, we should remove them. A long time ago symbolizers that depended on feature attributes were turned into these dynamic styles to avoid many object creations, then JVM became faster at dealing with garbage collection and we switched back, but the code that creates the dynamic styles was not removed. Since those methods are public we have to deprecate them first and remove them on trunk.
Now back to the patch. There are a few things that are not working or that could be done better.
The UomRescaleStyleVisitor does not seem to handle styles that do depend on feature own attributes. In general the rescaling code should check if the Expression is a Literal, in that case extracting the value using evaluate(null, Double.class) is the right approach. But if the Expression is not a Literal, it should be concatenated with a multiplication by the desired rescale factor instead (use FilterFactory.multiply to generate that).
All the modifications in SLDStyleFactory are not really necessary in my opinion. You should get the same result with less work, both coding wise and runtime wise, by applying the visitor in StreamingRenderer.processStylers (either against the original style, or, better, at least for big styles with many scale dependencies, against the active rules in the selected feature type styles). This should take care of the legend drawing as well, no need to modify it.
That also removes the need to work with scale ranges, since in that method you have the scale denominator already computed.
About the idea of computing the pixels per meter range directly from the world to scren transform... I thinking about it, but it does not add up for me. That transform goes from the units of the data to the screen, what if the data is expressed in feet, or worse, it's expressed in geographic coordinates? The scale factor of the affine transform is expressed in pixel/native unit, it's not a pure number (whilst the real map scale should be a pure number).
The geographic coordinates case is also important since the scale factor used in the WMS specification is "odd" as it basically assumes the Earth is a cylinder instead of a ball (yes, I know, it's wrong, but that's how the spec is made and it's there for good reasons), but to be consistent with the rest of the renderer, in my opinion you should start from the scale denominator as computed (if you don't specify the OGC compliance hint you'll get a proper scale, though it will also fail very quickly to compute the moment you get out of the area of definition of the current projection... which again, should not happen in a perfect world, but users are far from aware of these issues)
By the way, I don't see anywhere in the code some real units handling. What if the UOM specified in the symbolizer and the one in the data do not match? For example, what happens if the the user specifies the UOM in the symbolizer as feet?
Finally, there is an oddity in the UomRescaleStyleVisitorTest. Code like:
{code}
double rescaledSize = rescaledPolySymb.getStroke().getWidth().evaluate(filterFactory, Double.class);
{code}
does not make sense to me. Why pass the filterFactory as a parameter to evaluate? That method expects a Feature, or null if there is none available.
When fixing the visitor to be aware of generic expressions (functions, use of feature attributes) you should also make at least one test checking that the visitor builds the right multiplication, and you can also, eventually, build a feature that you can pass in those evaluates to check the results are ok (assuming a style that uses feature attributes, that is).
Show
added a comment - Looked a bit into the patch.
First a clarification about the dynamic styles: yes, as far as I know they are not used anymore, we should remove them. A long time ago symbolizers that depended on feature attributes were turned into these dynamic styles to avoid many object creations, then JVM became faster at dealing with garbage collection and we switched back, but the code that creates the dynamic styles was not removed. Since those methods are public we have to deprecate them first and remove them on trunk.
Now back to the patch. There are a few things that are not working or that could be done better.
The UomRescaleStyleVisitor does not seem to handle styles that do depend on feature own attributes. In general the rescaling code should check if the Expression is a Literal, in that case extracting the value using evaluate(null, Double.class) is the right approach. But if the Expression is not a Literal, it should be concatenated with a multiplication by the desired rescale factor instead (use FilterFactory.multiply to generate that).
All the modifications in SLDStyleFactory are not really necessary in my opinion. You should get the same result with less work, both coding wise and runtime wise, by applying the visitor in StreamingRenderer.processStylers (either against the original style, or, better, at least for big styles with many scale dependencies, against the active rules in the selected feature type styles). This should take care of the legend drawing as well, no need to modify it.
That also removes the need to work with scale ranges, since in that method you have the scale denominator already computed.
About the idea of computing the pixels per meter range directly from the world to scren transform... I thinking about it, but it does not add up for me. That transform goes from the units of the data to the screen, what if the data is expressed in feet, or worse, it's expressed in geographic coordinates? The scale factor of the affine transform is expressed in pixel/native unit, it's not a pure number (whilst the real map scale should be a pure number).
The geographic coordinates case is also important since the scale factor used in the WMS specification is "odd" as it basically assumes the Earth is a cylinder instead of a ball (yes, I know, it's wrong, but that's how the spec is made and it's there for good reasons), but to be consistent with the rest of the renderer, in my opinion you should start from the scale denominator as computed (if you don't specify the OGC compliance hint you'll get a proper scale, though it will also fail very quickly to compute the moment you get out of the area of definition of the current projection... which again, should not happen in a perfect world, but users are far from aware of these issues)
By the way, I don't see anywhere in the code some real units handling. What if the UOM specified in the symbolizer and the one in the data do not match? For example, what happens if the the user specifies the UOM in the symbolizer as feet?
Finally, there is an oddity in the UomRescaleStyleVisitorTest. Code like:
{code}
double rescaledSize = rescaledPolySymb.getStroke().getWidth().evaluate(filterFactory, Double.class);
{code}
does not make sense to me. Why pass the filterFactory as a parameter to evaluate? That method expects a Feature, or null if there is none available.
When fixing the visitor to be aware of generic expressions (functions, use of feature attributes) you should also make at least one test checking that the visitor builds the right multiplication, and you can also, eventually, build a feature that you can pass in those evaluates to check the results are ok (assuming a style that uses feature attributes, that is).
New Feature
Closed
Major
GEOT-1536
ImageLoader overhaul needed
First a clarification about the dynamic styles: yes, as far as I know they are not used anymore, we should remove them. A long time ago symbolizers that depended on feature attributes were turned into these dynamic styles to avoid many object creations, then JVM became faster at dealing with garbage collection and we switched back, but the code that creates the dynamic styles was not removed. Since those methods are public we have to deprecate them first and remove them on trunk.
Now back to the patch. There are a few things that are not working or that could be done better.
The UomRescaleStyleVisitor does not seem to handle styles that do depend on feature own attributes. In general the rescaling code should check if the Expression is a Literal, in that case extracting the value using evaluate(null, Double.class) is the right approach. But if the Expression is not a Literal, it should be concatenated with a multiplication by the desired rescale factor instead (use FilterFactory.multiply to generate that).
All the modifications in SLDStyleFactory are not really necessary in my opinion. You should get the same result with less work, both coding wise and runtime wise, by applying the visitor in StreamingRenderer.processStylers (either against the original style, or, better, at least for big styles with many scale dependencies, against the active rules in the selected feature type styles). This should take care of the legend drawing as well, no need to modify it.
That also removes the need to work with scale ranges, since in that method you have the scale denominator already computed.
About the idea of computing the pixels per meter range directly from the world to scren transform... I thinking about it, but it does not add up for me. That transform goes from the units of the data to the screen, what if the data is expressed in feet, or worse, it's expressed in geographic coordinates? The scale factor of the affine transform is expressed in pixel/native unit, it's not a pure number (whilst the real map scale should be a pure number).
The geographic coordinates case is also important since the scale factor used in the WMS specification is "odd" as it basically assumes the Earth is a cylinder instead of a ball (yes, I know, it's wrong, but that's how the spec is made and it's there for good reasons), but to be consistent with the rest of the renderer, in my opinion you should start from the scale denominator as computed (if you don't specify the OGC compliance hint you'll get a proper scale, though it will also fail very quickly to compute the moment you get out of the area of definition of the current projection... which again, should not happen in a perfect world, but users are far from aware of these issues)
By the way, I don't see anywhere in the code some real units handling. What if the UOM specified in the symbolizer and the one in the data do not match? For example, what happens if the the user specifies the UOM in the symbolizer as feet?
Finally, there is an oddity in the UomRescaleStyleVisitorTest. Code like:
{code}
double rescaledSize = rescaledPolySymb.getStroke().getWidth().evaluate(filterFactory, Double.class);
{code}
does not make sense to me. Why pass the filterFactory as a parameter to evaluate? That method expects a Feature, or null if there is none available.
When fixing the visitor to be aware of generic expressions (functions, use of feature attributes) you should also make at least one test checking that the visitor builds the right multiplication, and you can also, eventually, build a feature that you can pass in those evaluates to check the results are ok (assuming a style that uses feature attributes, that is).