Hello Andrea Thanks a lot and sorry for having this take you so much time. I will take a close look at the patch (it's against 2.6.x right?). But, in general, I think whatever modification you do in the code is fine with me (provided it still works, of course ;) ).
About some of your remarks: - Sorry for the formatting, I didn't notice that the new files were formatted according to our own code conventions. I will really try to avoid that in the future.. - About synchronizing, I thought it was synchronizing on the node instance, which is not so bad. But of course, setting it on the Graphics should be muuch better (a little ashamed that I didn't think of that) - Funny thing, I really thought I still saw SVGGlyphRenderer around on 2.6.0. Anyway, I'll see how it works with ExternalGraphicFactory, Icons, etc. Again, whatever you change is more than OK: our approach was to try to fit our changes messing as little as possible the original code. But of course a bigger re-write (and re-organization of the original code) is always better. - Sorry about breaking the Style2D limits with the GlyphStyle2D. I guess this was once again the cleanest way we came up with for doing this without changing much original GeoTools code. I'll see how the changes work here and give you feedback when possible. Thanks again for all Milton Andrea Aime wrote: > Hi Milton, > today I've tried to look into and apply your patch. > > After six hours of work I think I've reached some success, but I'll > need your help double checking what I've done. > > The reason for such a long work is, basically, that the patch is not > applicable as is for a number of reason, so I had to change it quite > a bit: > *) it does not respect normal GeoTools formatting > *) it uses synchronization in the middle of rendering hindering > significantly scalability > *) it breaks the division of work between Style2D, that only carries > around information, and StyledShapePainter, that does the actual > painting > *) it uses SVGGlyphRenderer, which is gone on 2.6.x since the 2.6.0 > release and does not work with ExternalGraphicFactory and Icon > which are its replacement > > Let me elaborate on each point. > > Formatting > -------------------------------- > > I know the rendering subsystem source code looks like a battlefield. > It is, it's the result of half a dozen developers hacking on the code > for years, each attempt with a specific scope that did not allow a > full cleanup (a rewrite). > > However, GeoTools still has code conventions. Your patch contains > wrong placement of curly braces and marks fields with the _ prefix, > something that is not used in the rest of the code. > When providing patches for GeoTools please follow the exisisting coding > convention (see our dev guide). > > Synchronization > ------------------------------- > > This is a bit of a pet peeve of mine, and I take full responsibility > for the change. Basically I've modified the code so that the Batik > node is not modified for the sake of rendering, but the Graphics > transform is instead. > This should allow GeoServer to scale up much better under concurrent > PDF generation request (because, yes, I was to use your changes right > away ;-) ) > > Division of work > ------------------------------- > > Not much to say here, all styles limit themselves to carry only Java2D > information that the StylerShapePainter uses to do the actual rendering. > So I've moved the method you created in GlyphStyle2D back into > StyledShapePainter and factored out the code to create the path iterator > > Usage of GlyphRenderer vs Icon > ------------------------------- > > GlyphRenderer is simply put going the way of the Dodo. > It has always been a local hack that one cannot plug into and I'm > slowly phasing it out. > > When people asked SVG support to be factored out of the renderer module > I've also removed SVGGlyphRenderer, so the patch missed that file. > > However, the SVG paths still work fine by using SVGGraphicsFactory, > which in preparation for the work you did does not return an Image, but > an Icon, that is, something that can draw itself, and can draw itself > in a vector way if it wants to. > > So I changed you patch to create an SVG based icon, preserving the > same caching you provided. Icon does not know anymore about all > the transforms applied to Graphics, and besides, the rest of the > code applies the transforms during the painting, not inside the > icon, so I've moved displacement and rotation handling in > StyledShapeRenderer instead. > > I've also added a bit of testing (interactive, visual, not really > automated unfortunately) in the svg module. > > Conclusion > ------------------- > > Your patch has been changed quite a bit, though I've tried hard to > preserve all the fundamentals of it. It should be still working fine, > but I really need you to check it throughly to see if I've forgot > anything or if there are mistakes introduced by me. > > Looking forward to apply these improvements to GeoTools > > Cheers > Andrea > -- Milton Jonathan Grupo GIS e Meio Ambiente Tecgraf/PUC-Rio Tel: +55-21-3527-2502 ------------------------------------------------------------------------------ This SF.Net email is sponsored by the Verizon Developer Community Take advantage of Verizon's best-in-class app development support A streamlined, 14 day to market process makes app distribution fast and easy Join now and get one step closer to millions of Verizon customers http://p.sf.net/sfu/verizon-dev2dev _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
