Extract interfaces from NetTopologySuite

[GoogleCodeExporter]

Hello chaps,

In order to ease the transition of NTS integration in SharpMap and to be able 
to use almost 'old' SharpMap.Geometries, I was wondering if you guys object if 
I extract 

- IBinaryGeometryReader from WKBReader
- IBinaryGeometryWriter from WKBWriter
- ITextGeometryReader from WKTReader
- ITextGeometryWriter from WKTWriter

and place them in GeoAPI.IO.
AFAICT many of the reader/writer classes in NetTopologySuite.IO projects could 
implement those interfaces as well (the binary ones).

cheers Felix

Original issue reported on code.google.com by felix.ob...@netcologne.de on 1 Nov 2011 at 5:00

[GoogleCodeExporter]

no objections, actually

Original comment by diegogu...@gmail.com on 2 Nov 2011 at 7:46

[GoogleCodeExporter]

+1 to extract interfaces for readers/writers, not sure if they should be in 
GeoAPI.

--- some other thoughts, not sure yet:

Do we need to distinguish between type of the reader (text / binary) on 
interface level or should it be IGeometryReader, IGeometryWriter? 

Should they be readers / writers or something like TypeConverter? 

... or even: 

interface IGeometry
{
  void FromWellKnownText(string text)
  void FromWellKnownBinary(byte[] byteArray)

  string ToWellKnownText();
  byte[] ToWellKnownBinary();
}

Original comment by Gennadiy.Donchyts on 2 Nov 2011 at 1:37

[GoogleCodeExporter]

Using the reader/writer approach we can parse not just only WKB/WKT 
representations of geometries, but also setup reader and writer for e.g. 
- SqlServerGeometry/-Geography, 
- PostGis geometry, 
- SpatiaLite geometry blob (GaiaGeo), 
- Oracle SDO
- GeoJSON
- ...

That way we can skip the conversion to and from WKB.

Please have a look at NetTopologySuite.IO solution folder, which contains 
reader/writer for the above.

The interfaces are currently set up like this:

#region reader interfaces
interface IGeometryReader<TSource, TStream>
{
    IGeometryFactory { get; set; }
    IGeometry Read(TSource source);
    IGeometry Read(TStream stream);
}

interface ITextGeometryReader : IGeometryReader<string, System.IO.TextReader> {}
interface IBinaryGeometryReader : IGeometryReader<byte[], System.IO.Stream> {}
#endregion

The interfaces for the writers are analogous.

On the IGeometry interface there are already 

   byte[] AsBinary() and 
   string AsText()

defined.

I think extending IGeometryFactory interface with

    IGeometry FromBinary(IBinaryReader reader, byte[] bytes) and
    IGeometry FromText(ITextGeometryReader reader string text)

would make more sense than adding to the IGeometry interface, since we cannot 
change the concrete geometry object (e.g. make Polygon a Point) and we cannot 
implement interfaces with static functions.

Original comment by felix.ob...@netcologne.de on 2 Nov 2011 at 2:25

[GoogleCodeExporter]

... right, Parse() in interface does not make much sense, in .NET it is part of 
concrete class like in .NET types. 

... convertion to WKB/WKT is in IGeometry and from WKT/WKB somewhere else, a 
bit confusing.

Sorry, I've missed IGeometryReader<TSource, TStream>, I use old version 

Is there an example somewhere demonstrating how they are supposed to be used? I 
find generic arguments <TSource, TStream> too implicit. It can be anything. 
Also if we already have stream, why do we need to specify some TSource? 
Implementation already knows how it is stored. 

... should example be *always* written demonstrating how API is supposed to be 
used before changes? TDD philosophy.

// for example, assume that we want to create geometries via factory:

void CreateGeometriesFromWellKnownFormats()
{
   var geometryFactory = ... how do we normally get factory without referencing NTS?

   // from WKT
   var point1 = geometryFactory.CreateGeometry<IPoint>("POINT(1.0 1.0)");

   // from WKB, BE
   var point2 = geometryFactory.CreateGeometry<IPoint>(byteArray, true);
}

interface IGeometryFactory
{
   ...
   public TGeometry CreateGeometry<TGeometry>(string wkt) : where TGeometry is IGeometry
   public TGeometry CreateGeometry<TGeometry>(byte[] wkb, bool bigEndian) : where TGeometry is IGeometry
   ...
}

Original comment by Gennadiy.Donchyts on 2 Nov 2011 at 9:30

[GoogleCodeExporter]

... I studied recent code in NTS.IO a bit more, now it makes more sense :).

The only concern is about TSource, TStream.  Will someone use these base 
interfaces?  Then the client code will look a bit messy:

 if(reader is IGeometryReader<string, TextReader>) { ... }.

Original comment by Gennadiy.Donchyts on 2 Nov 2011 at 10:14

[GoogleCodeExporter]

so +1 for extracting interfaces. 

We should check that working with interfaces is as simple as with concrete 
implementations. If we have code in WktReaderTest:

var reader = new WktReader(...);
reader.Read("POINT(1.0 1.0)");

... then it should be as simple to do the same with interface:

ITextGeometryReader reader = ... get it somewhere
reader.Read("POINT(1.0 1.0)");

-----
the same is true for writers:

IGeometryWriter oracleWriter = ...
IGeometry geometry = ...

oracleWriter.Write(geometry, stream);

Original comment by Gennadiy.Donchyts on 2 Nov 2011 at 10:23

[GoogleCodeExporter]

I tend to get my geometry factory implementation using 
http://commonservicelocator.codeplex.com

var factory = 
Microsoft.Practices.ServiceLocator.GetInstance<IGeometryFactory>();

I will add a sample asap.

Regarding the generic base interface:
- TStream is there because the WKT reader/writer implementation take TextReader
  and TextWriter while WKB reader/writer simply uses System.IO.Stream on both 
  interfaces.

- TSource/TOutput is there, to cover the cases where the result or input may be 
a
  concrete object (SqlGeometry, SqlGeography, some OpenMI.SpatialObject?) rather
  than an array of bytes.
  So MSSqlGeometryReader it would implement IGeometryReader<SqlGeometry, Stream>.
  But if there are objections: I'm not married with those generic base interfaces,
  we can revert that, and leave that functionality to the concrete implementation.

Original comment by felix.ob...@netcologne.de on 3 Nov 2011 at 9:03

[GoogleCodeExporter]

No, these interfaces look nice for implementation. But we probably never will 
know about concrete type such as SqlGeometry on GeoAPI level. So I'm thinking 
if we will need something more API-friendly in GeoAPI where no additional 
knowledge of a very specific type will be required? On the other hand it sounds 
useful to have from/to WKT/WKB conversion on API level since they are standards.

Maybe we should leave it as you think will be best for now and discuss again 
when we can meet in person :). 

If it will help to re-use code in NTS / SharpMap - (+1) to leave them in GeoAPI 
and discuss later how they can be improved. 

-------

... some more thoughts:

It looks like 2 tasks are beins solved here:

1) Convert IGeometry to/from another type (WKT string, GeoJSON string, WKB byte 
array, specific type like SqlGeometry). 

2) Actual IO, read / write goeometries using some data source (access file, 
access database, web service, etc.).

... just wondering if both of these requirements fit into reader / writer 
concept or is something more light-weight can be used in the first case. So 
that we can have an API concept to convert geometries to / from different 
representations and to read / write one or multiple geometries from different 
data sources, maybe specified using some Path (file path, connection string). 
The second feels more related to something like FeatureProviders where also 
attributes will be read / written ...

Original comment by Gennadiy.Donchyts on 3 Nov 2011 at 11:28

[GoogleCodeExporter]

+1 to parameterize on input and output types. However, I am opposed 
parameterizing based on different input types (as in TSource/TStream): I think 
we should settle on streams.

I'm also positive to the addition of FromText/FromBinary and changes to 
AsText/AsBinary as described above.

I'm glad to see this change as it enables a more pluggable architecture: we use 
custom-made WKT-reader and -writer.

Original comment by johan.li...@astando.se on 3 Nov 2011 at 1:07

[GoogleCodeExporter]

> However, I am opposed parameterizing based on different input types (as in 
> TSource/TStream): I think we should settle on streams.

You think the text interfaces should be
interface ITextGeometryReader : IGeometryWriter<string, System.IO.Stream> {};
interface ITextGeometryWriter : IGeometryWriter<string, System.IO.Stream> {};

So we can discard the System.IO.Stream part?

Original comment by felix.ob...@netcologne.de on 3 Nov 2011 at 2:00

[GoogleCodeExporter]

Personally I don't mind the <From,To> generic interfaces - though I think 
Stream and string are too broad as input or output types - how do you determine 
whether a string is GeoJson or Wkt without trying/ failing to parse it. same 
with stream and ~everything.

Personally I prefer the concept of conversion rather than reading /writing.  

Utilising the interfaces you can easily create a registry of converters which 
you can add to at runtime - you can also do it without the interfaces if you 
parameterize the registration of the converters.

E.g 

interface IConverter<TFrom, TTo>
{
    TTo Convert(TFrom obj);
}

class GeoJsonConverter : IConverter<GeoJsonStream, IGeometry> , 
IConverter<IGeometry, GeoJsonStream>
{
...
}
...

interface IConverterRegistry
{
    void Register<TFrom, TTo>(IConverter<TFrom, TTo> converter);
    bool CanConvert<TFrom, TTo>();
    TTo Convert<TFrom, TTo>(TFrom source);
    ...
}

IConverterRegistry converters = GetConverters();

converters.Register<GeoJsonStream, IGeometry>(new GeoJsonConverter());
converters.Register<IGeometry, GeoJsonStream >(new GeoJsonConverter());
converters.Register<WkbStream, IGeometry>(new WkbConverter());
converters.Register<IGeometry,WkbStream>(new WkbConverter());
converters.Register<WktStream, IGeometry>(new WktConverter());
converters.Register<IGeometry, WktStream>(new WktConverter());
converters.Register<SqlGeometry, WktStream>(...
.......

IGeometry g = GetGeometry();
IConverterRegistry converters =  GetConverters();

if(converters.CanConvert<IGeometry, WktStream>())
{
    WktStream s = converters.Convert<IGeometry, WktStream>(g);
}

where FooStream is either an type containing a Stream  property or a subclass 
of stream. If stream was used directly the registry would break down unless 
keyed with extra info.

hope that makes sense... got to go.. jd

Original comment by john.d...@newgrove.com on 3 Nov 2011 at 4:03

[GoogleCodeExporter]

I think it's best to keep it simple. Dealing with registrys of converters seems 
well out of scope for the problem at hand. I would think the most common use 
case is someone wanting to convert some GeoJSON or some WKB. 

For those use cases it would suffice with:

  IGeometryReader<TSource, TCoordinate> where TCoordinate : ... {
    IGeometry<TCoordinate> Read(TSource);
  }

And the writer:

  IGeometryWriter<TSink, TCoordinate> where TCoordinate : ... {
    void Write(IGeometry<TCoordinate> , TSink );
  }

That puts no restriction on source or sink types.

On top of that it's possible to engineer some register with converters, or 
whatever you want. If you want to put another level of abstraction wrt textual 
vs binary formats, i.e. ITextGeometryReader, that's fine (although I think that 
adds very little functionality).

Point being: keep it simple, please.

Original comment by johan.li...@astando.se on 4 Nov 2011 at 9:38

[GoogleCodeExporter]

>keep it simple, please.
I strongly agree with this

Original comment by diegogu...@gmail.com on 4 Nov 2011 at 9:55

[GoogleCodeExporter]

@gennadiy:
As a first step I'm only interested in reading and writing 
GeoAPI.Geometry.IGeometry instances from and to other spatial objects. The 
resulting reader/writers should be re-usable e.g. for SharpMap/(DotSpatial if 
it adheres to GeoAPI) data providers or NHibernate.Spatial. So I'm focused on 
just the geometry part.

@john:
Can't a converter registry can be setup using reader/writer?

@all:
I'm thinking about a way to control the ordinate output. IMHO we have two 
options, either use

  bool HandleOrdinate(Ordinate ordinate); // analogous to EmitZ, EmitM 

or a flag like approach

  Ordinates Ordinates { get; set; }

any suggestions/preferation?

Original comment by felix.ob...@netcologne.de on 4 Nov 2011 at 10:35

[GoogleCodeExporter]

@felix you can create a registry with reader/writer, it is just the semantics. 
If we are allowing conversion between types in addition to serialization and 
deserialization the I prefer the "conversion" concept as reading/writing 
implies serialization. The "Converter" may internally utilize a FooReader/ 
FooWriter in order to do its work when the source or target is a file.

Thinking aloud how about:

IDictionary<Ordinate, double> GetOrdinates(Ordinate ordinates)

Cheers jd

Original comment by john.d...@newgrove.com on 4 Nov 2011 at 11:20

[GoogleCodeExporter]

> Thinking aloud how about:
> IDictionary<Ordinate, double> GetOrdinates(Ordinate ordinates)

I thought of it more as a ForceCoordinate option (like in the v2 ShapeFile 
provider).

Original comment by felix.ob...@netcologne.de on 4 Nov 2011 at 11:33

[GoogleCodeExporter]

Agree with @jd on Conversion versus Writer/Reader. Actually they are also 
different in terms that a single converter is responsible for two-way 
conversion:

public interface IGeometryConverter
{
   ///
   /// actual type used during conversion, string, byte[] ...
   ///
   Type ConversionType { get; } 

   IGeometry ToGeometry<T>(TSource source);

   T FromGeometry<T>(IGeometry geometry);
}

public class GeoJsonGeometryConverter : IGeometryConverter
{
   IGeometry ToGeometry<T>(T source) { ... call ToGeometry(source as string) ... }

   T FromGeometry<T>(IGeometry geometry) { ... call FromGeometry(geometry) ... }

   // actual specific type implementations:

   IGeometry ToGeometry(string str) { ... }

   string FromGeometry(IGeometry geometry) { ... }
}

Original comment by Gennadiy.Donchyts on 4 Nov 2011 at 12:02

[GoogleCodeExporter]

Or maybe ConvesionType can be ommited and insted there should be:

public interface IGeometryConverter
{
   bool CanConvertGeometryFrom<T>(T source);
   bool CanConvertGeometryTo<T>(T target);

   IGeometry ConvertGeometryFrom<T>(T source);
   T ConvertGeometryTo<T>(IGeometry geometry);
}

Original comment by Gennadiy.Donchyts on 4 Nov 2011 at 12:06

[GoogleCodeExporter]

Ah right,  I like Flags :)

Original comment by john.d...@newgrove.com on 4 Nov 2011 at 12:08

[GoogleCodeExporter]

@gennadiy, john:
Can we agree on the IGeometryConvert thing being a different task? I hesitate 
to move away to far from the JTS source, since it may get harder to maintain or 
update, and the JTS.IO namespace works with readers/writers.

Original comment by felix.ob...@netcologne.de on 4 Nov 2011 at 12:23

[GoogleCodeExporter]

Fair enough, I just thought that ToBinary, ToText, FromBinary, FromText could 
delegate to a standardized Convert api. But in the interests of harmony that is 
fine :) 

Original comment by john.d...@newgrove.com on 4 Nov 2011 at 12:29

[GoogleCodeExporter]

@felix yes, sure, I feel that due to my comments this thread becomes an 
off-topic :).

Original comment by Gennadiy.Donchyts on 4 Nov 2011 at 1:50

[GoogleCodeExporter]

done with NTS 1.12

Original comment by felix.ob...@netcologne.de on 6 Mar 2012 at 2:49

• Changed state: Fixed