Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from command-line proj to pyproj(Low priority) #9

Closed
JonathanLochridge opened this issue Apr 29, 2024 · 20 comments
Closed

Migrate from command-line proj to pyproj(Low priority) #9

JonathanLochridge opened this issue Apr 29, 2024 · 20 comments

Comments

@JonathanLochridge
Copy link
Collaborator

JonathanLochridge commented Apr 29, 2024

Issue orginally found from a crash error. But now crashing doesn't generally occur, However an opportunity to improve potential user troubleshooting, and make packaging up the program easier in the future it migrating from pyproj to proj. The main benefits will come for linux, but it may make packaging the windows version easier in the future.

Old first post preserved for posterity below:

Crashes on creating a new game, after inputting all the options for a company.

File "/Documents/GitHub/highfrontier/intro.py", line 396, in <module> introGui = IntroGui() ^^^^^^^^^^ File "/Documents/GitHub/highfrontier/intro.py", line 70, in __init__ self.receive_click(event) File "/Documents/GitHub/highfrontier/intro.py", line 86, in receive_click button.activate(event) File "/Documents/GitHub/highfrontier/gui_components.py", line 497, in activate return_value = self.function(self.label, self.function_parameter) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Documents/GitHub/highfrontier/intro.py", line 30, in start_new_game self.main.start_loop(companyName = cn, File "/Documents/GitHub/highfrontier/main.py", line 92, in start_loop surface = sol.current_planet.draw_entire_planet(sol.current_planet.eastern_inclination,sol.current_planet.northern_inclination,sol.current_planet.projection_scaling) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Documents/GitHub/highfrontier/planet.py", line 1148, in draw_entire_planet surface = self.draw_bases(surface,eastern_inclination,northern_inclination,projection_scaling) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Documents/GitHub/highfrontier/planet.py", line 1400, in draw_bases base_positions = self.calculate_base_positions(eastern_inclination, northern_inclination, projection_scaling) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Documents/GitHub/highfrontier/planet.py", line 1214, in calculate_base_positions if plane_coordinates[i] != "Not seen": ~~~~~~~~~~~~~~~~~^^^ IndexError: list index out of range

From some tracing this might be due to an error or bad arguments passed to a function:
sphere_to_plane_total

I tried to make sense of it, The comment seems to suggest it is supposed be passed a list? But, It doesn't look like it is being passed one to me? So either I am misreading things, or like maybe it is supposed to but it's not in reality?

But somehow there is a mismatch between the size of "base names" and the list of coordinates being fed.
In particular "base names is larger"
I saw a place where it was defined as empty. And a couple things that do some stuff, But I haven't figured out how it works well enough to try a thorough tracing it yet.

A smaller trace has actually confirmed that somehow:

The array is actually empty that it is supposed to be iterating on, Considering the broader context, I suspect that maybe a deleted file/method/piece is causing this? Or some other change either in how python works, or as a side effect of the 3.10 changes.

@lionel42
Copy link
Collaborator

@JonathanLochridge

Maybe the two list are not equals because something happens during the call of sphere_to_plane_total .
The function sphere_to_plane_total calls another program at the computer level proj that does the calculation of the coordinates of the bases.

Maybe you have a problem with that program.

Could you add a print/debug statement there ?

stdout_text = stdout_text[0].split("\n")

If we can know what this variable is, maybe we could determine if this causes the problem.

It should be a long list of coordinates. For me it looks like this: ['4091819.52\t3307954.27\r', '3047537.87\t5204745.45\r', '499866.67\t940593.57\r', '5829359.93\t2526145.42\r', '-6172050.22\t1550992.04\r', '*\t*\r', '*\t*\r', '3197277.92\t5228364.14\r', '*\t*\r', '3695184.50\t2639985.58\r', '303820.76\t4939276.69\r', '5091371.97\t3570908.54\r', '2422560.81\t4767908.84\r', '2763541.60\t4677611.64\r', '6026945.25\t1940171.39\r', '5767070.73\t2506736.29\r', '-414215.91\t5110103.11\r', '*\t*\r', '1419580.00\t1308285.49\r', ... ]

@JonathanLochridge
Copy link
Collaborator Author

Looks like it is probably empty or: [' ']
The other place had a completely empty parenthesis: []

I also checked stdout_tuple which was: (b'', None)

None of that seems normal.

@lionel42
Copy link
Collaborator

It seems you have a problem with this proj package.

Probably it is not install on your computer.

You can try by running proj in a terminal.

To install, you can follow these instructions for linux: https://github.com/OSGeo/PROJ/blob/master/docs/source/install.rst#debian

@JonathanLochridge
Copy link
Collaborator Author

thanks! I installed that, but the basic error still occurs. Maybe proj wasn't installing correctly?
I used the command on the link for fedora. And the terminal said it was installed, but I am still seeing the error,

I did some double checking and finally ran it on windows and as you mentioned this bug doesn't occur there.

It looks like some sort of save error is still there, Although the line which it is exiting at is different than the one it did last time. Which might be promising, or mean things are worse and we have new bugs on top of the old there..
I will put more details in the appropriate issue soon.

@JonathanLochridge JonathanLochridge changed the title Crash on New Game (Issue in planet.py) Crash on New Game (Issue in planet.py, linux only) Apr 30, 2024
@lionel42
Copy link
Collaborator

lionel42 commented May 1, 2024

@JonathanLochridge I modified a bit that part of the code to better handle the proj issue and also fixed one or two other bugs. See #11 Maybe that helps

@JonathanLochridge
Copy link
Collaborator Author

So, I looked at the code in the PR and tested it, But the proj error didn't seem to actually trigger?

On a second check, it seems like there is some kind of issue with how my local github instance is syncing? I specifically said to fetch, and github desktop claims it is up to date, and I checked on the site too.

But that PR wasn't there.

Second glance is that seems to be because the code wasn't merged. I feel a little silly there.

After actually double checking that I confirmed that the error was thrown. Not sure why it isn't registering it as being installed? Maybe since I was running in VSCode?

It actually can manage dependancies though.

Well, I can try running it in a bare terminal. since I know for sure I installed proj there.

@JonathanLochridge
Copy link
Collaborator Author

Exception: Calling the subprocess proj did not work.
On windows machines the proj.exe is found in the main highfrontier directory.
Make sure you run the program from there.
On unix-based machines it probably means that you don't have the proj command installed.
Please check and install proj if missing.
$ sudo dnf install proj
Last metadata expiration check: 1:47:28 ago on Wed 01 May 2024 09:58:24 PM EDT.
Package proj-9.2.1-2.fc39.x86_64 is already installed.
Dependencies resolved.
Nothing to do.
Complete!

(This is my terminal log, it shows proj as being installed but it still crashes and triggers the error.)

@lionel42
Copy link
Collaborator

lionel42 commented May 2, 2024

No worries for the github thing ;) I just merged now the pull request.

Okay so now at least we know the problem is with proj.

What happens if you run proj in a terminal ? Is it on you PATH ?

@JonathanLochridge
Copy link
Collaborator Author

$ proj
Rel. 9.2.1, June 1st, 2023
usage: proj [-bdeEfiIlmorsStTvVwW [args]] [+opt[=arg] ...] [file ...]

I checked and i found a proj file in usr/bin which I am pretty sure is part of the core PATH. So, in theory I should have access to it everywhere.

But running the game from any terminal still has the bug on game start. (Or in VSCode, or pycharm.)
I will try checking to see if I can access proj inside of vscode though.

So, I tried checking and VScode can't run it. Perhaps including it in the requirements as well could help for linux installs that use something like code, pycharm, etc? Not sure?

But the bug still occurs even when run outside of it. Which makes me think that maybe proj is only part of the issue. Or it is something else related to proj rather than it directly not installing.

@lassefolkersen
Copy link
Owner

Hey, I looked into this a bit. I think the big/good solution would be to just migrate away from using various command-line versions of proj, and then instead use pyproj. Shouldn't be too hard to change honestly, it's really just a matter of finding the sphere_to_plane_total and the re-writing the first 20 lines to use pyproj, and put pyproj in requirements.txt. I just haven't had the time. I'm going to rename the issue to that, because really I think that would be the most beautiful fix.

What I did do was to insert ubuntu- and mac-specific installations of proj in the CI/CD tests now, here, hopefully that can be illustrative. And it also ensure that there are tests for proj actually working. Can I have a code-review please?

(note of interest: a first version, 15 years ago or so, actually just used trig-functions, sin, cos, tan to do the same, in python - but proj turned out to be sooo much faster, so I ditched that)

@lassefolkersen lassefolkersen changed the title Crash on New Game (Issue in planet.py, linux only) Migrate from command-line proj to pyproj May 7, 2024
@JonathanLochridge
Copy link
Collaborator Author

Oh, cool! 20 lines isn't terrible, are you sure it isn't used anywhere else?

If so, I can start on trying to change that today maybe? Probably won't finish today, but I might. I want to read a bit of the pyproj documentation and such too.

I definately can put it in the requirements.txt file at least.

@lassefolkersen
Copy link
Owner

Pretty sure -- that function is called a lot of other times though, but it's only there it calls the executable proj
(grep -n -p 'proj +proj' *). Give it a go!

And give me a ✅ here please

@JonathanLochridge
Copy link
Collaborator Author

Ok, things ended up not going as I planned today. So I didn't get a chance to really work on things, due to a sudden change of plans.

I don't have anything planned tommorow, so I am going to try changing just that bit, And then after testing that I can check the issue you linked to see if that resolves the saving issue for me?

If I read it correctly it is supposed to do that?

@JonathanLochridge
Copy link
Collaborator Author

So, with the new changes this seems to be working for me at least. (The root issue.)

It doesn't seem like we changed to pyproj, but maybe I missed that?
We could still do that?

I could work on that today, but I am unsure if it is necessary or not?

@lassefolkersen
Copy link
Owner

Hmmm... maybe I lost the thread on errors, because in these last 4 PRs several were fixed. Could be the reason this cleared up for you,. That proj -> pyproj is more like an ease-of-install-thing, so that users are not asked to install separate programs outside of requirements.txt(which could be easily packed). You judge if you'd call that necessary or not 😄

@JonathanLochridge
Copy link
Collaborator Author

That does seem pretty important, but lower priority now.
I still plan to work on it then. Maybe not today. (Or maybe I will if there isn't anything better to work on.)

@JonathanLochridge JonathanLochridge changed the title Migrate from command-line proj to pyproj Migrate from command-line proj to pyproj(Low priority) May 8, 2024
@lassefolkersen
Copy link
Owner

lassefolkersen commented May 8, 2024

Hey, wait - I had one thing you could do to test 'necessariness' @JonathanLochridge --- go to the moon (in the game) or ... anywhere but Mars and Earth really, and zoom in and spin around. Because they are not cached, so they'd activate proj. If it doesn't break, then it's low priority

@lionel42 lionel42 self-assigned this May 8, 2024
@lionel42
Copy link
Collaborator

lionel42 commented May 8, 2024

I am working on it on branch pyproj

lionel42 added a commit that referenced this issue May 8, 2024
@JonathanLochridge
Copy link
Collaborator Author

Hey, wait - I had one thing you could do to test 'necessariness' @JonathanLochridge --- go to the moon (in the game) or ... anywhere but Mars and Earth really, and zoom in and spin around. Because they are not cached, so they'd activate proj. If it doesn't break, then it's low priority

That is a great idea!

@lionel42
Copy link
Collaborator

This issue was fixed and implemented in #26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants