Opened 13 years ago

Closed 10 years ago

Last modified 10 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 13 years ago.
better diff file
test_lcp.zip​ (16.6 KB ) - added by Kyle Shannon 13 years ago.
lcpdataset_v2.patch​ (4.9 KB ) - added by Even Rouault 13 years ago.
Revised version by Even Rouault

Download all attachments as: .zip

Change History (20)

comment:1 by Even Rouault, 13 years ago

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...

by Kyle Shannon, 13 years ago

Attachment: lcpdataset.patch​ added

better diff file

comment:2 by Kyle Shannon, 13 years ago

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 by Even Rouault, 13 years ago

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.

by Kyle Shannon, 13 years ago

Attachment: test_lcp.zip​ added

in reply to:  3 comment:4 by Kyle Shannon, 13 years ago

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 by Even Rouault, 13 years ago

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)

by Even Rouault, 13 years ago

Attachment: lcpdataset_v2.patch​ added

Revised version by Even Rouault

comment:6 by Kyle Shannon, 13 years ago

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

comment:7 by Even Rouault, 13 years ago

Milestone: 1.6.4 → 1.7.0
Resolution: → fixed
Status: new → closed

Patch commited in trunk in r18123

comment:8 by Kyle Shannon, 13 years ago

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

comment:9 by warmerdam, 13 years ago

Cc: Even Rouault added

Even, see previous comment.

comment:10 by Even Rouault, 13 years ago

Test updated in r18206

in reply to:  10 comment:11 by Kyle Shannon, 13 years ago

Replying to rouault:

Test updated in r18206

Thanks Even

comment:12 by Kyle Shannon, 10 years ago

Milestone: 1.7.0 → 1.10.1
Resolution: fixed
Status: closed → reopened

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 by Kyle Shannon, 10 years ago

Owner: changed from warmerdam to Kyle Shannon
Status: reopened → new

Taking ownership.

comment:14 by Kyle Shannon, 10 years ago

Resolution: → fixed
Status: new → closed

Fixed in trunk r26206 and branches/1.10 r26207.

comment:15 by Even Rouault, 10 years ago

Resolution: fixed
Status: closed → reopened

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 by Even Rouault, 10 years ago

Resolution: → fixed
Status: reopened → closed

Nevermind. I missed the commits r26209 and r26210

comment:17 by Kyle Shannon, 10 years ago

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

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

Note: See TracTickets for help on using tickets.