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

A python script to generate ship view #130

Merged
merged 5 commits into from
Nov 1, 2024
Merged

A python script to generate ship view #130

merged 5 commits into from
Nov 1, 2024

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Oct 27, 2024

This PR starts moving a lot of the ship view and later similar code from base_computer.cpp to python. The script can be run independently and pull data from units.json/llama.begin.

This works with vegastrike/Vega-Strike-Engine-Source#899.

Please answer the following:

Code Changes:

Issues:

  • This will break when we merge a PR that saves a resource in the form x/y/z. We'll need to modify this further to support.

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

Hi there!

Before building this, I see that you have to merge in the change where the config.json is installed into this branch, and so resolve that conflict.
Also, there is another new file that also is not installed through the CMakeLists.txt.

It is a bit tedious, but makes things easier down the line.

I don't see how the game engine is taking advantage of this new python script. It that to be implemented at a later stage when this is in master?

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.

Approving to get it in; but I'd like to see the File open and json.load move to the C++/engine side so we can present a more consistent interface to the game; unless of course we're expecting that config.json is a pure Game Asset - in which case we should go with option a in the comment and expose a good method for knowing that from the VS side - but all that can be evolution of this PR.

with open('config.json', 'r') as file:
data = json.load(file)
non_combat_speed_multiplier = data['components']['drive']['non_combat_mode_multiplier']
megajoules_multiplier = data['constants']['megajoules_multiplier']
Copy link
Member

Choose a reason for hiding this comment

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

since config.json would have to be in a specific place we need to either:
(a) define a working directory for the active "game"
(b) expose these as Python Objects instead of making each script load it if it wants access. This would eliminate the open and json.load steps; and we'd have to define the interface - either as a pure Python Dictionary/Object or as something else. A first step might be just having a load method that would load the contents and return the data object like you have above.

I'd rather b between the two.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking more about this...option a might actually be a better solution for game asset data. But let's evolve the conversation on this beyond just this PR.

non_combat_speed_multiplier = data['components']['drive']['non_combat_mode_multiplier']
megajoules_multiplier = data['constants']['megajoules_multiplier']

with open('units/units.json', 'r') as file:
Copy link
Member

Choose a reason for hiding this comment

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

same here


t = get_ship_description(llama_begin)
t = t.replace('#n#','\n')
print(t)
Copy link
Member

Choose a reason for hiding this comment

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

if this is for testing, then it probably should be wrapped. Usually if its a test to be called on the commandline (e.g just see the script output) then using:

if __name__ == "__main__":
   # python block

would be appropriate. If we can control the name and calling it in a test, then may be we call it __vegastrike_test_main__ instead...but I doubt that'd we'd have that control.

as per comments
@royfalk royfalk merged commit a1f47db into master Nov 1, 2024
9 of 10 checks passed
@royfalk royfalk deleted the task_ship_view branch November 1, 2024 09:23
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