Thanks for the feedback Andrea. Comments inline.
On Sun, May 29, 2011 at 10:29 AM, Andrea Aime
<andrea.a...@geo-solutions.it>wrote:
> On Fri, May 27, 2011 at 5:02 PM, Justin Deoliveira <jdeol...@opengeo.org>
> wrote:
> > Hi all,
> > A while back we had a client who was interested in funding some labelling
> > work to solve a problem they were having with labels being drawn over
> > external graphics. As an example:
> > https://skitch.com/jdeolive/fbiss/label-before
> > Andrea had the idea of introducing the concept of labelling obstacles.
> More
> > specifically marking a symbolizer as a "label obstacle" via a vendor
> option.
> > And when present such symbolizers would be factored into the labelling
> > routine. Basically it would mark the area taken by the symbolizer as
> "busy"
> > so that no label would be drawn over top of it.
> > I have taken a crack at implementing this. The initial results look
> > promising:
> > https://skitch.com/jdeolive/fbish/label-after
> > The current patch can be found on this jira issue:
> > http://jira.codehaus.org/browse/GEOT-3610
> >
> > The patch is still rough at this point, but I wanted to post it to
> > supplement some of my questions:
> > 1. I had to utilize the deprecated constructor on StyledShapePainter that
> > accepts the LabelCache as a parameter, however that constructor has been
> > deprecated. Is that because rendering is now multi threaded and that the
> > painting actually goes on in a separate thread? So access to the label
> cache
> > should be limited to the primary/data loading thread?
>
> It because the painter had no use for the label cache until now, so that
> one
> was a dead method.
>
> > I am trying to analyze the rendering code to figure out if actually any
> race
> > conditions are introduced. And if I understand correctly I don't think
> there
> > are but I could definitely be missing something. With the new code the
> only
> > time the painting thread has to update the label cache is to put a new
> > reserved rectangle into the cache... which simply updates the reserved
> list.
> > And as far as I can tell this value is not accessed by the primary thread
> > until after it has joined with the paint thread...
>
> Yep, you're right, in practice what you're doing is not going to cause a
> thread safety issue. uDig is hitting the label cache from multiple threads
> in parallel, but afaik they are talking to a synchronized wrapper of it.
>
> > Anyways, some guidance here on how best to proceed would be appreciated.
> > 2. The patch adds support for marking a Line and Poly symbolizer as a
> label
> > obstacle. Not sure if this is a great idea since the method is bounding
> box
> > based hence we have to reserve the entire bounding box of lines and
> > polygons. Which you can imagine can lead to (depending on the shape of a
> > geometry) large spaces with no labels. So I am wondering if label
> obstacles
> > should be limited to point symbolizers?
>
> Eh, good question, not sure how to answer this one... for polygons it may
> be acceptable or not, depends a lot on the polygons, for lines indeed it's
> going to cause some grief... I don't have a good answer at the moment,
> the best way would be to rethink the collision algorithm to use a raster
> based
> approach instead of a spatial index, but it would also take some time to
> implement. In this approach the put(Rectangle) method would be replaced
> by a put(Shape, float strokeThickness, boolean fill) method that would
> paint on a black and white "busy area" surface as big as the map we're
> drawing.
>
> Right. So barring a better algorithm it would seem our choices are:
1. Allow line and poly symbolizers to be label obstacles and just warn users
that in many cases depending on the data this might lead to strange results.
Or
2. Limit to only point symbolizers for now... and once we do have raster
based collision detection enable it for line and poly
3. Forgo this patch until we have raster based collision
> > That is about it for now... general feedback appreciated... but be
> gentle...
> > this is my first rendering patch :)
>
> I know it's just a rough patch, yet let me also point out minor things I've
> seen
> while looking at the code:
> - text symbolizer vendor option map just become redundant, it can be
> removed
> - vendorOptionString in SLDParser should be capitalized (yeah, I know there
> are
> other constant there not following the proper naming convention,
> they should be
> fixed as well)
>
Cool, will do.
> - painting with graphic strokes with the conflict option turned on
> will add a lot
> of entries to the conflict map... I may be worrying too much, yet with
> small
> symbols we easily end up adding several thousands of entries to the
> conflict map...
>
Yeah... i thought about that as well. I will try to test an extreme case
and see what the impact is.
- at the moment there are no tests. Now, I know the testing situation
> in renderer
> land is scary, however the new ImageAssert facility should make it
> possible to
> add a test without too much effort, just use a built in font like Dialog
> and
> allow for pixel difference in the assert to account for platform
> dependencies.
>
Cool will do, i have code that I have been using to test the various cases,
i will turn those into runnable tests.
>
> Cheers
> Andrea
>
> --
> -------------------------------------------------------
> Ing. Andrea Aime
> GeoSolutions S.A.S.
> Tech lead
>
> Via Poggio alle Viti 1187
> 55054 Massarosa (LU)
> Italy
>
> phone: +39 0584 962313
> fax: +39 0584 962313
>
> http://www.geo-solutions.it
> http://geo-solutions.blogspot.com/
> http://www.youtube.com/user/GeoSolutionsIT
> http://www.linkedin.com/in/andreaaime
> http://twitter.com/geowolf
>
> -------------------------------------------------------
>
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
------------------------------------------------------------------------------
vRanger cuts backup time in half-while increasing security.
With the market-leading solution for virtual backup and recovery,
you get blazing-fast, flexible, and affordable data protection.
Download your free trial now.
http://p.sf.net/sfu/quest-d2dcopy1
_______________________________________________
Geotools-devel mailing list
Geotools-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geotools-devel
