Frenando: I had a look again at your proposed interface, and it is not making sense to me, perhaps you could clarify:
public interface MarkFactoryProcessorProvider { public static final Hints.Key MARK_FACTORY_PROCESSOR_PROVIDER_KEY = new Hints.Key(MarkFactoryProcessorProvider.class); /** * Return the {@link MarkFactoryProcessor} corresponding to the provided feature. * * @param feature the feature to process or null if not needed. * @return the {@link MarkFactoryProcessor} instance. */ MarkFactoryProcessor get(Feature feature); } Questions: - Where is the matching MarkFactoryProcessor interface being returned by this provider? Checking GSIP-204 <https://github.com/geoserver/geoserver/wiki/GSIP-204> and it is not there either... I assume you mean something like this, if so please include in proposal: interface MarkFactoryProcessor { /** Used to filter and sort available factories */ Iterator<MarkFactory> process( Iterator<MarkFactory> factories ); } What was discussed further up in this chat was *not* having a MarkFactoryProcessor and instead using a hint to direction into DynamicSymbolFactoryFinder. Looking at the proposed GeoServer UI I may finally see why you would like a "MarkFactoryProcessor" - in order to have the ability to exclude factories. You are not only interested in ordering what is available; you would like to skip some also; so I added MARK_FACTORY_FILTER to the following example (new method marked in *bold*) class DynamicSymbolFactoryFinder { public static final Hints.Key MARK_FACTORY_ORDER = new Hints.Key(Comparator.class); public static final Hints.Key MARK_FACTORY_FILTER = new Hints.Key(Predicate.class); Iterator<MarkFactory> getMarkFactories() *Iterator<MarkFactory> getMarkFactories( Hints )* Iterator<ExternalGraphicFactory> getExternalGraphicFactories() Iterator<ExternalGraphicFactory> getExternalGraphicFactories( Hints hints) } The SLDStyleFactory getIcon method already shows how to use hints for icon lookup: private Icon getIcon(ExternalGraphic eg, Object feature, double size) { ... // scan the external graphic factories and see which one can be used Iterator<ExternalGraphicFactory> it = DynamicSymbolFactoryFinder.getExternalGraphicFactories(new Hints(renderingHints)); while (it.hasNext()) { ExternalGraphicFactory egf = it.next(); ... } ... } Using the same approach for getShape works cleanly: private Shape getShape(Mark mark, Object feature) { .... Iterator<MarkFactory> it = DynamicSymbolFactoryFinder.getMarkFactories(*new Hints(renderingHints)*); while (it.hasNext()) { MarkFactory factory = it.next(); ... } ... } If you only need to set this order per GetMap request the above should be sufficient. If you need to set this on a layer by layer basis then your lookup by feature would be required. -- Jody Garnett On Wed, 8 Sept 2021 at 08:04, Fernando Mino < fernando.m...@geosolutionsgroup.com> wrote: > Hi Jody, thanks for your feedback. > > The problem with the alphabetical default on class names would be we will > have SVG mark factory evaluated before WKT mark factories, which is the > current performance killer scenario. Our backward compatibility strategy > is to return the mark factory iterator as the original without any > filtering or ordering, allowing any mark factory extension to be present > and evaluated by the rendering code. > > To match our previous Tomcat version classloader behavior mark factories > order, which is alphabetical order by JAR module name class loading, we > need: > - Mark the mark factories to know the jar module name for every one, and > establish the default order using the jar name. Or, > - Add a priority field on the MarkFactory interface and implement the > appropriate priority on core mark factories to make the default iterator > follow the old tomcat classloader behavior. Or, > - Hard code the original previous Tomcat versions factories order as > default iterator result (maybe the worst idea). > > Probably this is out of the scope of this proposal, since it is only to > allow a custom filter and order by workspace configuration for Mark > Factories on GeoServer WMS GetMap, but I agree it would be great to reach > the original performance without the need of configuring it, I'm thinking > maybe is material for a new proposal only on GeoTools side to make this > default happen on the default factories code and define a good plan to > accomplish it. > > 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 9:17 AM Jody Garnett <jody.garn...@gmail.com> > wrote: > >> Aside; do we need to make the default value for this hint alphabetical? >> To match prior tomcat behavior letting folks upgrade without configuration >> change. >> -- >> Jody Garnett >> >> >> On Wed, 8 Sept 2021 at 07:07, Jody Garnett <jody.garn...@gmail.com> >> wrote: >> >>> 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