Thanks Fernando I appreciate you taking the time to revise the proposal.
+1

--
Jody Garnett


On Mon, 6 Sept 2021 at 08:09, Fernando Mino <
fernando.m...@geosolutionsgroup.com> wrote:

> Thanks Jody for your hints.
>
> Indeed, using rendering hints to pass the pre-process behavior to GeoTools
> makes sense, it helps us to avoid adding a new extension point and I added
> this approach to the proposal.  Since we need different behavior
> per-workspace we'll make rendering evaluate the current feature, and make
> GeoServer cache the per-feature-namespace result during the current
> rendering task in progress for performance optimization.
>
> The proposals were updated for your revision and feedback:
>
> https://github.com/geotools/geotools/wiki/Rendering-pre-process-Mark-Factories-Hint
> https://github.com/geoserver/geoserver/wiki/GSIP-204
>
>
> Regards,
>
> Fernando Mino
>
> ==
>
> GeoServer Professional Services from the experts!
>
> Visit http://bit.ly/gs-services-us for more information.
>
> ==
>
> Fernando Mino
>
> Software Engineer
>
> @fmy2kec
>
> GeoSolutions Group
> phone: +39 0584 962313
>
> fax:     +39 0584 1660272
>
> https://www.geosolutionsgroup.com/
>
> http://twitter.com/geosolutions_it
>
> -------------------------------------------------------
>
> Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE
> 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si
> precisa che ogni circostanza inerente alla presente email (il suo
> contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è
> riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il
> messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra
> operazione è illecita. Le sarei comunque grato se potesse darmene notizia.
>
> This email is intended only for the person or entity to which it is
> addressed and may contain information that is privileged, confidential or
> otherwise protected from disclosure. We remind that - as provided by
> European Regulation 2016/679 “GDPR” - copying, dissemination or use of this
> e-mail or the information herein by anyone other than the intended
> recipient is prohibited. If you have received this email by mistake, please
> notify us immediately by telephone or e-mail.
>
>
> On Fri, Aug 13, 2021 at 12:30 AM Jody Garnett <jody.garn...@gmail.com>
> wrote:
>
>> Right, sorry I missed that requirement earlier.
>>
>> For the workspace, or GetMap, use case that is a clear need for using a
>> Hint (pass your comparator in as a hint and make a getMarkFactories( hints
>> ) method; it probably already should of had one.
>>
>> Jody
>>
>> On Thu, Aug 12, 2021 at 2:34 PM Fernando Mino <
>> fernando.m...@geosolutionsgroup.com> wrote:
>>
>>> Hi Jody,
>>>
>>> Thanks for the insight on Factory registry as a better alternative.
>>> Indeed the DynamicSymbolFactoryFinder.getMarkFactories() ->
>>> FactoryRegister.setOrdering(..) approach will do the trick for one of the
>>> scenarios we want to support (I will amend the proposal to add this to
>>> support the global configuration use case) but we also have the
>>> per-workspace use case that allows:
>>> - To have several different MarkFactory ordering and filtering on
>>> several workspaces.
>>> - To have several WMS GetMap requests (and rendering threads) in
>>> parallel, rendering Mark shapes with different MarkFactory implementations
>>> order and availability profiles.
>>>
>>> Probably I'm wrong (and please let me know if indeed there is a way to
>>> achieve this with DynamicSymbolFactoryFinder.getMarkFactories() ->
>>> FactoryRegister.setOrdering), but looks like the per-workspace
>>> configuration + multithreading support is not possible with FactoryRegister
>>> due to its not-thread-safe, globally cached and synchronized-static-methods
>>> caller(DynamicSymbolFactoryFinder).
>>>
>>> To use FactoryRegistry we would need to change
>>> DynamicSymbolFactoryFinder to allow us to store and index different
>>> FactoryRegistry instances per workspace, but the problem here is GeoTools
>>> Rendering module is not aware of GeoServer workspaces, and that is why I
>>> proposed the on-the-fly filtering and ordering interceptor extension point,
>>> to allow us to control this filtering from GeoServer code during request
>>> runtime.
>>>
>>> Naturally any better idea to solve this second use case is welcome.
>>>
>>> Regards,
>>>
>>> Fernando Mino
>>>
>>> ==
>>>
>>> GeoServer Professional Services from the experts!
>>>
>>> Visit http://bit.ly/gs-services-us for more information.
>>>
>>> ==
>>>
>>> Fernando Mino
>>>
>>> Software Engineer
>>>
>>> @fmy2kec
>>>
>>> GeoSolutions Group
>>> phone: +39 0584 962313
>>>
>>> fax:     +39 0584 1660272
>>>
>>> https://www.geosolutionsgroup.com/
>>>
>>> http://twitter.com/geosolutions_it
>>>
>>> -------------------------------------------------------
>>>
>>> Con riferimento alla normativa sul trattamento dei dati personali (Reg.
>>> UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si
>>> precisa che ogni circostanza inerente alla presente email (il suo
>>> contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è
>>> riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il
>>> messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra
>>> operazione è illecita. Le sarei comunque grato se potesse darmene notizia.
>>>
>>> This email is intended only for the person or entity to which it is
>>> addressed and may contain information that is privileged, confidential or
>>> otherwise protected from disclosure. We remind that - as provided by
>>> European Regulation 2016/679 “GDPR” - copying, dissemination or use of this
>>> e-mail or the information herein by anyone other than the intended
>>> recipient is prohibited. If you have received this email by mistake, please
>>> notify us immediately by telephone or e-mail.
>>>
>>>
>>> On Wed, Aug 11, 2021 at 8:11 PM Jody Garnett <jody.garn...@gmail.com>
>>> wrote:
>>>
>>>> Thanks for doing this up as a proposal, it really helps with discussion.
>>>>
>>>> I did not think that MarkFactory or ExternalGraphicFactory needed to be
>>>> aware of priority as this is handled by FactoryRegistry; but you are
>>>> correct none of the existing implementations extend AbstractFactory.
>>>>
>>>> Your proposal is on MarkFactory; do you need a similar change for
>>>> ExternalGraphicFactory?
>>>>
>>>> I am not quite sure how MarkFactoriesProcessor  is intended to work:
>>>>
>>>>    - processFactories( Iterator<MarkFactory> factories ):
>>>>    Iterator<MarkFactory>
>>>>    Processing an iterator on the fly is good, it amounts to doing pair
>>>>    wise ordering, or a comparator, which is already supported by
>>>>    FactoryRegistry.
>>>>    - priority(): int
>>>>    What is the int for? Is it to order between competing instances
>>>>    of MarkFactoriesProcessor ?
>>>>
>>>>
>>>> My hesitation is seeing more than one way to do things, especially if
>>>> it is a one-off for a specific factory.  So I would like to see what the
>>>> limitation you are running into with the existing setup, and if your
>>>> proposal is an improvement we could do it for all FactoryFinders. We can
>>>> look at other factory finders for examples of helper methods used to
>>>> address common challenges (factory iterator providers for injecting
>>>> instances,  establishing pairwise ordering
>>>> <https://github.com/geotools/geotools/blob/06d379a2fdbfdbf94641c16813c37ef02aa7f63c/modules/library/referencing/src/main/java/org/geotools/referencing/ReferencingFactoryFinder.java#L546>,
>>>> ...). I know that establishing pairwise ordering is a pain, and there is
>>>> already a helper method that takes a comparator
>>>> <https://github.com/geotools/geotools/blob/main/modules/library/metadata/src/main/java/org/geotools/util/factory/FactoryRegistry.java#L1265>
>>>> to make this easier.
>>>>
>>>> Checking your proposal you reference propose changing the code here
>>>> <https://github.com/geotools/geotools/blob/2c0f22f0a5a13885aa1d85faa6f715ea87f3c90e/modules/library/render/src/main/java/org/geotools/renderer/style/SLDStyleFactory.java#L1362>
>>>> :
>>>>
>>>> *Iterator<MarkFactory> it =
>>>> DynamicSymbolFactoryFinder.getMarkFactories();*
>>>>
>>>>
>>>> I would ask instead that you change DynamicSymbolFactoryFinder
>>>> directly; that interface is already responsible for SPI discovery,
>>>>
>>>> Here is a proposed alternative approach:
>>>>
>>>> 1. Leave StyleFactory alone (it already has too much logic) and
>>>> trust DynamicSymbolFactoryFinder.getMarkFactories() to respect your
>>>> ordering, especially since you are trying to work around how factory
>>>> discovery is based on classpath order.
>>>>
>>>> 2. Create a method DynamicSymbolFactoryFinder setMarkFactoryOrder(
>>>> Comparator<MarkFactory> comparator ):
>>>>
>>>>     public static synchronized
>>>> Iterator<MarkFactory> setMarkFactoryOrder( Comparator<MarkFactory>
>>>> comparator ) {
>>>>         getServiceRegistry().setOrdering( MarkFactory.class, comparator
>>>> );
>>>>     }
>>>>
>>>> 4. This calls FactoryRegister.setOrdering( category, comparator )
>>>> <https://github.com/geotools/geotools/blob/main/modules/library/metadata/src/main/java/org/geotools/util/factory/FactoryRegistry.java#L1265>
>>>>  which
>>>> should do the trick.
>>>>
>>>>
>>>> --
>>>> Jody Garnett
>>>>
>>> --
>> --
>> Jody Garnett
>>
>
_______________________________________________
GeoTools-Devel mailing list
GeoTools-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geotools-devel

Reply via email to