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 <
[email protected]> 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 <[email protected]>
> 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 <[email protected]>
>> 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 <
>>> [email protected]> 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 <[email protected]>
>>>> 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 <
>>>>> [email protected]> 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 <[email protected]>
>>>>>> 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
