[Geoserver-devel] GEOS-1330, or ReprojectFeatueResults wrapping a ReprojectFeatureResults

Farber_Saul_EEA · October 10, 2007, 10:57pm

Hey all,

I chatted with Justin a bit about this earlier, but I think I've resolved
GEOS-1330. I discovered two things doing this, which I wanted to run by the
gs folks (particularly Andrea) and then I'll send the relevant parts to the
geotools lists to make sure I'm on the right track.

1) I think the fundamental problem was that line 328 of KMLTranslator.java
was just plain doing the wrong thing. Query.setCoordinateSystem()
(according to the javadocs) is supposed to be a declarative 'this is the
coordinate system of this featurecollection, and I will query it using this
coord sys'. At the minimum, it acts that way (see how
AbstractFeatureSource.getFeatures() treats it).

So when we called q.setCoordinateSystem(mapContext.getCoordSys()) this
simply told the featureSource that we were asserting that data inside that
featuresource was in that coordsys. So the returned FeatureResults from
that query wound up being asserted to be in mapContext.getCoordsys() (4326,
generally) and the ReprojectFeatureResults wound up wrapping a
DefaultFeatureResults that declared that it was returning features in 4326
-> so there was no reprojection.

I'll double-check with the gt crowd about how this is *supposed* to work,
but as I see it right now, if you know your featureSource has a legit crs
connected to it, then you shouldn't set q.setCoordinateSystem...you should
set q.setCoordinateSystemReproject() (if you want reprojection) or you
should set nothing.

2) Separately, I think I found a tricky bug in ReprojectFeatureResults
constructor. The bug is outlined here, by showing the current logic of the
constructor:

ReprojectFeatureResults(FeatureResults originalResults, CRS targetCoordSys)
{

a. Build a featuretype which is exactly like the ft of originalResults,
only with the coordsys co-erced to targetCoordsys (for those who will ask us
about our featureType)
b. Figure out if originalResults is ALSO a
ReprojectFeatureResults/ForceFeatureResults...cause if it is let's skip that
(now defunct) reprojection, and simply reproject the underlying results.
c. Get the coordsys of the originalResults, and if it's different than the
targetCoordsys, then find the right transform and remember it.

}

So the bug is that step b looks at the UNDERLYING results (which are
probably in a different coordsys) and step c looks at the PASSED-IN results.
This is almost always going to be a bug, and probably means that non-trivial
attepmts to ReprojectFeatureResults() a ReprojectFeatureResults probably
hasn't worked for a while.

Anyway, I fixed both 1 and 2 by:
* changing 1 to q.setCoordSysReproject() and
* changing line 98 of ReprojectFeatureResults.java to
CoordinateReferenceSystem originalCs =
this.results.getSchema().getDefaultGeometry().getCoordinateSystem();

Mostly I'm looking for some confirmation that this makes sense (all tests
pass in geotools and geoserver) and that I should go pester the GT list
about confirming that I'm right about #1 and that #2 really is a bug.

--saul
--
View this message in context: http://www.nabble.com/GEOS-1330%2C-or-ReprojectFeatueResults-wrapping-a-ReprojectFeatureResults-tf4604159.html#a13146465
Sent from the GeoServer - Dev mailing list archive at Nabble.com.

jive · October 10, 2007, 11:54pm

sfarber wrote:

Hey all,

I chatted with Justin a bit about this earlier, but I think I've resolved
GEOS-1330. I discovered two things doing this, which I wanted to run by the
gs folks (particularly Andrea) and then I'll send the relevant parts to the
geotools lists to make sure I'm on the right track.

1) I think the fundamental problem was that line 328 of KMLTranslator.java
was just plain doing the wrong thing. Query.setCoordinateSystem()
(according to the javadocs) is supposed to be a declarative 'this is the
coordinate system of this featurecollection, and I will query it using this
coord sys'. At the minimum, it acts that way (see how
AbstractFeatureSource.getFeatures() treats it).

You got it - here is a user guide page with the example (http://docs.codehaus.org/display/GEOTDOC/Welcome+to+Eclipse+Developers). The example is available on 2.4.x demo/example.

So when we called q.setCoordinateSystem(mapContext.getCoordSys()) this
simply told the featureSource that we were asserting that data inside that
featuresource was in that coordsys. So the returned FeatureResults from
that query wound up being asserted to be in mapContext.getCoordsys() (4326,
generally) and the ReprojectFeatureResults wound up wrapping a
DefaultFeatureResults that declared that it was returning features in 4326
-> so there was no reprojection.

I'll double-check with the gt crowd about how this is *supposed* to work,
but as I see it right now, if you know your featureSource has a legit crs
connected to it, then you shouldn't set q.setCoordinateSystem...you should
set q.setCoordinateSystemReproject() (if you want reprojection) or you
should set nothing.

Your understanding is correct:
- setCoordinateSystem() is used to "force" the projection (ie just change the FeatureType)
- setCoordinateSystemReproject() is used to set up a MathTransform (ie change the FeatureType and the data)

You may find that the two feature collection implementations (as are used by the renderer) are better tested.

Jody

2) Separately, I think I found a tricky bug in ReprojectFeatureResults
constructor. The bug is outlined here, by showing the current logic of the
constructor:

ReprojectFeatureResults(FeatureResults originalResults, CRS targetCoordSys)
{

a. Build a featuretype which is exactly like the ft of originalResults,
only with the coordsys co-erced to targetCoordsys (for those who will ask us
about our featureType)
b. Figure out if originalResults is ALSO a
ReprojectFeatureResults/ForceFeatureResults...cause if it is let's skip that
(now defunct) reprojection, and simply reproject the underlying results.
c. Get the coordsys of the originalResults, and if it's different than the
targetCoordsys, then find the right transform and remember it.

}

So the bug is that step b looks at the UNDERLYING results (which are
probably in a different coordsys) and step c looks at the PASSED-IN results.

Doh. Okay that makes sense.

This is almost always going to be a bug, and probably means that non-trivial
attepmts to ReprojectFeatureResults() a ReprojectFeatureResults probably
hasn't worked for a while.

You are correct; I don't think that what you describe is a senerio that occurs as part of featureSource.getFeatures( Query ).

Anyway, I fixed both 1 and 2 by:
* changing 1 to q.setCoordSysReproject() and * changing line 98 of ReprojectFeatureResults.java to
CoordinateReferenceSystem originalCs =
this.results.getSchema().getDefaultGeometry().getCoordinateSystem();

Mostly I'm looking for some confirmation that this makes sense (all tests
pass in geotools and geoserver) and that I should go pester the GT list
about confirming that I'm right about #1 and that #2 really is a bug.

You are correct; nice detective work Saul.
Jody

Farber_Saul_EEA · October 11, 2007, 2:14pm

Excellent. Glad that I wasn't out on a limb.

I'll commit the fix both to gs and gt. Thanks jody.

--saul

Jody Garnett wrote:

sfarber wrote:

Hey all,

I chatted with Justin a bit about this earlier, but I think I've resolved
GEOS-1330. I discovered two things doing this, which I wanted to run by
the
gs folks (particularly Andrea) and then I'll send the relevant parts to
the
geotools lists to make sure I'm on the right track.

1) I think the fundamental problem was that line 328 of
KMLTranslator.java
was just plain doing the wrong thing. Query.setCoordinateSystem()
(according to the javadocs) is supposed to be a declarative 'this is the
coordinate system of this featurecollection, and I will query it using
this
coord sys'. At the minimum, it acts that way (see how
AbstractFeatureSource.getFeatures() treats it).


You got it - here is a user guide page with the example
(http://docs.codehaus.org/display/GEOTDOC/Welcome+to+Eclipse+Developers).
The example is available on 2.4.x demo/example.

So when we called q.setCoordinateSystem(mapContext.getCoordSys()) this
simply told the featureSource that we were asserting that data inside
that
featuresource was in that coordsys. So the returned FeatureResults from
that query wound up being asserted to be in mapContext.getCoordsys()
(4326,
generally) and the ReprojectFeatureResults wound up wrapping a
DefaultFeatureResults that declared that it was returning features in
4326
-> so there was no reprojection.

I'll double-check with the gt crowd about how this is *supposed* to work,
but as I see it right now, if you know your featureSource has a legit crs
connected to it, then you shouldn't set q.setCoordinateSystem...you
should
set q.setCoordinateSystemReproject() (if you want reprojection) or you
should set nothing.


Your understanding is correct:
- setCoordinateSystem() is used to "force" the projection (ie just
change the FeatureType)
- setCoordinateSystemReproject() is used to set up a MathTransform (ie
change the FeatureType and the data)

You may find that the two feature collection implementations (as are
used by the renderer) are better tested.

Jody

2) Separately, I think I found a tricky bug in ReprojectFeatureResults
constructor. The bug is outlined here, by showing the current logic of
the
constructor:

ReprojectFeatureResults(FeatureResults originalResults, CRS
targetCoordSys)
{

a. Build a featuretype which is exactly like the ft of originalResults,
only with the coordsys co-erced to targetCoordsys (for those who will ask
us
about our featureType)
b. Figure out if originalResults is ALSO a
ReprojectFeatureResults/ForceFeatureResults...cause if it is let's skip
that
(now defunct) reprojection, and simply reproject the underlying results.
c. Get the coordsys of the originalResults, and if it's different than
the
targetCoordsys, then find the right transform and remember it.

}

So the bug is that step b looks at the UNDERLYING results (which are
probably in a different coordsys) and step c looks at the PASSED-IN
results.


Doh. Okay that makes sense.

This is almost always going to be a bug, and probably means that
non-trivial
attepmts to ReprojectFeatureResults() a ReprojectFeatureResults probably
hasn't worked for a while.


You are correct; I don't think that what you describe is a senerio that
occurs as part of featureSource.getFeatures( Query ).

Anyway, I fixed both 1 and 2 by:
  * changing 1 to q.setCoordSysReproject() and
  * changing line 98 of ReprojectFeatureResults.java to
CoordinateReferenceSystem originalCs =
this.results.getSchema().getDefaultGeometry().getCoordinateSystem();

Mostly I'm looking for some confirmation that this makes sense (all tests
pass in geotools and geoserver) and that I should go pester the GT list
about confirming that I'm right about #1 and that #2 really is a bug.


You are correct; nice detective work Saul.
Jody

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

--
View this message in context: http://www.nabble.com/GEOS-1330%2C-or-ReprojectFeatueResults-wrapping-a-ReprojectFeatureResults-tf4604159.html#a13156705
Sent from the GeoServer - Dev mailing list archive at Nabble.com.