Hi Eli:
Sounds like you have done some good detective work. The screen map work was
originally part of a shapefile renderer which included many of these kind
of optimisations.
While I am not directly close to the shapefile code I can help by giving
you some background reading in terms of:
a)
contributing<http://docs.geotools.org/latest/developer/procedures/contribute.html>-
this has the contribution agreement we need on file in order to accept
a
patch from you
b) pull
request<http://docs.geotools.org/latest/developer/procedures/pull_requests.html>-
a new notes on how we use github pull requests
It looks like your commit introduces a new system property
"filterBeforeScreenMap". I would recommend making that an (optional)
connection parameter so that this decision can be made on a case-by-case
basis. At the very least if you are introducing a new system property you
would need to add a note to the shapefile documentation :)
I also do not understand the setFilter method being added to
ShapeFileFeatureReader? The method does not seem to be used by your patch,
and we prefer the readers to be set up during the constructor (so it is not
modified dynamically during the use of a reader).
Jody Garnett
On Tue, May 6, 2014 at 10:23 AM, Eli Miller <[email protected]> wrote:
> Hi GeoTools Developers,
>
> We have been using GeoTools for several years now and greatly appreciate
> the community effort that makes it such a successful project. During a
> recent project we noticed a GeoServer rendering anomaly and traced it back
> to the ScreenMap optimizations in ShapefileFeatureReader.
>
> We have implemented changes on a forked repository (
> https://github.com/emiller-planalytics/geotools) which reproduce the
> problem in the unit test FilteredScreenMapShapefileTest and also include a
> conceptual solution with backward compatible modifications to
> ShapefileFeatureReader and ShapefileFeatureSource. We have confirmed that
> these changes do not adversely affect existing unit tests.
>
> The crux of the problem is that the ScreenMap optimization currently
> filters out (screen) coincident features without consideration for
> subsequent filtering. This becomes apparent when ScreenMap filtering
> accepts a feature that will be rejected by a subsequent filter and rejects
> a subsequent, (screen) coincident feature that would be accepted by the
> later filter. In this situation a feature should be rendered but is not.
> The included test case uses several coincident points to illustrate the
> problem, although we believe that nearly coincident (with respect to
> original CRS) and polygon features could also exhibit this behavior.
>
> The proof-of-concept solution pushes the layer filter to
> ShapefileFeatureReader. When provided, the filter is evaluated before
> assessing the ScreenMap filtering. Because the filter evaluation in the
> forked repo is naive (it always assumes that attributes are required), the
> new execution path effectively removes the benefits of the ScreenMap
> optimization for any layers with filters. This could be improved by making
> the filter evaluation smarter in cases where a DBF read is not required.
> The changes do not include similar enhancements for
> IndexedShapefileFeatureReader but it is expected that this should be
> handled in a similar manner to ShapefileFeatureReader.
>
> I look forward to your thoughts on this matter. Pleas let me know if I
> can clarify anything. I will attempt to issue a pull request for reference.
>
> Best regards,
> Eli Miller
> --
>
> Eli Miller
> Director of Development / Enterprise Architect, Planalytics
> (W) 610.407.2930
>
> (M) 484.467.7926
>
> 920 Cassatt Road, Suite 300
> Berwyn, PA 19312
> [email protected]
> <http://www.planalytics.com/>
> *Predict. Perform. Profit*
> www.planalytics.com
> Follow Planalytics: [image: Twitter] <http://www.twitter.com/planalytics>
> [image:
> LinkedIn] <http://www.linkedin.com/company/planalytics> [image:
> YouTube]<http://www.youtube.com/planalytics>
>
> ***************************************************
> The information contained in this e-mail message is intended only for the use
> of the recipient(s) named above and may contain information that is
> privileged, confidential, and/or proprietary.
>
> If you are not the intended recipient, you may not review, copy or distribute
> this message. If you have received this communication in error, please notify
> the sender immediately by e-mail, and delete the original message.
> ***************************************************
>
>
>
>
> ------------------------------------------------------------------------------
> Is your legacy SCM system holding you back? Join Perforce May 7 to find
> out:
> • 3 signs your SCM is hindering your productivity
> • Requirements for releasing software faster
> • Expert tips and advice for migrating your SCM now
> http://p.sf.net/sfu/perforce
> _______________________________________________
> GeoTools-Devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>
>
------------------------------------------------------------------------------
Is your legacy SCM system holding you back? Join Perforce May 7 to find out:
• 3 signs your SCM is hindering your productivity
• Requirements for releasing software faster
• Expert tips and advice for migrating your SCM now
http://p.sf.net/sfu/perforce
_______________________________________________
GeoTools-Devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
