Ari,<br><br> as a user (and teacher of users), I would urge you to keep to the model where the output is always a new object (actually the same as ArcGIS), rather than consider modifying an input for any of these spatial operations. That automatically solves the read-only input issue.<br>
<br>Best wishes,<br><br>Peter<br><br><div class="gmail_quote">On 3 May 2012 07:34, Ari Jolma <span dir="ltr"><<a href="mailto:ari.jolma@gmail.com" target="_blank">ari.jolma@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 05/02/2012 09:56 PM, Jason Roberts wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Ari,<br>
<br>
I have been following this proposal with interest. I have a couple of<br>
questions; sorry to bring them up late in the RFC process.<br>
<br>
It looks like the inspiration for this was the Overlay toolset in the ArcGIS<br>
geoprocessing toolbox. That is nice for people like me who regularly work<br>
with ArcGIS; it increases the possibility of being able to replace<br>
automation currently based on ArcGIS with something based on GDAL. Assuming<br>
you are aiming for basic parity with ArcGIS:<br>
</blockquote>
<br>
ArcGIS is an example which is assumably pretty good, and I'm not aware of any standards or better ones so that's the reason. Personally, I don't use ArcGIS, so I can't say much.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
1. Would you consider implementing a Symmetrical Difference method? It is<br>
the only one from the ArcGIS Overlay toolset that you did not implement<br>
(except Spatial Join, which does not really apply here).<br>
</blockquote>
<br>
I'd be happy to. Can you point me to a description of the method?<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
2. In ArcGIS, the Intersect and Union tools support multiple inputs. Is that<br>
something you would consider implementing? I don't have a strong opinion on<br>
this. It is occasionally necessary to use all of these tools with multiple<br>
inputs (e.g. erase several layers from a starting layer). I'm not<br>
immediately sure why ESRI elected to support multiple inputs for two of<br>
their tools but not more of them.<br>
</blockquote>
<br>
We've chosen to implement these tools as C = A.method(B), i.e., they always produce a result layer and not change A nor B (the C has to be created before the actual method). For Update, Clip and Erase it might make sense (as the schema of the result layer is only that of input layer) to have the method be an in-place method: A.method(B), i.e., change A. Then it would be trivial to write<br>
<br>
for all B in (a list of method layers)<br>
A.method(B)<br>
<br>
I would in fact support this syntax as it is more versatile and saves memory in some cases. The downside is that it does not work with read-only data sources and thus is probably not suitable for GDAL.<br>
<br>
In Intersection, Union and Identity the schema of the result may contain attributes from the method layer, thus IMO they do not fit so well into the in-place syntax.<br>
<br>
In the current syntax, to run a method on several method layers:<br>
<br>
for all B in (a list of method layers)<br>
C = new Layer like A<br>
A.method(B, C)<br>
delete A<br>
A = C<br>
<br>
It really seems to me that this would be practical only for memory layers. Maybe the support for multiple layers should be left for the command line tool or scripting language layer.<br>
<br>
For example I'll probably build a support into the Perl bindings for the in-place syntax.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
3. Will these functions be exposed from the Python bindings?<br>
</blockquote>
<br>
They are already, if you apply the patch. The test code is written in Python.<br>
<br>
Thanks for the interest.<br>
<br>
Ari<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Best regards,<br>
<br>
Jason<br>
<br>
-----Original Message-----<br>
From: <a href="mailto:gdal-dev-bounces@lists.osgeo.org" target="_blank">gdal-dev-bounces@lists.osgeo.<u></u>org</a><br>
[mailto:<a href="mailto:gdal-dev-bounces@lists.osgeo.org" target="_blank">gdal-dev-bounces@<u></u>lists.osgeo.org</a>] On Behalf Of Ari Jolma<br>
Sent: Wednesday, April 25, 2012 9:33 AM<br>
To: <a href="mailto:gdal-dev@lists.osgeo.org" target="_blank">gdal-dev@lists.osgeo.org</a><br>
Subject: [gdal-dev] RFC 39<br>
<br>
Folks,<br>
<br>
I'll start a new thread for this RFC.<br>
<br>
I've uploaded a new patch, which hopefully corrects the issues Even brought<br>
up. Below are comments.<br>
<br>
I've not added the Append or Buffer methods, which basically work on just<br>
one layer. I believe such functionality is simple enough to create using<br>
feature iteration.<br>
<br>
Ari<br>
<br>
the patch:<br>
<a href="http://trac.osgeo.org/gdal/attachment/wiki/rfc39_ogr_layer_algebra/rfc39.pat" target="_blank">http://trac.osgeo.org/gdal/<u></u>attachment/wiki/rfc39_ogr_<u></u>layer_algebra/rfc39.pat</a><br>
ch<br>
<br>
<br>
On 04/19/2012 02:04 PM, Even Rouault wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<a href="http://trac.osgeo.org/gdal/wiki/rfc39_ogr_layer_algebra" target="_blank">http://trac.osgeo.org/gdal/<u></u>wiki/rfc39_ogr_layer_algebra</a><br>
<br>
</blockquote>
Ari,<br>
<br>
I have reviewed your proposal and it looks interesting. Here are my<br>
observations<br>
:<br>
<br>
1) Argument order. A.Operation(B, C) where C is the target layer<br>
doesn't seem very intuitive, but I'm not sure I have something<br>
fundamentally better to propose. An alternate solution would be to<br>
make the operations static methods, and have<br>
OGRLayer::Operation(output_<u></u>layer, input_layer, method_layer) ? I'd<br>
suggest at least to rename B as method_layer and C as result_layer to be<br>
</blockquote>
more explicit on the role of each argument.<br>
<br>
<br>
I've changed the names to Input, Method and Output. For example the layer<br>
object pointers are pLayerInput etc.<br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2) It would perhaps be prudent to add a char** papszOptions argument as a<br>
provision for future needs.<br>
</blockquote>
I've added it.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
3) I'm wondering if it would not be more appropriate to separate the<br>
</blockquote>
creation of<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
fields of the output layer in a separate method that might be called, or<br>
</blockquote>
not,<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
before the real operation. For example IdentifyPrepare() for Identity().<br>
</blockquote>
Of<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
course this would complicate a bit the implementation of the operation<br>
</blockquote>
because<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
it will need to look for which fields exist in the output layer and how<br>
</blockquote>
the<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
input and method layer fields map to those fields. But the establishment<br>
</blockquote>
of the<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
mapping and the setting of the output feature from the fields of the input<br>
</blockquote>
and<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
method features could be made in common methods<br>
<br>
This way the XXXXXPrepare() would not need to check that CreateField()<br>
succeeds because XXXX() could work with an output layer with no fields at<br>
</blockquote>
all.<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Checking that there are not fields with same name would not be necessary<br>
</blockquote>
(in<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
that case the value from B would override the value from A).<br>
</blockquote>
<br>
I've created a new method into Feature class: SetFieldsFrom, which uses<br>
a map. It is similar to SetFrom, which uses a map. (thus the field copy<br>
could be optimized into one function)<br>
<br>
The layer methods now first check the existence of fields in the result<br>
layer. If there are fields, it is assumed that that is what the user<br>
wants. If there are no fields, a complete set of fields are created for<br>
fields in input and method layers (in some cases only input layer). This<br>
behavior could later be overridden with options.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
4) It would be nice to able to provide a progress callback to get progress<br>
report (and also being able to interrupt) for such potentially long<br>
</blockquote>
operations.<br>
<br>
<br>
I agree. I've added these to the API, but I've not yet added the calls.<br>
This requires for example the inclusion of gdal.h into ogr_api.h.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
5) A few remarks on the implementation itself (only code review, no actual<br>
testing). You can likely wait for comments from others before heading to<br>
</blockquote>
fix<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
them...<br>
<br>
a) CreateField() and CreateFeature() return code should be tested and<br>
</blockquote>
acted<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
upon. Likely interrupt the whole process ? (not strictly necessary for<br>
CreateField() is you adopt the idea developed in point 3))<br>
</blockquote>
<br>
I've added tests. I've used the "goto done;" style, where done is a<br>
label just before the release of used resources and exit from the<br>
method. I've tried to be careful to leave no memory leaks.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
b) I think there's something wrong with OGRGeometry *Bfilter =<br>
B->GetSpatialFilter(). When you later do<br>
B->SetSpatialFilter(x_geom-><u></u>Intersection(Bfilter)), I believe it will<br>
</blockquote>
destroy<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
the previous installed filter, and thus Bfilter will point to a garbage<br>
</blockquote>
object.<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
You likely need to clone the initial spatial filter (and delete it after<br>
restoring it at the end of the method)<br>
</blockquote>
<br>
Yes. I checked all calls to GetSpatialFilter and now clone the initial<br>
one and destroy it at the end.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
c) Each time you do a new OGRFeature(), you need do delete it (after the<br>
</blockquote>
call to<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
CreateFeature()).<br>
</blockquote>
They are now deleted.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
d) GetGeometryRef() can return NULL (even for spatial layers). Currently<br>
</blockquote>
it is<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
never checked, and there are a few places where it is dereferenced.<br>
</blockquote>
The validity of the geometry is checked. If there is no geometry, the<br>
next feature is fetched.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
e) Lines like x_geom_diff = x_geom_diff->Difference(y_<u></u>geom) (or geom =<br>
geom->Union(y_geom)) will leak the initial x_geom_diff object. You likely<br>
</blockquote>
need<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
to do :<br>
OGRGeometry* x_geom_diff_new = x_geom_diff->Difference(y_<u></u>geom);<br>
delete x_geom_diff;<br>
x_geom_diff = x_geom_diff_new;<br>
</blockquote>
Fixed.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
f) The object returned by GetNextFeature() should be deleted after use.<br>
</blockquote>
It is deleted, even if the iteration is continued.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
g) You should be careful with SetGeometryDirectly(). You cannot pass<br>
</blockquote>
directly<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
the geometry belonging to another feature because this will segfault when<br>
</blockquote>
you<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
free the said feature.<br>
</blockquote>
Fixed<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
h) Nothing wrong, but calls to UnsetField() are unnecessary for a feature<br>
</blockquote>
that<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
was just created and never set before (all the lines z->UnsetField(...) )<br>
</blockquote>
The unnecessary code is removed.<br>
<br>
______________________________<u></u>_________________<br>
gdal-dev mailing list<br>
<a href="mailto:gdal-dev@lists.osgeo.org" target="_blank">gdal-dev@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/gdal-dev" target="_blank">http://lists.osgeo.org/<u></u>mailman/listinfo/gdal-dev</a><br>
<br>
______________________________<u></u>_________________<br>
gdal-dev mailing list<br>
<a href="mailto:gdal-dev@lists.osgeo.org" target="_blank">gdal-dev@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/gdal-dev" target="_blank">http://lists.osgeo.org/<u></u>mailman/listinfo/gdal-dev</a><br>
</blockquote>
<br>
______________________________<u></u>_________________<br>
gdal-dev mailing list<br>
<a href="mailto:gdal-dev@lists.osgeo.org" target="_blank">gdal-dev@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/gdal-dev" target="_blank">http://lists.osgeo.org/<u></u>mailman/listinfo/gdal-dev</a><br>
</blockquote></div><br><br clear="all"><br>-- <br>----------------------------------------------------------------------------------------------------------------<br>Peter J Halls, GIS Advisor & Team Leader Applications Support and Training, <br>
Information Directorate, University of York<br>Telephone: 01904 323806 Fax: 01904 323740<br>Snail mail: Harry Fairhurst Building, University of York, <br> Heslington, York YO10 5DD<br>This message has the status of a private and personal communication<br>
----------------------------------------------------------------------------------------------------------------<br><br>