[gdal-dev] Layer operations, a proposal

[Even Rouault]

> http://trac.osgeo.org/gdal/wiki/rfc39_ogr_layer_algebra
>

Ari,

I have reviewed your proposal and it looks interesting. Here are my observations
:

1) Argument order. A.Operation(B, C) where C is the target layer doesn't seem
very intuitive, but I'm not sure I have something fundamentally better to
propose. An alternate solution would be to make the operations static methods,
and have OGRLayer::Operation(output_layer, input_layer, method_layer) ? I'd
suggest at least to rename B as method_layer and C as result_layer to be more
explicit on the role of each argument.

2) It would perhaps be prudent to add a char** papszOptions argument as a
provision for future needs.

3) I'm wondering if it would not be more appropriate to separate the creation of
fields of the output layer in a separate method that might be called, or not,
before the real operation. For example IdentifyPrepare() for Identity(). Of
course this would complicate a bit the implementation of the operation because
it will need to look for which fields exist in the output layer and how the
input and method layer fields map to those fields. But the establishment of the
mapping and the setting of the output feature from the fields of the input and
method features could be made in common methods like the following (untested) :

  int* ComputeFieldMapping(OGRFeatureDefn* outputDefn, OGRFeatureDefn*
inputDefn)
  {
      int *panInputToOuputMap = (int*)CPLMalloc(sizeof(int) *
inputDefn->GetFieldCount());
      for( int iField = 0; iField < inputDefn->GetFieldCount(); iField++ )
      {
          panInputToOutputMap[iField] =
outputDefn->GetFieldIndex(inputDefn->GetFieldDefn(iField)->GetNameRef());
      }
      return panInputToOutputMap;
  }

  where panInputToOutput gives for each field of inputDefn the index of the
field in outputDefn that matches (or -1 if there's no match).

  And for the copying you would have :

  void SetFieldsFrom(OGRFeature* outputFeat, OGRFeature* inputFeat, int*
panInputToOutputMap)
  {
      for( int iField = 0; iField < poSrcFeature->GetFieldCount(); iField++ )
      {
        iDstField = panInputToOutputMap[iField];
        if( iDstField < 0 )
            continue;
        set_field(outputFeat, iDstField, inputFeat, iField);
      }
  }

   CopyFields() is a simplified version of the existingOGRFeature::SetFrom(
OGRFeature * poSrcFeature, int *panMap , int bForgiving ) which doesn't try to
copy the geometry, style or FID. But I see from the set_field() method that you
likely had already a look at this method ;-)

   This way the XXXXXPrepare() would not need to check that CreateField()
succeeds because XXXX() could work with an output layer with no fields at all.
Checking that there are not fields with same name would not be necessary (in
that case the value from B would override the value from A).

4) It would be nice to able to provide a progress callback to get progress
report (and also being able to interrupt) for such potentially long operations.

5) A few remarks on the implementation itself (only code review, no actual
testing). You can likely wait for comments from others before heading to fix
them...

a) CreateField() and CreateFeature() return code should be tested and acted
upon. Likely interrupt the whole process ? (not strictly necessary for
CreateField() is you adopt the idea developed in point 3))

b) I think there's something wrong with OGRGeometry *Bfilter =
B->GetSpatialFilter(). When you later do
B->SetSpatialFilter(x_geom->Intersection(Bfilter)), I believe it will destroy
the previous installed filter, and thus Bfilter will point to a garbage object.
You likely need to clone the initial spatial filter (and delete it after
restoring it at the end of the method)

c) Each time you do a new OGRFeature(), you need do delete it (after the call to
CreateFeature()).

d) GetGeometryRef() can return NULL (even for spatial layers). Currently it is
never checked, and there are a few places where it is dereferenced.

e) Lines like x_geom_diff = x_geom_diff->Difference(y_geom) (or geom =
geom->Union(y_geom)) will leak the initial x_geom_diff object. You likely need
to do :
	OGRGeometry* x_geom_diff_new = x_geom_diff->Difference(y_geom);
	delete x_geom_diff;
	x_geom_diff = x_geom_diff_new;

f) The object returned by GetNextFeature() should be deleted after use.

g) You should be careful with SetGeometryDirectly(). You cannot pass directly
the geometry belonging to another feature because this will segfault when you
free the said feature.

The following is OK :

OGRFeature *x = GetNextFeature()
OGRGeometry *geom = ...
OGRGeometry *x_geom = x->GetGeometryRef();
z->SetGeometryDirectly(x_geom->Difference(geom));
delete x;
delete geom;

But not :

OGRFeature *x = GetNextFeature()
OGRGeometry *x_geom = x->GetGeometryRef();
z->SetGeometryDirectly(x_geom); // use z->SetGeometry(x_geom) instead
delete x;


h) Nothing wrong, but calls to UnsetField() are unnecessary for a feature that
was just created and never set before (all the lines z->UnsetField(...) )

Well, enough for now ;-)

Best regards,

Even