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

Use boost for JSON parsing #923

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

Use boost for JSON parsing #923

wants to merge 38 commits into from

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Dec 2, 2024

This PR replaces singleson (json.*) with the standard boost JSON facilities.
Note: this still parses a text only JSON. We should move away to a proper JSON with types.

Please answer the following:

Code Changes:

Issues:

  • Ship view appears broken. The entire area needs to be refactored, as there is duplicate code.

Fix #688

@royfalk royfalk self-assigned this Dec 2, 2024
@evertvorster
Copy link
Contributor

Ship view does not appear THAT broken to me.
In ship info, turning response and forward acceleration both show 0 for my one saved game with a Goddard. When starting a new campaign, only turning response show 0.

This does not seem to have any influence on actually flying around.

I think we should fix any obvious problems with the game before merging, though. It is the best time, after all.

@royfalk
Copy link
Contributor Author

royfalk commented Dec 3, 2024

Ship view does not appear THAT broken to me.

Take a look at the console. Because the various stats are saved as x/y/z, the python script is unhappy.

I think we should fix any obvious problems with the game before merging, though. It is the best time, after all.

I agree. However, this is not the right repo. You can test everything else

@evertvorster
Copy link
Contributor

Merged in that fix for the turning response, and now of course it is fixed. I had a root around the ship view, and it appears OK. Also flew around a bit. Will do a little more testing, but so far this one looks good to go.

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.

Ship view looks normal to me.
I had a few battles, and the ships there also looked pretty normal.
New Campaign had no new issues.
Able to trade goods, and it all looks normal.

@evertvorster
Copy link
Contributor

I do see some syntax warnings in the console, but it does not seem to affect the game.

@royfalk
Copy link
Contributor Author

royfalk commented Dec 5, 2024

I do see some syntax warnings in the console, but it does not seem to affect the game.

Please post them here and I'll check if they're relevant. It looks like there's more work to support Windows and CodeQL has some issues, so there will be at least one more commit.

@evertvorster
Copy link
Contributor

These are the errors that I am seeing in the console:

Asset API Version: 3
/usr/share/vegastrike/modules/quests/quest_tutorial.py:840: SyntaxWarning: invalid escape sequence '\T'
  text = "CEPHID SECURITY INITIATIVE GIVES TRAINING FOR FLIGHT SAFETY\\\The Cephid Security Initiative (CSI) is offering training for pilots with the purpose of enhancing flight safety in and out of the system. "
/usr/share/vegastrike/modules/news.py:80: SyntaxWarning: invalid escape sequence '\T'
  newsfooter = "\\\\\This story was first broadcast on: "
/usr/share/vegastrike/modules/news.py:125: SyntaxWarning: invalid escape sequence '\T'
  STARDATE_TEXT = "\\\\\This story was first broadcast on: "
/usr/share/vegastrike/modules/dynamic_news.py:68: SyntaxWarning: invalid escape sequence '\T'
  STARDATE_TEXT = "\\\\\\\This story was first broadcast on: "
/usr/share/vegastrike/modules/quests/quest_dispute.py:156: SyntaxWarning: invalid escape sequence '\T'
  text = "PRIVATEER SAVES SHIPLOAD OF WOUNDED\\\Today, an unprecedented dispute about landing priorities took place close to a station in the Regallis system of Sol sector. "
/usr/share/vegastrike/modules/quests/quest_dispute.py:189: SyntaxWarning: invalid escape sequence '\T'
  text = "MALICIOUS MERCHANT MASSACRES MARXIST MERCY MISSION\\\Today, an unprecedented dispute about landing priorities took place close to a station in the Regallis system of Sol sector. "
/usr/share/vegastrike/modules/quests/quest_blockade.py:99: SyntaxWarning: invalid escape sequence '\I'
  text = "JUMP POINT BLOCKADE IN " + self.system + " SYSTEM\\\Intelligence has uncovered Aeran plans to invade Confederation space."
/usr/share/vegastrike/modules/quests/quest_shipyardbomb.py:63: SyntaxWarning: invalid escape sequence '\D'
  text = "NAVAL SHIPYARDS HIT BY BOMB\\\Disaster struck the Confederate Naval Shipyards orbiting Alpha Centauri hours ago, when a powerful explosive device detonated, crippling a fleet carrier that was nearing completion. "

@royfalk
Copy link
Contributor Author

royfalk commented Dec 6, 2024

These are the errors that I am seeing in the console:

I just took a look and these are indeed errors in the python scripts. For some reason, VS is now printing them, but they were always there.

Consider the first error in the string: "CEPHID SECURITY INITIATIVE GIVES TRAINING FOR FLIGHT SAFETY\\The ..."
There are three backslashes followed by capital T.

I extracted this to a simple python program:

some_string = '\T'
print(some_string)

and got the same warning.

For now, I don't plan to fix this, as there may be something I'm missing. The game is publishing this string to C++. I'll need to do some more digging.

@royfalk
Copy link
Contributor Author

royfalk commented Dec 6, 2024

It looks like the first warning gets triggered by finishing the tutorial. Can you confirm this?

@evertvorster
Copy link
Contributor

Hi there!

Firstly, what I am testing:
I'm merging in task_bug_924, task_more_ship_view and task_boost_json

Using the current master of Assets, with task_more_config applied

I see these errors pop up when starting the game. Not loading any game or starting a new campaign, just loading the game and exiting.

Of course, the more you play, the more these errors appears.

@BenjamenMeyer BenjamenMeyer added this to the 0.10.x milestone Dec 7, 2024
Remove Suse 15.2 and 15.3 - too old.
Fedora versions before 39 are possibly too old to support boost json. There's no version history because these versions are out of support.
Restore jammy and focal
- code ql - restrict to run on ubuntu 24.04. ubuntu-latest includes focal and jammy.
- gh-actions-pr.yml - add sudo to bootstrap
- vcpkg.json - add json. Prevent fail on windows
- boostrap - remove jammy and focal again. Turns out they use boost 1.74.
in github-actions.
Remove old OSes.
@royfalk
Copy link
Contributor Author

royfalk commented Dec 10, 2024

@BenjamenMeyer take a look at CICD build (rockylinux).

For some reason, all Linux distros use the apt-get of Ubuntu Noble. I don't even know what we're testing here.

@BenjamenMeyer
Copy link
Member

Looks like Windows is failing the builds on the Boost version as it has a newer version than expected.

@royfalk
Copy link
Contributor Author

royfalk commented Dec 15, 2024

@BenjamenMeyer

First, please turn your attention to my comment about Rocky Linux: "For some reason, all Linux distros use the apt-get of Ubuntu Noble. I don't even know what we're testing here."

Second, I've used the build-system-docker-images to try and reproduce the fail for ubuntu and failed. I downloaded noble, followed the github action. Even added the flags as required.

    1  git status
    2  dpkg -l | grep vim 
    3  cd ..
    4  cd Vega-Strike-Engine-Source/
    5  script/bootstrap 
    6  vim script/bootstrap
    7  apt install vim
    8  vim script/bootstrap
    9  script/bootstrap 
   10  echo $FLAGS
   11  export FLAGS="-DOpenGL_GL_PREFERENCE=LEGACY -DENABLE_PIE=OFF"
   12  script/cibuild

I am at a loss here.

@BenjamenMeyer
Copy link
Member

Yeah, I've had trouble reproducing it too. Not sure why.

@royfalk
Copy link
Contributor Author

royfalk commented Dec 16, 2024

Rocky Linux 9.4 can't compile on clang due to boostorg/mpl#69.

Add autoremove to bookworm - try to make it work
to use version 1.81 and not 1.74
@royfalk
Copy link
Contributor Author

royfalk commented Dec 16, 2024

@BenjamenMeyer please review and confirm following changes:

  • codeql.yml - move from ubuntu-latest (22) to 24.04
  • gh-actions-pr.yml
    • remove old OS (boost < 1.75)
    • disable rocky linux 9.4 due to bug
    • re-run bootstrap
  • bootstrap
    • debian bookworm - remove default installed boost1.74 and install 1.81.
    • Remove unsupported debian bullseye, ubuntu jammy and focal, Suse 15.2, 15.3, Rocky 8.0 to 8.10
  • docker-entrypoint.sh
    • rerun bootstrap inside docker
  • CMakeLists.txt
    • Suppress CMP0102 warning
    • Simplify boost process - remove option for internal boost and for old versions. If someone wants to, they can hack the game to do so. Why provide support for boost and not for other libraries?

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.

Migrate to the Boost JSON parser eventually?
3 participants