Hi Jody,
Thanks for the feedback. The information about the rendering optimizations
relative to the shapefile store help to explain things -- we were expecting
the logic in the streaming renderer. We will review the contributing and
pull request information and make sure that we are following the
appropriate procedures.
We will assess the use of a connection parameter for the shapefile to
control these behaviors and also make sure that everything is appropriately
documented. Our intent was really to throw this out as a proof-of-concept
in order to get feedback from the experts. We are happy to work through
the implementation and documentation, as is appropriate, but wanted to make
sure that we were going down the right path first.
As we discussed offline, the unit test is probably best moved closer to the
ScreenMap code (instead of the renderer). The current unit test is relying
on the filterBeforeScreenMap system property in order to influence how
ShapefileFeatureSource interacts with the ShapefileFeatureReader, passing
the filter reference when appropriate.
Eli
On Tue, May 6, 2014 at 12:47 PM, Jody Garnett <[email protected]>wrote:
> 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
>>
>>
>
--
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
