Support UOM in the rendering process (a.k.a. variable size symbolizers)

Andrea Aime added a comment - 12/Mar/10 11:16 AM 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).

Milton Jonathan added a comment - 12/Mar/10 12:58 PM Hey there: - OK about the dynamic styles - Thanks a lot for clarifying about Expression multiplication with FilterFactory.multiply. Makes a lot of sense: it will remain an Expression to be evaluated with a Feature, but whatever comes out of it will be multiplied - About the tests: sure, we'll add at least one to test the Expression multiplication. And about the weird code in the test... yeah, it's weird, sorry. - We ended up applying the visitor in SLDStyleFactory because we wanted to make sure ALL symbolizers would be rescaled appropriately (unless otherwise stated), rather than changing code in both renderers, etc. So, I understand you prefer us to apply the visitor in all renderers, and anywhere everywhere it is needed, right? OK! Regarding doing work runtime wise, I see now that we did a stupid job here, in that we do not need to go through the visitor if the symbolizer's UOM is pixel or null, but that can be done. - About taking care of other UOM definitions (feet, etc.), check out UomRescaleStyleVisitor's lines 99-101: we use javax.measure's Converter class to do the job (rather clean in my view). Actually, there are a number of tests there for checking sizes given in NonSI.FOOT. - About the scale denominator: I guess I didn't express myself well. It's pretty easy to compute the map extent in meters (or kilometers), and that is exactly what's done in RendererUtilities.calculateScale(ReferencedEnvelope envelope, int imageWidth, int imageHeight, double DPI). What I wanted to say was that that particular method does everything needed to compute a pixels/meter ratio, but it uses the provided DPI to end up with a meters/meters scale denominator. It's thus a little stupid to then recompute the pixels/meter ratio by retrieving the DPI value again, but I guess it's not such a big deal.

Andrea Aime added a comment - 12/Mar/10 1:06 PM Apply the visitors in "all" renderers -> in the existing two, and it's one liner if you apply it to the style as a whole, and little more if you apply it to the active rules. It seems much less work to me, and much less disruptive. Where is other code that needs to apply the visitor? Sorry about the feet thing, I did not notice it was handled.

Milton Jonathan added a comment - 19/Mar/10 7:23 AM Made the changes (see reviewed path). I also reviewed UomRescaleStyleVisitor's code and added some tests after checking for their code coverage, which resulted in finding a deficiency in DuplicatingStyleVisitor.visit(LinePlacement). Hurray for testing with code coverage!.. But I was wondering a little about UomRescaleStyleVisitor: my original approach, when I only wanted to cover some specific cases, was to just implement visit() for symbolizers, which is where the UnitOfMeasure parameter is available. However, this way we have to explicitly rescale stroke, graphic, etc. everywhere. It would certainly be much better if we implemented rescaling methods visit(Stroke), visit(Graphic), etc., but then I guess we would need something like a "currentUnitOfMeasure" variable, that would have to be set by each visit(Symbolizer) method (and would be null by default, which means no UOM rescaling). I guess I would prefer it that way, what do you think?

Andrea Aime added a comment - 26/Mar/10 6:56 AM Sorry for the usual one week delay, I have time to look at these things only on Friday and there is a long list of patches and report to look at. I agree that keeping the current uom in a field so that you don't have to manually would be better, but you have to pay attention and reset it after you're done, otherwise it will rescale also the subsequent symbolizers that do not have a UOM. Something like: {code}   this.uom = symbolizer.getUOM();   try {     super.visit(symbolizer);   } finally {     this.uom = null;   } {code} The patch also looks better, there is still one bit that can be improved. With the current code you're cloning symbolizers for each feature, it would be better to do that just once, up front. I also noticed that the rescale code makes sure the width is at least one pixel. In fact we support float widths and I use often 0.5, and even if you set 0 something will be drawn (a super-tiny line) so please remove that check. I've prepared a modified patch with the above 2 modifications for you to review.  

Milton Jonathan added a comment - 26/Mar/10 4:28 PM Hey there Overall, patch looks good. I guess I can send later that change to UomRescaleStyleVisitor, so that it uses an internal UOM field. Thanks for the code snippet, I wouldn't have thought of using finally.. However, I disagree about the width issue. I've put that check because, with size 0, a line may be drawn, but somehow Graphic objects seem to be set to size 16 (the default size). And, I don't know exactly where, but there is code that casts sizes to integer values (since they should be pixels), so anything < 1 becomes 0. The result is that when zooming out a lot, suddenly tiny dots become large icons with size 16 :P So, my original understanding was that a size given in pixels should preferably be an integer value anyway. If not, then we need to track down places where size 0 is understood as the default size. I guess that would be the way to go then?

Andrea Aime added a comment - 27/Mar/10 2:26 AM No, a size in pixel is not bound to be an integer, all the java2d api uses floats and, as I already said, there are already users that extensively use non integer sizes to get the right visual effect. Plus, the SLD 1.0 specification is quite explicit, size is a floating point: {panel} The Size element gives the absolute size of the graphic in pixels encoded as a floating- point number. This element is also used in other contexts than graphic size and pixel units are still used even for font size. The default size for an object is context-dependent. Negative values are not allowed. {panel} same goes for "stroke-width": {panel} The "stroke-width" CssParameter element gives the absolute width (thickness) of a stroke in pixels encoded as a float. (Arguably, more units could be provided for encoding sizes, such as millimeters or typesetter's points.) The default is 1.0. Fractional numbers are allowed (with a system-dependent interpretation) but negative numbers are not. {panel} I see the problem with Graphics, that is a bug that needs fixing. It's probably in the SLDStyleFactory, I'll have a look.

Milton Jonathan added a comment - 29/Mar/10 8:42 AM OK, so if the bug can be easily tracked and killed, then I guess we're good to go! Hmm, just remembered another issue - not sure how things are working nowadays, but a while ago GeoTools would have problems when zooming in too much: a point symbolizer would become too large (once rescaled to pixels), and since GeoTools likes to cache a buffer image of it, it would attempt to instantiate a giant one and end up in an OutOfMemoryException.. If that's still an issue, maybe it could simply check the size of the image and decide not to cache it in case it's too large?

Andrea Aime added a comment - 29/Mar/10 8:44 AM It is still an issue, yeah. What we should do is to prevent "forever caching" and just use a soft hash map instead. That's something I've been meaning to do for quite some time, I guess it's time to do it for good.

Andrea Aime added a comment - 08/May/10 6:44 AM Ok, so yesterday I killed the ImageLoader and switched image caching to a synchronized soft cache (so no image will last forever)

Andrea Aime added a comment - 08/May/10 6:46 AM Ah, btw, if you keep zooming in the OOM will happen anyways because that single image is using all the memory, caching or not. It is difficult to do anything about it though...

Andrea Aime added a comment - 08/May/10 8:31 AM Made some more progress on this one, fixed an issue with the meta buffering computation (the meta buffer computation was called before rescaling, so zooming in enough on a point depicted with a mark you'd end up getting nothing, because the point was too far away from the rendering area). Also fixed the issue with images popping up at the default size when zooming out enough by making them of size 1 when the size goes below one (since a BufferedImage cannot be smaller than that). As for the OOM, it still happens as the scaled image gets generated (nothing to do with image caching though). Fixing that can be nasty. So, I looked around and using Graphics.drawRenderedImage does not incur in OOM issue. Sample program here: {code} public static void main(String[] args) throws IOException {         BufferedImage img = ImageIO.read(new File("/home/aaime/tmp/draw.png"));                  BufferedImage bi = new BufferedImage(1024, 768, BufferedImage.TYPE_4BYTE_ABGR);         Graphics2D g = bi.createGraphics();         AffineTransform at = AffineTransform.getScaleInstance(10000, 10000);         g.setRenderingHint(RenderingHints.KEY_INTERPOLATION, RenderingHints.VALUE_INTERPOLATION_BILINEAR);         g.drawRenderedImage(img, at);         g.dispose();                  ImageIO.write(bi, "png", new File("/tmp/test.png"));     } {code} So we could solve the problem by keeping the image as is, and drawing it with a rescale factor using drawRenderedImage. The thing is, since the rescaling is done on the fly, it has to be more expensive than scaling the image once and just blitting it. Sooo.... not sure what to do. One way could be to do add a scale factor to GraphicsStyle2D, to be used for drawRenderedImage. If the image rescaling is reasonable we do it in preprocessing and makr the style2d factory as 1, if the rescaling goes beyond a certain threshold (e.g., generates a 1024x1024 image or bigger) then we keep the image at its original size and set the scaling factory appropriately?

Andrea Aime added a comment - 08/May/10 10:19 AM Made a little synthetic test comparing the two approaches. Generally speaking, the current approach of pre-scaling is around 20 times faster than scaling during rendering, which is significant even taking into account the time it takes to load the data and so on. However, it's interesting to see how using a identity transform makes using drawRenderedImage somewhat faster. Sooo... I guess we'll have to adopt the differential strategy I was talking about above if we want to keep in both performance and stability

Andrea Aime added a comment - 09/May/10 8:16 AM Here is a patch that handles this the way depicted above, it basically falls back to vector rendering mode if the image to be rasterized is too big, using 512 pixels as the threshold for doing so. I also had to optimize a bit the vector mode graphic fill rendering path so that it would not loop almost forever to paint fills whose "tile" was extremely small compared to the geometry. Seems to work fine and at this point I'm confident to enough commit the path as is on trunk. Want to double check it before I go?

Milton Jonathan added a comment - 11/May/10 8:28 AM Great to hear that you're sorting out these final issues. Well, actually I couldn't apply the patch since it seems to have been generated from GIT and not SVN, but that's OK. In any case, it sounds like a very good solution to fall back to vector rendering if the image gets too large: after all, if it becomes THAT big, we'll probably only have a couple of icons on the screen anyway, so it probably won't hurt much not to use an image. About the optimization in the vector graphic fill, it's probably a good idea too. When I first implemented this, it was actually a LOT slower until I checked if the stipple intersected the clip area: I was dumbly rendering all stipples and then clipping them :P But yeah, it should be even better to eliminate lots of them right away and save a ton of calls to clipShape.intersects(). Nice! I would just suggest changing the final nStippleX and nStippleY variables to something like "toX" and "toY" since after the subtractions they're no longer nStipple anything :) Finally, about the issue with sizes < 1, it also sounds good to postpone this check to the renderer (I remember I originally checked this in the UomRescaleVisitor). Guess that's all, everything really nice.. Thanks for taking time to review this over, Andrea

Andrea Aime added a comment - 11/May/10 8:40 AM Patches generated by GIT only need to be slight massaging, on the command line I usually apply SVN patches as: patch -p0 < file.patch whilst the GIT ones need patch -p1 < file.patch Basically you tell patch to ignore the first element in the file paths.

Andrea Aime added a comment - 14/May/10 4:10 AM Cool. Attached a final patch that includes your suggestion about renaming the variable in the styled shape painter and adds a missing header.

Andrea Aime added a comment - 21/Jun/10 12:43 PM Enabled by default on trunk, committed to 2.6.x as well but needs to be enabled by setting a system variable, -DenableDpiUomRescaling=true

Andrea Aime added a comment - 28/Nov/10 2:57 AM Mass closing all issues that have been in "resolved" state for 2 months or more without any feedback or update