Opened 10 years ago

Closed 10 years ago

#842 closed defect (fixed)

wx location wizard: spincontrol busted for UTM zone

Reported by: hamish Owned by: grass-dev@…
Priority: critical Milestone: 6.4.0
Component: wxGUI Version: svn-develbranch6
Keywords: location wizard, utm Cc:
CPU: x86-64 Platform: Linux

Description

Hi,

the spin control for selecting the UTM zone in the new wx Location creation wizard is busted on multiple Debian/Lennies? for me but works on Michael's Mac.

For me it locks on zone "1" as soon as you try and change it. The only way we've found to unstick it is to comment out the first place the default value of 30 is set on line 710 ( self.pval[num] = '30' ), but then I get errors in the terminal because it is uninitialized and if I continue past the resulting PROJ_INFO +lon_0 comes in at -183 degrees regardless of zone.

we tried changing the following with no luck:

- self.pval[num] = '30'
+ self.pval[num] = 30
  • remove value=
  • remove initial=
  • remove style=

spin controls work ok for me in module dialogs, such as r.composite.

any ideas? anyone else see this?


also the wizard does not create UTM zones as GRASS's type PROJECTION_UTM (1) but rather as expanded +proj4 terms as PROJECTION_OTHER (99). Lat/lon correctly sets itself as PROJECTION_LL (3).

in the g.proj C code I have followed it as far as lib/proj/ convert.c's GPJ_osr_to_grass(), which has some code to specifically find the UTM zone from the +proj string, but then it trails off a bit.

Here is where it decides to use GRASS's lat/lon mode instead of go on using an "other" custom +proj string:

if (G_strcasecmp(pszToken, "proj") == 0) {
     /* The ll projection is known as longlat in PROJ.4 */
     if (G_strcasecmp(pszValue, "longlat") == 0)
         pszValue = "ll";

     pszProj = pszValue;
     continue;
}

but there is no similar short-circuit for UTM zones or state plane. see also the GPJ_osr_to_grass() function header comments about return value.

State Plane (PROJECTION_SP: 2) is less of a priority for now as the wx wizard end of that hasn't been written yet, but some basic placeholder code could go in.

Hamish

Attachments (3)

location_wizard.patch (4.9 KB) - added by cmbarton 10 years ago.
Fix for UTM zone spin control and adds error trapping to extent setting page
location_wizard.patch2 (6.4 KB) - added by cmbarton 10 years ago.
location_wizard.patch3 (2.9 KB) - added by cmbarton 10 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 10 years ago by hamish

ah, I've just spotted this "hack around some weirdness", which may be a major clue..

# deal with weird spin control text event behavior, sending dup. event with negative number       
   if event.GetId() >= 2000:
      num = event.GetId() - 2000

source:grass/branches/develbranch_6/gui/wxpython/gui_modules/location_wizard.py@40163#L720

Hamish

comment:2 Changed 10 years ago by hamish

... commenting that out has no effect

comment:3 Changed 10 years ago by cmbarton

MIght not have any effect because it is broken on your box. We can work on this some more this week.

Michael

Changed 10 years ago by cmbarton

Attachment: location_wizard.patch added

Fix for UTM zone spin control and adds error trapping to extent setting page

comment:4 Changed 10 years ago by cmbarton

I think I've fixed this. Please try the patch I just attached here. It is a diff against the svn (releasebranch_6_4) from your source tree root. The patch also adds error trapping for the default extents setting page.

Michael

comment:5 Changed 10 years ago by hamish

no luck for me using the latest patch with 6.5svn. As soon as I click up or down it gets stuck at "1". If I leave it at 30 it successfully makes a zone 30 location, if it gets stuck on 1 it successfully makes a zone 1 location.

Hamish

comment:6 Changed 10 years ago by cmbarton

This patch is for 6.4, not 6.5

Michael

comment:7 in reply to:  6 Changed 10 years ago by hamish

Replying to cmbarton:

This patch is for 6.4, not 6.5

oh ok, it applied to 6.5 without errors just an offset of a few lines. When applied to 6.4 I get the same results. Stuck at "1".

is all the region try: rows vs. cols stuff in the patch somehow related or is that just along for the ride?

Hamish

comment:8 Changed 10 years ago by cmbarton

I'll try one more thing. I'm pretty sure it's the binding and event handler. It has code that tries to keep the value between 1 and 60.

The other stuff is a bug fix I noticed when I was testing this. I was oversure of fixing it I guess or I wouldn't have added this fix to the patch. Have to live with it now.

Michael

comment:9 Changed 10 years ago by cmbarton

Please try the following. Comment out lines 731-742 in the OnParamEntry? method

#        if self.ptype[num] == 'zone':
#            try:
#                valint = int(val)
#            except:
#                valint = 0
                
#            if valint < 1: 
#                self.pentry[num].SetValue(1)
#                val = '1'
#            if valint > 60: 
#                self.pentry[num].SetValue(60)
#                val = '60'

This disables the validation check for the text box of the spin control. If this works, maybe I can validate this another way.

Michael

comment:10 in reply to:  9 ; Changed 10 years ago by hamish

Replying to cmbarton:

Please try the following. Comment out lines 731-742 in the OnParamEntry? method

...

This disables the validation check for the text box of the spin control. If this works, maybe I can validate this another way.

Yes- that lets me spin. If I manually type in 67 or -1 for the zone I get a pop up and it corrects me to the nearest valid value.

but if I let it spin full-scale a few times in one direction when I press continue it complains about an invalid zone even though the number I see on the screen is in-range.

Replying to cmbarton:

The other stuff is a bug fix I noticed when I was testing this. I was oversure of fixing it I guess or I wouldn't have added this fix to the patch. Have to live with it now.

probably just worry about this for the development branches for now & when you get a chance, but re. bounds settings a couple of wishes:

  • popup error if s>n, or w>e (except LL), or abs(ns)>90 (LL), right now you can click the button but nothing happens if its invalid. feedback as to what the error is would be good.
  • after setting the bounds the new region settings are copied to the terminal. left over debug code?
  • LL bounds should accept [NnSsEeWw:] as well as [+-.0-9] chars, and res should accept ':'; currently it just accepts floats. maybe quietly accept 123d45'43.21"S format as well if we are feeling generous?
  • it would be really really gee wiz cool if for LL a red rectangle would draw covering the region you've selected on the center map. even cooler but not that useful in practice would to be able to drag a box on it to get the bounds numbers close, rounded to the current resolution.

thanks, Hamish

comment:11 in reply to:  10 Changed 10 years ago by cmbarton

Replying to hamish:

Replying to cmbarton:

Please try the following. Comment out lines 731-742 in the OnParamEntry? method

...

This disables the validation check for the text box of the spin control. If this works, maybe I can validate this another way.

Yes- that lets me spin. If I manually type in 67 or -1 for the zone I get a pop up and it corrects me to the nearest valid value.

but if I let it spin full-scale a few times in one direction when I press continue it complains about an invalid zone even though the number I see on the screen is in-range.

That is REALLY weird! The SpinCtrl? is limited in the code but the accompanying text box is not. My system behaves as described in the wxPython docs. I can spin all I want, but it only goes from 1->60->1->... No way to spin it above 60 or below 1. On the other hand, I can type whatever I want. If type 80, for example, the zone simply doesn't get put in. I will add a validator to the page changing handler, where it should not interfere with the spinning. But the behavior on your system is odd. Seems like a bug in the GTK version.

The error trapping I did for bounds is minimal, but it prevents the wizard from throwing an error to terminal if the rows or columns are 0. Your other suggestions are good. But like you said, should go into 6.5+.

Michael

Replying to cmbarton:

The other stuff is a bug fix I noticed when I was testing this. I was oversure of fixing it I guess or I wouldn't have added this fix to the patch. Have to live with it now.

probably just worry about this for the development branches for now & when you get a chance, but re. bounds settings a couple of wishes:

  • popup error if s>n, or w>e (except LL), or abs(ns)>90 (LL), right now you can click the button but nothing happens if its invalid. feedback as to what the error is would be good.
  • after setting the bounds the new region settings are copied to the terminal. left over debug code?
  • LL bounds should accept [NnSsEeWw:] as well as [+-.0-9] chars, and res should accept ':'; currently it just accepts floats. maybe quietly accept 123d45'43.21"S format as well if we are feeling generous?
  • it would be really really gee wiz cool if for LL a red rectangle would draw covering the region you've selected on the center map. even cooler but not that useful in practice would to be able to drag a box on it to get the bounds numbers close, rounded to the current resolution.

thanks, Hamish

Changed 10 years ago by cmbarton

Attachment: location_wizard.patch2 added

comment:12 Changed 10 years ago by cmbarton

OK. I just attached a new patch for GRASS 6.4: location_wizard.patch2. It's like the first one, patched against the svn from the root of your source tree. Hopefully, this will do the trick.

Michael

comment:13 in reply to:  12 ; Changed 10 years ago by hamish

Replying to cmbarton:

OK. I just attached a new patch for GRASS 6.4: location_wizard.patch2. It's like the first one, patched against the svn from the root of your source tree. Hopefully, this will do the trick.

no dice.

I can spin it, it loops past 60 and back to 1, but if I touch the up/down controls at all or type in a number I get a pop up window that says "Something is missing! You must enter a value for Projection Zone (1-60)". If I don't touch it at all I can go past and create a location for UTM zone 30.

after I press ok on the error popup, this appears in the terminal:

Traceback (most recent call last):
  File "/usr/src/grass/svn/releasebranch_6_4/dist.x86_64-unknown-linux-gnu/etc/wxpython/gui_modules/location_wizard.py", line 769, in OnPageChange
    if int(self.pval[num]) > 60 or int(self.pval[num]) < 1:
ValueError: invalid literal for int() with base 10: ''

If I put some debug messages directly before L769,

        num = self.utmzoneNum
        print "num=",num
	print "self.utmzoneNum=",self.utmzoneNum
	print "self.pval[num]=",self.pval[num]

I get this on the terminal after pressing ok to the error popup message:

num= 0
self.utmzoneNum= 0
self.pval[num]= 

Hamish

comment:14 Changed 10 years ago by hamish

Priority: majorcritical

comment:15 in reply to:  13 Changed 10 years ago by cmbarton

Replying to hamish:

Replying to cmbarton:

OK. I just attached a new patch for GRASS 6.4: location_wizard.patch2. It's like the first one, patched against the svn from the root of your source tree. Hopefully, this will do the trick.

no dice.

I can spin it, it loops past 60 and back to 1, but if I touch the up/down controls at all or type in a number I get a pop up window that says "Something is missing! You must enter a value for Projection Zone (1-60)". If I don't touch it at all I can go past and create a location for UTM zone 30.

after I press ok on the error popup, this appears in the terminal:

Traceback (most recent call last):
  File "/usr/src/grass/svn/releasebranch_6_4/dist.x86_64-unknown-linux-gnu/etc/wxpython/gui_modules/location_wizard.py", line 769, in OnPageChange
    if int(self.pval[num]) > 60 or int(self.pval[num]) < 1:
ValueError: invalid literal for int() with base 10: ''

If I put some debug messages directly before L769,

        num = self.utmzoneNum
        print "num=",num
	print "self.utmzoneNum=",self.utmzoneNum
	print "self.pval[num]=",self.pval[num]

I get this on the terminal after pressing ok to the error popup message:

num= 0
self.utmzoneNum= 0
self.pval[num]= 

Hamish

Drat!

This is a page changing validator that someone else added since last I worked on the location wizard. Good idea, but it's wrong. I noticed but it hadn't been causing errors so I left it alone, wanting to mess with this as little as possible. I guess I have to fix it too.

Michael

comment:16 Changed 10 years ago by hamish

for 6.4.0 a regular text entry box should be fine; we can keep working on the spincontrol in the dev branches and have it ready for 6.4.1.

Hamish

Changed 10 years ago by cmbarton

Attachment: location_wizard.patch3 added

comment:17 Changed 10 years ago by cmbarton

I've changed the UTM Zone entry widget to a simple text widget with validation to keep it within the 1-60 range. I've added a new patch here: location_wizard.patch3. Please test.

Michael

comment:18 in reply to:  17 ; Changed 10 years ago by hamish

Replying to cmbarton:

I've changed the UTM Zone entry widget to a simple text widget with validation to keep it within the 1-60 range. I've added a new patch here: location_wizard.patch3. Please test.

Yes it works if you type in zone=1-60.

I'm seeing that +lon_0=-183 again though and I figured out how to reproduce it. If you enter "0" or "99" for the zone it automatically resets to zone 1 or 60, respectively. Apparently this is only visually. If you proceed to the summary page in the location wizard instead of +zone=23 you will see +proj=tmerc +lon_0=411 etc. ...

-PROJ_INFO-------------------------------------------------
name       : Transverse Mercator
proj       : tmerc
datum      : wgs84
ellps      : wgs84
lat_0      : 0
lon_0      : -183
k          : 0.9996
x_0        : 500000
y_0        : 0
no_defs    : defined
-PROJ_UNITS------------------------------------------------
unit       : Meter
units      : Meters
meters     : 1

fwiw1: I notice that PROJ_UNITS is uppercased, which seems different to how I always seem to have remembered it there.

fwiw2: I notice this patch no longer has the try: row/column stuff. (different things should go into 2 different commits I guess)

Hamish

comment:19 in reply to:  18 Changed 10 years ago by hamish

Replying to hamish:

fwiw1: I notice that PROJ_UNITS is uppercased, which seems different to how I always seem to have remembered it there.

this may be problematic, I see that lib/gis/proj2.c and proj3.c do work pretty much raw from the PROJ_UNITS file. there is a lower() function in there, but I am not confident that it covers all bases. Probably want a double fix at both ends if possible.

Hamish

comment:20 in reply to:  18 Changed 10 years ago by cmbarton

Replying to hamish:

Replying to cmbarton:

I've changed the UTM Zone entry widget to a simple text widget with validation to keep it within the 1-60 range. I've added a new patch here: location_wizard.patch3. Please test.

Yes it works if you type in zone=1-60.

I'm seeing that +lon_0=-183 again though and I figured out how to reproduce it. If you enter "0" or "99" for the zone it automatically resets to zone 1 or 60, respectively. Apparently this is only visually. If you proceed to the summary page in the location wizard instead of +zone=23 you will see +proj=tmerc +lon_0=411 etc. ...

-PROJ_INFO-------------------------------------------------
name       : Transverse Mercator
proj       : tmerc
datum      : wgs84
ellps      : wgs84
lat_0      : 0
lon_0      : -183
k          : 0.9996
x_0        : 500000
y_0        : 0
no_defs    : defined
-PROJ_UNITS------------------------------------------------
unit       : Meter
units      : Meters
meters     : 1

This is all fixed and now committed. I'll try to port this to to 6.5 and 7 after it's had a few more tests--and will then mark this resolved. Thanks for all your help in getting this worked out. The bindings and events generated by the spinctrl were byzantine. This works much better.

fwiw1: I notice that PROJ_UNITS is uppercased, which seems different to how I always seem to have remembered it there.

I don't understand what you're referring to. There is no upper() function in the module and nothing named proj_units that I could find. The location wizard does not set the proj units. This is done by g.proj when the proj4 parameters are sent to it. I use the lower() function to make searching more rewarding regardless of case.

fwiw2: I notice this patch no longer has the try: row/column stuff. (different things should go into 2 different commits I guess)

I had just archived this aside until I got the spin box fixed. It is in the bugfix commit.

Michael

Hamish

comment:21 Changed 10 years ago by hamish

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.