GDALSuggestedWarpOutput2 does more work than necessary

[gaige]

The edge sample transformation test fails 100% of the time due to multiple errors. Fortunately, this has little effect on the user perception, since the grid test works just fine afterwards, resulting in a slightly longer process to check the bounds, but not creating a critical problem.

Among the problems were:

• dfRatio wasn't recalculated, so all points were tested against points outside of the box
• the test of the reverted points actually tested the original projected points instead
• the test of the reverted points only tested the X coordinate (twice) instead of both the X and Y coordinates

==== //gdaltransformer.cpp#13 - gdaltransformer.cpp ====
***************
*** 428,433 ****
--- 428,437 ----
                  if( !abSuccess[i] )
                      nFailedCount++;
                  
+ 				dfRatio = 0.0 + floor(i/4)*(1.0/N_STEPS);
+ 				if (dfRatio>0.99)
+ 					dfRatio = 1.0;
+ 					
                  double dfExpectedX, dfExpectedY;
                  if ((i % 4) == 0)
                  {
***************
*** 450,457 ****
                      dfExpectedY   = dfRatio * nInYSize;
                  }
                  
!                 if (fabs(adfX[i] - dfExpectedX) > nInXSize / N_STEPS ||
!                     fabs(adfX[i] - dfExpectedX) > nInYSize / N_STEPS)
                      nFailedCount ++;
              }
          }
--- 454,461 ----
                      dfExpectedY   = dfRatio * nInYSize;
                  }
                  
!                 if (fabs(adfXRevert[i] - dfExpectedX) > nInXSize / N_STEPS ||
!                     fabs(adfYRevert[i] - dfExpectedY) > nInYSize / N_STEPS)
                      nFailedCount ++;
              }
          }

[warmerdam]

Gaige,

I red-facedly concur with your analysis. Please go ahead and apply these changes in trunk and 1.7 branch.

[Even Rouault]

Ahem, *I* ref-facedly concur. This is code I modified substantially for ticket #2305 (http://trac.osgeo.org/gdal/changeset/18035) during 1.7 developement. I remember it was quite tricky to come with, and as Gaige found, the code just after in the algorithm fallbacks with computations on the full grid, which hides the fact that this code just before was buggy. Your changes look right Gaige.

[gaige]

Committed to Trunk (18995) and 1.7 (18996)

[Even Rouault]

Thanks !

Minor notes : in your commit messages, in addition to the ticket number, it would be good to add a short description of what the fix is. This makes the life of people writing the ChangeLog easier ;-)

Ah, and a Trac tip, when you indicate a revision number, indicate the r before the number, such as r18995 so Trac automagically makes a hyperlink to the changeset.

[gaige]

Replying to rouault:

Thanks !

Minor notes : in your commit messages, in addition to the ticket number, it would be good to add a short description of what the fix is. This makes the life of people writing the ChangeLog easier ;-)

Ah, and a Trac tip, when you indicate a revision number, indicate the r before the number, such as r18995 so Trac automagically makes a hyperlink to the changeset.

Thanks. I looked for some examples and some specifics in the RFCs and didn't see them (maybe I looked in the wrong place), but I didn't find anything. Thanks and I'll make sure to do that in the future.

[Even Rouault]

Yes, everything is not written. I've updated RFC3 to reflect my above advice. I think "Use meaningful descriptions for SVN commit log entries." in SVN Commit Practices paragraph matches what I meant with "add a short description of what the fix is"

[Even Rouault]

Win32 doesn't like floor()...

gdaltransformer.cpp(431) : error C2668: 'floor' : ambiguous call to overloaded function
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(559): could be 'long double floor(long double)'
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(511): or 'float floor(float)'
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(137): or 'double floor(double)'
        while trying to match the argument list '(int)'

I take care of fixing this

[Even Rouault]

r19003 /trunk/gdal/alg/gdaltransformer.cpp: Compilation fix for r18995 on Windows platform (#3447)

r19004 /branches/1.7/gdal/alg/gdaltransformer.cpp: Compilation fix for r18996 on Windows platform (#3447)

[gaige]

Replying to rouault:

Win32 doesn't like floor()...

gdaltransformer.cpp(431) : error C2668: 'floor' : ambiguous call to overloaded function
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(559): could be 'long double floor(long double)'
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(511): or 'float floor(float)'
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(137): or 'double floor(double)'
        while trying to match the argument list '(int)'

I take care of fixing this

Thanks for that. I need to figure out how the buildfarm works as I don't have a win32 development system to test against.

Next time, it'll be better... -Gaige