Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#3255 closed enhancement (fixed)

patch to add projection file support to lcpdataset.cpp.

Reported by: Kyle Shannon Owned by: Kyle Shannon
Priority: normal Milestone: 1.10.1
Component: GDAL_Raster Version: svn-trunk
Severity: normal Keywords: lcpdataset
Cc: Even Rouault

Description

Add the functionality for a lcpdataset to look for and load a spatial reference to a farsite landscape file. This was implemented the same way as it is in the AAIGrid driver.

Attachments (3)

lcpdataset.patch (5.6 KB) - added by Kyle Shannon 7 years ago.
better diff file
test_lcp.zip (16.6 KB) - added by Kyle Shannon 7 years ago.
lcpdataset_v2.patch (4.9 KB) - added by Even Rouault 7 years ago.
Revised version by Even Rouault

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by Even Rouault

Kyle,

would you mind providing a link to a downloadable dataset that uses a .prj so that we can test your change ?

Also it would be far easier to review if your patch only contained the semantic changes, not all those whitespaces changes...

Changed 7 years ago by Kyle Shannon

Attachment: lcpdataset.patch added

better diff file

comment:2 Changed 7 years ago by Kyle Shannon

Sorry about the diff file, it's my first patch. I think I got a better one up there. Also you can download this small lcp file with a prj file in a zip archive here: http://www.smoke-fire.us/trac/kyle/export/75/lcp_driver/test_lcp.zip

Let me know if I can do anything else. Thanks.

comment:3 Changed 7 years ago by Even Rouault

The patch looks much better. Thanks. However I can't access your link. It asks me a login / password. I tried anonymous but it didn't work. If it's small enough, you could try attaching it to this ticket. Otherwise you can try to my email : even dot rouault at mines dash paris dot org.

Changed 7 years ago by Kyle Shannon

Attachment: test_lcp.zip added

comment:4 in reply to:  3 Changed 7 years ago by Kyle Shannon

Replying to rouault:

The patch looks much better. Thanks. However I can't access your link. It asks me a login / password. I tried anonymous but it didn't work. If it's small enough, you could try attaching it to this ticket. Otherwise you can try to my email : even dot rouault at mines dash paris dot org.

Just attached the zip archive.

comment:5 Changed 7 years ago by Even Rouault

I've attached a revised version of the patch because I noticed you've copied&pasted stuff that isn't really used. The poDS->adfGeoTransform array which existed before your wasn't used as the GetGeoTransform?() method doesn't use it. So I've removed all its use as well as the OSR_GDS() function. It was probably something specific to the AAIGrid driver that doesn't make sense for LCP ? I've also fixed the missing initialization of pszProjection and it's deallocation.

Could you confirm that this version is OK ? (it gives identical results as your version on your sample)

Changed 7 years ago by Even Rouault

Attachment: lcpdataset_v2.patch added

Revised version by Even Rouault

comment:6 Changed 7 years ago by Kyle Shannon

I patched my source and everything worked well over here. Thanks for the help.

comment:7 Changed 7 years ago by Even Rouault

Milestone: 1.6.41.7.0
Resolution: fixed
Status: newclosed

Patch commited in trunk in r18123

comment:8 Changed 7 years ago by Kyle Shannon

Even, should we add a prj to the autotest data directory at some point for testing this patch?

comment:9 Changed 7 years ago by warmerdam

Cc: Even Rouault added

Even, see previous comment.

comment:10 Changed 7 years ago by Even Rouault

Test updated in r18206

comment:11 in reply to:  10 Changed 7 years ago by Kyle Shannon

Replying to rouault:

Test updated in r18206

Thanks Even

comment:12 Changed 4 years ago by Kyle Shannon

Milestone: 1.7.01.10.1
Resolution: fixed
Status: closedreopened

Re-opening as I found a couple issues with the feature. 1) pszProjection was initialized to NULL, so GetProjectionRef?() returned NULL if no prj was found. 2) Initializing pszProjection to non-NULL ("") caused the prj file to be added to the sibling list even if it didn't exist.

I have a fix, just going to do a bit of testing before I submit.

comment:13 Changed 4 years ago by Kyle Shannon

Owner: changed from warmerdam to Kyle Shannon
Status: reopenednew

Taking ownership.

comment:14 Changed 4 years ago by Kyle Shannon

Resolution: fixed
Status: newclosed

Fixed in trunk r26206 and branches/1.10 r26207.

comment:15 Changed 4 years ago by Even Rouault

Resolution: fixed
Status: closedreopened

Kyle,

I believe this will leak memory. You should likely CPLFree(poDS->pszProjection); just before oSRS.exportToWkt( &(poDS->pszProjection) ); Other solution you keep the null initialization of pszProjection, and in GetProjectionRef?() you return pszProjection ? pszProjection : ""

comment:16 Changed 4 years ago by Even Rouault

Resolution: fixed
Status: reopenedclosed

Nevermind. I missed the commits r26209 and r26210

comment:17 Changed 4 years ago by Kyle Shannon

Summary: patch to add projection file support to lcpdataset.cpppatch to add projection file support to lcpdataset.cpp.

I eventually remembered, :). I knew I forgot something.

Note: See TracTickets for help on using tickets.