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

Fix issue caused by new drive PR #896 #134

Merged
merged 3 commits into from
Nov 24, 2024
Merged

Fix issue caused by new drive PR #896 #134

merged 3 commits into from
Nov 24, 2024

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Nov 20, 2024

Resource now save in format x/y/z which breaks ship_info

Thank you for submitting a pull request and becoming a contributor to Vega Strike: Upon the Coldest Sea.

Code Changes:

Resource now save in format x/y/z which breaks ship_info
@royfalk royfalk self-assigned this Nov 20, 2024
@evertvorster
Copy link
Contributor

Testing against current master of the Engine. (If this is the wrong branch, some indication as to which branch of the engine code to test against would be appreciated)
When starting a new campaign, in ship info shield info indicates that there is no shield installed.
In the upgrade bay, I can see that there is, in fact, a shield installed.
When purchasing refuel and repair, I get a message that the ship has no damage, and that there would be no charge. Normally there is some work done, and the ship maximum speed changes from 120 to 125.
When launching this ship, there is no shielding on the ship.

@royfalk
Copy link
Contributor Author

royfalk commented Nov 20, 2024

Testing against current master of the Engine. (If this is the wrong branch, some indication as to which branch of the engine code to test against would be appreciated)

No need to test more than ship view.
Not a bad idea to test against master.
But really, you should test against PR #896, which is task_real_drive_comp.

@evertvorster
Copy link
Contributor

Hi there!
I just tested this branch against the task_real_drive_comp branch of the engine.
Unfortunately this same issue remains.
When starting a new campaign, the shields of the Llama.begin does not seem to be interpreted correctly.
It shows no shielding information in ships info, even though a shield is present in the ship upgrade window. Also, when launching, there is no shield on the ship.
For some reason Oswald's ship also seems damaged, not quite sure if this is related.

@royfalk
Copy link
Contributor Author

royfalk commented Nov 20, 2024

It is. Are you by any chance using the new units.json from #133 ?
That would be consistent with it.

@evertvorster
Copy link
Contributor

Hi there!
Is there any way I can check? As far as I know, the Arch build system totally removes all the files from the previous install before installing the current branch.
However, I will test again with this branch.

@evertvorster
Copy link
Contributor

Hi there!
I think I found the issue, and it was on my side. I was under the impression that telling git to checkout a certain branch would show only the files from that branch.
Anyways, I totally removed the source tree, and checked out this branch.
Now that issue with the shields is gone. I'll give it some more testing and see if I can find any show stoppers.
Thanks for your patience.

@royfalk
Copy link
Contributor Author

royfalk commented Nov 22, 2024

Merged changes from #129 .

@royfalk
Copy link
Contributor Author

royfalk commented Nov 24, 2024

@evertvorster is this OK? I already merged vegastrike/Vega-Strike-Engine-Source#896 and then discovered the matching Assets is still not approved. You can test this branch against engine/master.

Copy link
Contributor

@evertvorster evertvorster left a comment

Choose a reason for hiding this comment

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

Yes, this is the branch I tested against, and should be merged.

@royfalk royfalk merged commit 451881a into master Nov 24, 2024
10 of 11 checks passed
@royfalk royfalk deleted the task_fix_ship_view branch November 24, 2024 15:49
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.

2 participants