Thanks Fernando, i saw your PR yesterday and it indeed looks good (note it is still in draft).
The one feedback I had was to consider changing your implementation from gt-render MarkFactoryListPredicate to gt-metadata org.geotools.util.factory.FactoryListPredicate<? extends Factory> The functionality is based on the factory name and is not MarkFactory specific. -- Jody Garnett On Sun, 12 Sept 2021 at 14:14, Fernando Mino < fernando.m...@geosolutionsgroup.com> wrote: > Hi Jody, > > Thanks one more time for your detailed feedback and the proposal > alternative, it is a good one. I made an implementation based on it and it > works well, so I updated the GeoServer proposal as well and created the > draft PRs for the community evaluation and review. > > GeoTools PR: > https://github.com/geotools/geotools/pull/3634 > > GeoServer PR: > https://github.com/geoserver/geoserver/pull/5279 > > 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, Sep 8, 2021 at 8:34 PM Jody Garnett <jody.garn...@gmail.com> > wrote: > >> Fernando: >> >> Rather than watching communication go by: >> - Moved your proposal to the description section (you had the proposal >> outlined as tasks) and wrote in my assumption for MarkFactoryProcessor >> interface. >> - Wrote up alternate proposal more formally >> >> -- >> Jody Garnett >> >> >> On Thu, 12 Aug 2021 at 14:34, 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 >>>> >>>
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
