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

Move code to python - starting with configuration #892

Merged
merged 15 commits into from
Oct 16, 2024
Merged

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Oct 10, 2024

Use boost python to get a C++ struct/class from python.
Implement Graphics2Config (temp name) - has resolution and screen number. Populate struct from config.json via C++.
Modify game to take resolution and screen values from config.yaml. Deprecates settings app.
Implement unit tests in configuration.

Code Changes:

Issues:

  • Python code should be placed somewhere logical. My vote is /python.
  • Lower resolution causes game to go out of bounds. Already noted by Evert.
  • Need to complement this with a PR to assets.
  • Need to complement this with a PR to assets for WC.

@royfalk royfalk self-assigned this Oct 10, 2024
@evertvorster
Copy link
Contributor

evertvorster commented Oct 10, 2024

While I agree that the configuration part of Vega Strike needs work, I have a concern about including a Python package as part of the game.

While it is easier for software developers to just add Python stuff everywhere, it is a nightmare for packagers in distrobutions to resolve conflicts.

In my opinion it would be better to have pyyaml as a dependency of Vega Strike, and let the system provide the package.

On my system, pyyaml is already installed, and required by the following packages:

Required By     : beets  bottles  breezy  conan  picard  python-confuse  python-pytorch-opt-cuda
Optional For    : extra-cmake-modules  libinput  python-markdown  python-numba

If it becomes part of Vega Strike, I would have to strip it out as part of the install process to enable Vega Strike to co-exist with the above mentioned packages.

Arch specifically does not allow pip to run as pacman manages the Python packages installed in the system.

@royfalk
Copy link
Contributor Author

royfalk commented Oct 10, 2024

Arch specifically does not allow pip to run as pacman manages the Python packages installed in the system.

So, I'm not real good with the whole dev-ops side of things. You raise some excellent points and I think that if we do support auto-install of python packages, it'll have to be as part of the install script. @BenjamenMeyer worked on these and I saw some apt-get directives there. I'll edit the above to say consider instead and let's see what Benjamen says.

Add prints to identify crashed test
@BenjamenMeyer
Copy link
Member

@evertvorster would us trying to figure out how to use a Python Virtual Environment help?

Generally, we don't have much access to the system Python installation or packages. It would be nice to be able to use some other packages too. A Python Virtual Env might be a good way to go - let's us more specifically control the environment and reduces the potential for incompatibility with the system. Not sure how easy it is to do with Boost::Python though.

Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about reading the config in Python... it'd probably be better to read it in C++ land and expose some methods to update and save it to the Python world.

If we wanted to keep the YAML format (more below) then this would also remove the Python dependency; we'd just have to figure out how to do a C++ version (preferably easily integrated with Boost).

That said, we're already using JSON. We should probably be consistent with the file formats we're using and not use different ones all over. So it's probably wise to keep it to JSON.

All said - I do like the transition. Though to deprecate vssettings we would have to make sure all those settings get migrated over to the GUI.

@@ -0,0 +1,24 @@
import yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while YAML is a little easier to read; it's not built-in to Python so requires an additional package.
We're already using JSON elsewhere and JSON is built-in to Python. It might be wiser to be consistent with ourselves and just use what's built-in. The code below barely changes; the file format does a bit.

YAML is a little nicer looking; but for the moment ...not sure if it's the right solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/jbeder/yaml-cpp looks promising...but they removed their Boost dependency.
Boost currently lists YAML as a future development.

Move config loading back to cpp.
Move graphics_config from python to configuration folder and out of python library.
Remove empty files without code.
@royfalk
Copy link
Contributor Author

royfalk commented Oct 15, 2024

I'm at a loss here. I have no idea why it's failing, except that it's all basically the same version.
Ubuntu Focal is based on Debian Bullseye/sid and Linux mint 20 is based on Focal.

@royfalk
Copy link
Contributor Author

royfalk commented Oct 15, 2024

OK. Found it. It seems newer systems can do this on their own or something.

/usr/bin/ld: CMakeFiles/vegastrike_python.dir/src/python/config/python_utils.cpp.o: relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I'll try and figure this out on my own tomorrow.

@royfalk
Copy link
Contributor Author

royfalk commented Oct 16, 2024

Yesss!!! Finally.
@BenjamenMeyer please review.

@royfalk royfalk merged commit 77296c1 into master Oct 16, 2024
43 checks passed
@royfalk royfalk deleted the task_more_python branch October 16, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants