lassefolkersen / highfrontier

The goal of High Frontier is a civilization-like game, which uses more realistic modelling of economy, climate, research and migration. Uniquely this game starts today - the initiation date of a game is todays date and the existing world is the world as we know it today, complete with cities and countries and people. The aim from there - that is whatever the player thinks the aim is for the world. Developing of the underdeveloping world, action against climate changes, expansion into the rest of the solar system, or just racking in a lot of money.
13 stars 3 forks source link

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

Closed JonathanLochridge closed 1 month ago

JonathanLochridge commented 2 months ago

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 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 commented 2 months ago

@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 ? https://github.com/lassefolkersen/highfrontier/blob/c345d6f6f5a607d71f0268cf39e574ccadd523fe/planet.py#L683

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

lionel42 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

$ 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 commented 2 months ago

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)

JonathanLochridge commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

lassefolkersen commented 1 month ago

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 commented 1 month ago

I am working on it on branch pyproj

JonathanLochridge commented 1 month ago

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 commented 1 month ago

This issue was fixed and implemented in #26