SharpMap Uml

Topics: SharpMap v0.9 / v1.x
Coordinator
Jun 30, 2012 at 8:54 PM

I've spent some time analyzing design of the SharpMap in trunk, please have a look at the diagrams here: SharpMap Architecture v1.0 then see below my comments:

1. Map

1.1. Map.VariableLayers - what is this?!?

1.2. Why Map.Layers is a BindingList (very implementation specific, I think we should use some observable collection interface of .NET 4 instead)

1.3. ICanQueryLayer - hmm, sounds like a bad design decision, we can already query data on IProviders, right? YAGNI (remove it!)

2. ILayer issues

2.1. Layers is a Collection<Layer> and Map.Layers is a BindingList<ILayer> - massy, make both IObservableList<ILayer>

2.2. BaseVectorLayer<TGeometry>, AnyGeometryVectorLayer, LinealVectorLayer, PolygonalVectorLayer, PuntalVectorLayer - do we really need it, it seems to duplicate VectorLayer? Is't it easier to use something like ISymbolizer in VectorLayer? (StrategyPattern, define non-generic IGeometrySymbolizer or IGeometryRenderer for this to use in non-generic VectorLayer). 
BaseVectorLayer<> + derived layers sounds like an overkill (derived classes don't add a new behaviour really) compare to the VectorLayer. 

2.3. VectorLayer.Themes - what is this?!? There is a Theme property already. If we need to remember a number of themes - it should be somewhere else.

3. IProvider issues

3.1. FilterProvider - if we need filtering on provider leven - it must be added on IProvider and not like it is done now.

3.2. BaseProvider - is it supposed to be used by all provider implementations? Currently SqlServer2008 and PreparedGeometryProvider use it and all other provider don't use it.
What is PreparedGeometry, sounds like some implementation details?!?!

3.3. GeometryFeatureProvider and GeometryProvider: looks like one of the most important architectural rules were violated. Every class / interface must have a single responsibility, there should be no other class with the same responsibility!

3.4. SqlServer2008 vs. MsSql - what are the differences?
... looks like it makes sence to have ...Provider suffix in all IProvider implementations to avoid confusions?

Coordinator
Jul 2, 2012 at 7:10 AM

Hello Gena thanks for your effort. Some clarification/annotation

1.1 VariableLayers is intended as a list of layers whose content changes frequently (e.g. vehicle tracking). Its layers are always rendered on top of all others therfore, if the viewport don't change you can cache the images of Background- and LayersCollection and just render the "variable ones".

1.2 I have no preference/objections on changing that.

1.3 Not all layers have a Datasource (Gdal/BruTile) and in fact ILayer doesn't even expose IProvider. This interface is there to be able to distinguish the layers that can be queried from the ones that can't be.

2.1 Again, I have no objections.

2.2 Assigning the symbolizer is what I did in the first place, but the way VectorLayer and VectorRenderer are set up the strategy cannot be chosen once for the whole layer rendering cycle. It is done on every geometry. Therfore the distinction in Puntal/Lineal/Polygonal. It gives better performance. One of the implemented symbolizers converts all geometries to their graphics path's and then applies a series of symbolizers on the graphics paths. This really boosts performance compared to the current VectorLayer approach.

2.3 Havn't noticed that.

3.2 BaseProvider/PreparedGeometryProvider (WorkInProgress) Introduced with NTS Geometries. The base provider should be base class for all providers. It should ensure that they all have a valid geometry factory that matches the spatial reference id. As for the Db providers we should rely on their query capabilities to return required features, there are others that don't have that (e.g. custom providers, shapefile) that need to rely on e.g. NTS. When relying on NTS it is best to use NTS PreparedGeometries of for spatial predicates, hence the PreparedGeometryProvider.

3.3 We should remove geometryprovider in favor of a geometryFeature provider with a featuredatatable containing just ID and geometry.

3.4 I can live with that.

 

 

Coordinator
Jul 2, 2012 at 7:44 AM

Hi, 

My comments

1.1, As Felix wrote.. they are supposed to enhance performance for "trackinglayers"

1.2 OK for me, event better maybe to use something generic as IList<ILayer> ?

1.3 Nice abstraction for example for WMS / GetFeatureInfo implementations or Identify implementations of RasterLayers where there is no Datasource in that sense

2.1 or maybe IList ? Do we need to be able to expose the collection as obserable, or could we instead add an event on the MapClass for LayerAdded, LayerRemoved or somehting?

2.3 I also saw that and have no idea what it should be used for

3.4, First of all we should remove those that are marked as Obsolete (MsSql etc.). Agree with the ...Provider naming rule

Developer
Jul 20, 2012 at 4:30 PM

 


--- 1.1. Map.VariableLayers.

alternatives: Add a property to ILayer like 'Variable', or perhaps the term 'Dynamic' is better. Or work with a DataChanged event mechanism and set an Invalidated property on that event. A general remark: Caching of what is rendered is a performance optimization and should not have a big impact on the core architecture. 

 

--- 1.2. Why Map.Layers is a BindingList (very implementation specific, I think we should use some observable collection interface of .NET 4 instead)

ok i guess.

 

--- 1.3. ICanQueryLayer - hmm, sounds like a bad design decision, we can already query data on IProviders, right? YAGNI (remove it!)

In WMS you need a distinction between the image and the feature info. Perhaps there are other alternatives we could discuss on Skype.

 

--- 2.1. Layers is a Collection<Layer> and Map.Layers is a BindingList<ILayer> - massy, make both IObservableList<ILayer>

or just IList, like peter mentions. If we can keep it simple we should.


--- 2.2. BaseVectorLayer<TGeometry>, AnyGeometryVectorLayer, LinealVectorLayer, PolygonalVectorLayer, PuntalVectorLayer - do we really need it, it seems to duplicate VectorLayer? Is't it easier to use something like ISymbolizer in VectorLayer? (StrategyPattern, define non-generic IGeometrySymbolizer or IGeometryRenderer for this to use in non-generic VectorLayer). BaseVectorLayer<> + derived layers sounds like an overkill (derived classes don't add a new behaviour really) compare to the VectorLayer. 

It sounds like this should be simplified.


--- 2.3. VectorLayer.Themes - what is this?!? There is a Theme property already. If we need to remember a number of themes - it should be somewhere else.

In Mapsui I have a single IEnumerable<IStyle> where IStyle could be an IThemeStyle. 


--- 3.1. FilterProvider - if we need filtering on provider leven - it must be added on IProvider and not like it is done now.

need to take a look


--- 3.2. BaseProvider - is it supposed to be used by all provider implementations? Currently SqlServer2008 and PreparedGeometryProvider use it and all other provider don't use it.What is PreparedGeometry, sounds like some implementation details?!?!

Lets remove it

--- Provider suffix in all IProvider implementations to avoid confusions?

I agree

Developer
Aug 29, 2012 at 2:37 AM
FObermaier wrote:
1.3 Not all layers have a Datasource (Gdal/BruTile) and in fact ILayer doesn't even expose IProvider. This interface is there to be able to distinguish the layers that can be queried from the ones that can't be.

I think all layer should already query data on IProviders.  If datasource not support this, we can add no support alert in to the method in datasource.

I developed edited fearture methods for some datasource and I realized we should not add a ICanEditedLayer :). It makes the class inherits more complex in our source code.

Coordinator
Aug 29, 2012 at 2:05 PM

If I understood you correctly, the suggestion is to:

Have querying / editing functionality next to IProvider (IFeatureProvider) - I completely agree with this. ILayer uses IProvider in order to render it on a Map. It is not supposed to provide editing / quering functionality.

 

A bit more thoughts:

Layers query (iterate) features from their providers based on the current envelop (what to render).

I'm currently reviewing feature editing functionality (Delta Shell branch) and also getting feeling that IFeatureEditor is something to have introduced very close to IProvider (IFeatureProvider).

Map interaction, required for feature editors, is another thing. It is something to keep very close to map controls.

---

In the first place we should try to stick to SOLID principles (http://en.wikipedia.org/wiki/SOLID_(object-oriented_design).  Single responsibility is what we have to check here. The responsibility of the ILayer is to "render geospatial features for a given envelope / map using given theme and data source". Adding feature editing, querying, etc. would be an overkill (too much responsibility).

 

 

 

Developer
Aug 30, 2012 at 3:42 AM
gennadiy_d wrote:

If I understood you correctly, the suggestion is to:

Have querying / editing functionality next to IProvider (IFeatureProvider) - I completely agree with this. ILayer uses IProvider in order to render it on a Map. It is not supposed to provide editing / quering functionality.

Yes, Exactly my suggestion. In other my project was designed by the way.

Developer
Oct 6, 2012 at 1:51 PM
Edited Oct 6, 2012 at 1:56 PM
gennadiy_d wrote:

2.3. VectorLayer.Themes - what is this?!? There is a Theme property already. If we need to remember a number of themes - it should be somewhere else.

The way it looks, VectorLayer.Themes is used exclusively by WMS Service

will it be a ok to try something like

public static Dictionary<string, Dictionary<string, ITheme>> LayerThemes

in wms server the first string being layer name and second being theme name