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 #997 #998

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

stephengtuggy
Copy link
Contributor

Code Changes:

Issues:

Purpose:

  • What is this pull request trying to do? Fix a bug that affects saving and loading unit files on Windows
  • What release is this for? 0.10.x
  • Is there a project or milestone we should apply this to? 0.10.x

@stephengtuggy stephengtuggy added this to the 0.10.x milestone Jan 17, 2025
@stephengtuggy stephengtuggy self-assigned this Jan 17, 2025
@stephengtuggy stephengtuggy marked this pull request as ready for review January 17, 2025 03:34
@evertvorster
Copy link
Contributor

evertvorster commented Jan 17, 2025

You know what would be great? If the PR's or engine have some sort of description of which branch of Assets they need to be tested against.
Unfortunately I don't have a Windows environment, so this is about the only positive contribution I can make at this point.

@kheckwrecker
Copy link

Having a problem with game crashing after launching new campaign. Here's the debugger output:
Exception thrown at 0x00007FFDEABEB699 in vegastrike-engine.exe: Microsoft C++ exception: boost::wrapexceptboost::system::system_error at memory location 0x00000006C056C2E0.
Error parsing JSON in UnitJSONFactory::ParseJSON(): syntax error [boost.json:1 at D:\VS_Master_Test\Vega-Strike-Engine-Source\build\VS2022Win64-pie-disabled-release\vcpkg_installed\vcpkg\blds\boost-json\src\ost-1.84.0-6591c5c930.clean\include\boost/json/basic_parser_impl.hpp:1270:19 in function 'parse_escaped']
The thread 10244 has exited with code 0 (0x0).
The thread 'alsoft-mixer' (9648) has exited with code 0 (0x0).
The thread 11380 has exited with code 0 (0x0).
The thread 11572 has exited with code 42 (0x2a).
The thread 9248 has exited with code 42 (0x2a).
The thread 11648 has exited with code 42 (0x2a).
The thread 12080 has exited with code 42 (0x2a).
The thread 2316 has exited with code 42 (0x2a).
The thread 12104 has exited with code 42 (0x2a).
The thread 9356 has exited with code 42 (0x2a).
The thread 11808 has exited with code 42 (0x2a).
The thread 12272 has exited with code 42 (0x2a).
The thread 9444 has exited with code 42 (0x2a).
The thread 11564 has exited with code 42 (0x2a).
The thread 10836 has exited with code 42 (0x2a).
The program '[12228] vegastrike-engine.exe' has exited with code 42 (0x2a).

Is this the same problem?

@stephengtuggy
Copy link
Contributor Author

stephengtuggy commented Jan 17, 2025

@kheckwrecker yeah, I'm pretty sure that's the same problem that this PR addresses.

Or one of them, anyway.

@kheckwrecker
Copy link

Ok. I'll try pulling this patch and see if it works.

@kheckwrecker
Copy link

GitHub is having trouble with the pull request. Something about not find your work? Anyway, I'll just wait until it's approved and merged then try again.

@kheckwrecker
Copy link

I pulled this patch and re-compiled, but it's still crashing with the same error. It's entirely possible that I'm screwing up the pull and merge, so I'll wait and try again after this patch is officially merged with master. If the problem still occurs after that, I'll create a new issue report.

Copy link
Contributor

@royfalk royfalk left a comment

Choose a reason for hiding this comment

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

Where is the fix for root? I assumed the offending line would be
unit_attributes["root"] = file.GetRoot();
but you didn't actually change it.

engine/src/cmd/csv.cpp Outdated Show resolved Hide resolved
engine/src/cmd/unit_csv.cpp Outdated Show resolved Hide resolved
engine/src/cmd/unit_csv.cpp Outdated Show resolved Hide resolved
engine/src/cmd/unit_json_factory.cpp Outdated Show resolved Hide resolved
#include <vector>
#include <string>
#include <map>
#include <boost/json.hpp>

#include "resource/json_utils.h"


void UnitJSONFactory::ParseJSON(VSFileSystem::VSFile &file, bool player_ship) {
Copy link
Contributor

Choose a reason for hiding this comment

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

My code (deficiencies and all) assumed the following:

[
   {"a": "1", "b": 2},
   {"a": "1", "b": 2},
   {"a": "1", "b": 2},
   ...
]

However, this looks like it supports other file formats such as {"a": "1", "b": 2} without an array. Why would we do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that's how the Boost JSON serialization function constructs it. So when we go to load the file, we need to support either format -- with or without an array at the top level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify further: Roy, your code saved an array with a single object element inside of it:

[
    {
        ...
    }
]

My code simply removes the root array:

{
    ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, we actually want the array. Right now the game saves each ship in its own file. There's no reason to do this. We just need to figure the whole player_ship issue. Either, replace it with player_ship_1...2...3 or player_ship_<random 6 alphanumeric> or something like that and check for prefix.

This would also solve the issue of not supporting a fleet containing two ships of the same type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see.

@stephengtuggy
Copy link
Contributor Author

Where is the fix for root? I assumed the offending line would be unit_attributes["root"] = file.GetRoot(); but you didn't actually change it.

I thought it would be too, but as it turned out, that was not the problem.

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