Adding support for WKT strings to MapServer...

[sdlime]

This is the parent bug for the proposal to add WKT (Well Known Text) support to 
MapServer as described in RFC 2.

[sdlime]

*** Bug 948 has been marked as a duplicate of this bug. ***

[sdlime]

*** Bug 1115 has been marked as a duplicate of this bug. ***

[dmorissette]

Steve, is RFC2 completed in 4.8 or will it come only in a later release?

[sdlime]

The majority is already in 4.8. Both Frank and I have added wrapper for GEOS 
and OGR. There is a new WKT keyword in the feature object of a mapfile so you 
can create features via the mapfile or via URL. The swig MapScript interface 
includes the ability to use WKT via the shapeObj constructor and the shapeObj 
has a toWKT() method.

Todo's:

  - WKT via PHP/MapScript
  - WKT format support for mapshape CGI parameter
  - updating the Python tests

We were rushing to beat the feature freeze and I forgot to update this bug. 
Sorry.

Steve

[szekerest]

Steve,

You should keep the original constructor in shape.i, beacuse the current 
modification breaks the compatibility with the existing code in c#, because it 
cannot handle default values.

Tamas Szekeres

[sdlime]

I don't know. Very few people use those bindings and it seems a shame to 
convolute things because of a SWIG problem. There really isn't a good 
alternative other than perhaps using the original constructor for C# and the 
new one for everyother language. Ideas? If we have to break things then this is 
the time to do it.

Steve

[szekerest]

Steve,

I think using multiple constructors (function overloading) is not a bad 
concept  depending on those parameters that are needed for the initialisation. 
Many of the languages highly play upon this feature. If you intend not to 
provide a default constructor it implies that this object should be 
initialized from a wkt most of the time, but I think it's not the case. The 
shapeObj can also be created by adding lineObj after the initialisation.

So for this case a static createFromWKT() would be more appropriate.

After all if you decide to keep this solution it should be notified as a 
warning for the users.

Tamas

[dmorissette]

I'd like to add the functions to PHP MapScript once you've decided on the final
function names to use. Can you please summarize once you've made a final
decision... and perhaps also update the mapscript/doc/mapscript.txt?

So far what I see in shape.i is:

... the constructor:
    shapeObj(int type=MS_SHAPE_NULL, char *wkt=NULL) 

... and 1 method:
    char *toWKT()

[sgillies@…]

Steve, see http://www.swig.org/Doc1.3/Java.html#overloaded_functions about
function overloading to have multiple constructor forms for Java and C#.

[sdlime]

Thanks for the link Sean.

Steve

[sdlime]

Looks like swig 1.3 fixes things with respect to overloading. Who are the best 
candidates to test java and C# with respect to 4.8? I need to update the CVS 
machine to the newest version of SWIG as well.

Steve

[szekerest]

For C#, I have tried both of the constructors and worked without any problems. 
If you commit the proposed change i will try it.

Tamas

[sdlime]

This is the 4.8 tree now (has been for awhile).

[dmorissette]

Steve, can you please confirm that the new methods that need to be added to PHP
MapScript are:

... the constructor:
    shapeObj(int type=MS_SHAPE_NULL, char *wkt=NULL) 

... and 1 method:
    char *toWKT()

The PHP methods may have to wait until after the 4.8.0 release though.

[sdlime]

That's the ticket...

Steve

[sdlime]

I've been thinking about this more. I wonder if the overloaded constructor 
really makes sense or should we do a fromWKT... So, processing would look like:

# in perl
$shape = new $mapscript::shapeObj($mapscript::MS_SHAPE_NULL);
$shape->fromWKT($wkt_string);

It's not too late to make this change since this is a new feature. Any opinions 
or should I quit worrying about it.

Steve

[szekerest]

It would be desirable to mark this function as "static" in order to eliminate
the need of creating a new object previously.

Tamas

[sgillies@…]

I mentioned it in an email thread, but I should have commented here before. I
think a fromWKT is much better.

[sdlime]

I agree with you and Tamas, just took me awhile to catch on. I will make the 
change to shape.i this evening.

Steve

[sdlime]

Guys: Any idea how to code this? I've tried a couple of things and either:

  - it works great if there's no error, with a segfault if there is an error

or 

  - it works great with a error, but segfaults with solid input

The interface code for the last case is below. The former happens if you blow
away the old shapeObj (self) and then use self = msShapeFromWKT()...

Mapscript code looks like:

$shape = new mapscript::shapeObj() or die("Unable to create shape.");
if(($status = $shape->fromWKT('LINESTRING(0 0,1 1,1 2)')) != 0) {
  die(mapscript::msGetErrorString("\n")); 
}

Steve


    int fromWKT(char *string)
    {
        shapeObj *shape;
        int i;

        if(!string) return(MS_SUCCESS); /* ok, I guess */

        shape = msShapeFromWKT(string);
        if(!shape) return(MS_FAILURE);

        /* Allocate memory for 4 values */
        if ((shape->values = (char **)malloc(sizeof(char *)*4)) == NULL) {
            msSetError(MS_MEMERR, "Failed to allocate memory for values",
"fromWKT()");
            return(MS_FAILURE);
        } else {
            for (i=0; i<4; i++) shape->values[i] = strdup("");
        }
        shape->numvalues = 4;

        /* free anything previously allocated */
        msFreeShape(self);
        free(self);
        self = shape;

        return(MS_SUCCESS);
    }

[szekerest]

I prefer the following solution:

%newobject fromWKT;
    static shapeObj *fromWKT(char *wkt) 
    {
        shapeObj *shape;
        int i;
        
		shape = msShapeFromWKT(wkt);
		if (!shape)
            return NULL;
       
        /* Allocate memory for 4 values */
        if ((shape->values = (char **)malloc(sizeof(char *)*4)) == NULL)
        {
            msSetError(MS_MEMERR, "Failed to allocate memory for values",
                       "fromWKT()");
            return NULL;
        }
        else
        {
            for (i=0; i<4; i++) shape->values[i] = strdup("");
        }
        shape->numvalues = 4;
        
        return shape;
    }

To create the shape in c# one could use:

shapeObj myNewShape = shapeObj.fromWKT("LINESTRING(0 0,1 1,1 2)");


I suggest to keep from using keywords such as "string" as a parameter name. This
may result in uncompilable code.


Tamas

[sdlime]

Why static? I'm not familiar with that with regards to SWIG... If I get what
your suggesting then the perl code would be:

$shape = mapscript::shapeObj.fromWKT($wk); 

or something like that. Correct? Sort of an alternate constructor. Will SWIG
still  destroy it properly if the class constructor is not used?

(I agree on the string name and will change) 

Steve

[szekerest]

Steve

Static functions can be called without the need of creating an object instance.
This approach suits best for your aim mentioned previously, because you would
not like to use multiple constructors. The object construction takes place
inside the fromWKT function which behaves like a constructor, but constructs an
other object not the self.

After creating the object it behaves like any other objects, the destructor -if
any - will be called automatically.

Nevertheless I wonder if this function works correctly because of a confusion in  
msOGRShapeFromWKT in mapogr.cpp.

See
http://mapserver.gis.umn.edu/bugs/show_bug.cgi?id=1614
for more details


Tamas Szekeres

[sdlime]

I figured that's what was up. Sounds like a good solution if it works. I can't
get a perl version working. I may just be having a syntax problem and will test
more. I've only used the GEOS WKT support so no idea on the OGR problems you're
seeing.

Steve

[sdlime]

Ok, I've verified things work in Perl. You do something like:

  $shape = mapscript::shapeObj::fromWKT('LINESTRING(0 0,1 1,1 2)');

I'll commit with just a bit more feedback about other languages, especially Dan
on PHP...

Steve

[szekerest]

I have tested some wkt-s with C# applying Frank's fix to msOGRShapeFromWKT and
worked.

Tamas

[dmorissette]

In response to Steve's question from comment #25, we do not use object-oriented
constructors in PHP MapScript, we use regular functions instead (e.g.
ms_newLayerObj())... this is legacy from the PHP3 days since I think we should
be able to use "new layerobj();" in PHP4 and PHP5 (I didn't try though).

Anyway, we can't change that now, so to be consistent with what's already in
place, we could create a ms_shapeObjFromWkt() function for PHP MapScript.

[dmorissette]

I'll add the PHP methods now:

    shapeObj ms_shapeObjFromWkt(char *wkt) 
    char *shapeObj::toWKT()

[dmorissette]

I've added shapeObj::toWkt() and ms_shapeObjFromWkt() to PHP MapScript in both
the 4.8 branch and 4.9-dev. Jeff: the PHP MapScript readme has been updated with
the two new functions, you may need to update the docs on the website.

Steve: it seems that the msOGRShapeToWkt() implementation is not finished and it
always returns NULL, is this possible?

Also, can you please confirm that the return value (string) from msShapeToWkt()
is a newly allocated string that should be freed by the caller? I could not get
confirmation of that from looking at the code.

[sdlime]

I updated shape.i to use the method favored by Tamas and Sean in both 4.8 and
4.9. I've also added Frank in the CC to comment on OGR stuff. The GEOS WKT
writer does not create free'able output. It could depending on what Frank's
stuff is doing.

Steve

[dmorissette]

My current implementation calls msFree() on the string returned by toWkt(). It's
easy to change, just let me know.

[fwarmerdam]

I have completed implementation of the OGR shape2wkt function (which 
I had apparently not finished before) and I have ensured the returned string
is suitable to free with msFree(), even in odd ogr-is-in-a-dll situations.

[sdlime]

I'll go ahead then and update the geos code to produce a free'able string, and
the SWIG code to create a newobject...

Steve

[sdlime]

Made the last of the changes. This one should be good to go in 4.8 and 4.9...

Steve

[jmckenna@…]

i've updated the phpmapscript doc on the plone site.

[hobu]

Reading through, I don't see any reason why this bug can't be closed.

[sdlime]

You are correct sir...