Upgrade SDL to version 2 #792

royfalk · 2023-08-07T04:02:43Z

This upgrades SDL to version 2.
This is needed for dear imGui and also a good thing in general.
This PR touches inputs and not much else. Things to test are keyboard, mouse and joystick.

Code Changes:

Have the PR Validation Tests been run? See https://github.com/vegastrike/Vega-Strike-Engine-Source/wiki/Pull-Request-Validation

Issues:

Joystick not tested (need someone to help)
Buy new ship fails with mesh error. Almost certainly Unit destructor crashes the game #716.

BenjamenMeyer

Exciting to see this. I'd like to play test before I approve.

royfalk · 2023-08-09T05:44:27Z

@Loki1950 , I'd like you to play test this with a joystick as I don't have one setup.

https://stackoverflow.com/questions/27567846/how-can-i-check-out-a-github-pull-request-with-git

royfalk · 2023-08-09T05:49:34Z

Also, it looks like we need to add sudo apt-get install libsdl2-2.0-14 to our test environments. Is this done somewhere in the code?

BenjamenMeyer · 2023-08-09T15:21:44Z

Also, it looks like we need to add sudo apt-get install libsdl2-2.0-14 to our test environments. Is this done somewhere in the code?

In our dependency scripts.

https://github.com/vegastrike/Vega-Strike-Engine-Source/blob/master/script/bootstrap

BenjamenMeyer · 2023-08-10T04:00:55Z

@royfalk I installed libsdl2-dev and libsdl2-gfx-dev. Is there anything else? There's also libsdl2-mixer-dev which we may want to eventually use too.

Any how...what version of Assets-Production are you using? I rebuilt by ~/.vegastrike directory, rebuilt the settings, and then tried to load a Campaign (new game) and it kinda of worked but then crashed because of a JSON issue:

::: Engine Version 0.9.0.e5691f3b1 :::
::: Asset API Version 2 :::
Loading active missions True
  IS NOW AT  Special/Empty

Launching bases for Special/Empty
Adding news
terminate called after throwing an instance of 'json::parsing_error'
  what():  Input was not an array
Aborted (core dumped)

Guessing I need to update the Assets-Production some how.

royfalk · 2023-08-12T08:10:29Z

I usually work against asset-production master. I see I split the units in a relatively recent commit so try updating it.

BenjamenMeyer · 2023-08-12T18:12:45Z

I usually work against asset-production master. I see I split the units in a relatively recent commit so try updating it.

Yeah, that was it. My last couple employers don't do forking, so I've been getting use to just pulling origin and needed to pull upstream instead. It's relatively stable now - at least with respect to your changes; only issues I'm running into were previously known and now SDL related. Still wish we could improve our logging when JSON stuff fails so we could point this out better.

- Log which build is being used - For DEBUG builds do not do *any* optimization so that source can be easily inspected under GDB/Debugger - Add logging of JSON data to help with Debugging - Use an iterable loop in the GFX Mesh instead of index based for better reliability; old loop could end up in weird circumstances

BenjamenMeyer · 2023-08-12T19:11:25Z

Files royfalk#1 with a couple small fixes.

stephengtuggy · 2023-08-12T21:37:08Z

Also, it looks like we need to add sudo apt-get install libsdl2-2.0-14 to our test environments. Is this done somewhere in the code?

In our dependency scripts.

https://github.com/vegastrike/Vega-Strike-Engine-Source/blob/master/script/bootstrap

Don't forget to update https://github.com/vegastrike/build-system-docker-images/blob/master/script/bootstrap as well. It will be an apt command on some distros; an RPM-based command on others; etc. But hopefully you can just add SDL2 to the existing package lists, without too much trouble. Let me know if you want any help with that.

We will also need to adjust vcpkg.json for the Windows builds. We will need to adjust the macOS CI scaffolding as well.

stephengtuggy · 2023-08-12T21:38:08Z

I usually work against asset-production master. I see I split the units in a relatively recent commit so try updating it.

Yeah, that was it. My last couple employers don't do forking, so I've been getting use to just pulling origin and needed to pull upstream instead. It's relatively stable now - at least with respect to your changes; only issues I'm running into were previously known and now SDL related. Still wish we could improve our logging when JSON stuff fails so we could point this out better.

Yeah, pulling upstream can be pretty important. I try to do that on a regular basis.

stephengtuggy · 2023-08-12T21:41:00Z

engine/src/gfx/coord_select.cpp

@@ -38,10 +37,11 @@ int CoordinateSelectChange = 0;
 int CoordinateSelectmousex;
 int CoordinateSelectmousey;
 extern Vector MouseCoordinate(int mouseX, int mouseY);
-extern KBSTATE keyState[LAST_MODIFIER][KEYMAP_SIZE];
+
+extern KBSTATE mouseButtonState;


Um... what? Is it keyboard state or mouse button state?

At best, this is very confusing; at worst, it may not be correct at all.

It's interesting....
https://wiki.libsdl.org/SDL2/SDL_GetMouseState - 32bit value
https://wiki.libsdl.org/SDL2/SDL_GetKeyboardState - 8bit value

Checks for a modifier key is also pressed ie:Ctrl,Alt etc

ahh...it's our own definition - https://github.com/vegastrike/Vega-Strike-Engine-Source/blob/master/engine/src/in.h#L25

Perhaps it should be renamed to BUTTON_STATE or something - equally applies to Mouse, Keyboard, and even Joysticks and other inputs that way.

I like the BUTTON_STATE idea.

I'm mixed on whether that should be in this PR or a separate one

I don't want this in this PR. It's a mostly cosmetic thing and will impact 520 instances in around 30 files. It's better to do it in a separate PR.

@royfalk OK, fair enough. That can be done in a separate PR.

stephengtuggy

See my comment about keyboard state vs. mouse button state.

Protect from dividing by zero, and Log when it's attempted

royfalk · 2023-08-14T15:38:32Z

Sorry. I'll try to get to all of this tomorrow.

royfalk · 2023-08-16T19:54:50Z

Nope I'm flying tomorrow. Will have to wait till middle of next week.

royfalk · 2023-08-22T07:16:21Z

Still wish we could improve our logging when JSON stuff fails so we could point this out better.

I think this is a limitation of the JSON library we're using. I suspect that when we move to boost, we'll get better errors.
I had to use this because we still supported older boost versions and JSON support is recent. However, the move should be very straightforward.

royfalk · 2023-08-22T15:55:44Z

Don't forget to update https://github.com/vegastrike/build-system-docker-images/blob/master/script/bootstrap as well.

We will also need to adjust vcpkg.json for the Windows builds. We will need to adjust the macOS CI scaffolding as well.

Should we get this PR ready for commit before changing that as it is in a different repo?

royfalk · 2023-08-23T09:00:53Z

@BenjamenMeyer @Loki1950 please try and build and play using a joystick. Before we go and switch dependencies, I'd like to know basic game play isn't affected.

BenjamenMeyer · 2023-08-23T13:19:30Z

@royfalk I don't have a joystick set working yet at all. I think @Loki1950 does though.

I haven't seen anything unexpected this far aside from what seems like losing control input randomly for left/right movement, but I don't think that's new.

Loki1950 · 2023-08-23T16:59:05Z

@stephengtuggy has joystick same model as mine even I am limited to my laptop ATM which does not have a build environment.

stephengtuggy · 2023-09-03T01:42:27Z

Sorry, guys, I've been pretty busy the last few weeks. New job and such. I will try to test these changes some this weekend. With my joystick.

We really need to get the CI fixed. Both in general, and for this PR specifically. I don't have a high degree of confidence in this code without the CI working (even if that's not the fault of these changes). There's just too much uncertainty involved.

Also, if the left-right control cuts in and out, that's concerning to me. I don't think it was like that before. Was it?

Another thing. Why do we have to update to the latest Assets-Production upstream master in order to run the latest game engine code? I thought we agreed that the game engine was supposed to remain backwards compatible with the old asset formats?

Finally: A lot of these "JSON errors" seem to come from expected JSON files just not being present at all. Can't we check for those conditions specifically, and display an error message that is more specific about the files being missing? (If we error out at all in that case -- see last paragraph.)

We discussed the keyboard state vs. mouse state nomenclature issue, and agreed to tackle that later.

royfalk · 2023-09-04T15:36:17Z

We really need to get the CI fixed.

I am a lot less familiar with the CI. What's broken and how can I help?

Also, if the left-right control cuts in and out, that's concerning to me. I don't think it was like that before. Was it?

I have not experienced that in my testing. The game has many bugs and instabilities. Not sure if this is new or specific to @BenjamenMeyer 's system and hardware.

Another thing. Why do we have to update to the latest Assets-Production upstream master in order to run the latest game engine code? I thought we agreed that the game engine was supposed to remain backwards compatible with the old asset formats?

Finally: A lot of these "JSON errors" seem to come from expected JSON files just not being present at all. Can't we check for those conditions specifically, and display an error message that is more specific about the files being missing? (If we error out at all in that case -- see last paragraph.)

This is partly a limitation of simpleson, the JSON parsing library I've used. I wanted boost, but we did not support this version yet. We need 1.75 for it.
I'll open an issue for it and try and get it fixed soon.
However, regarding the split to multiple files, this is more difficult. My plan is to make specific files routed to specific constructors. It's very hard to keep the assets stable in such a case.

stephengtuggy · 2023-09-04T22:41:58Z

We really need to get the CI fixed.

I am a lot less familiar with the CI. What's broken and how can I help?

You could maybe start by reviewing #789 .

Also, if the left-right control cuts in and out, that's concerning to me. I don't think it was like that before. Was it?

I have not experienced that in my testing. The game has many bugs and instabilities. Not sure if this is new or specific to @BenjamenMeyer 's system and hardware.

This PR specifically touches code related to the keyboard and mouse inputs. If we are seeing issues related to that functionality -- issues that might be new -- that, to me, is a major red flag. I'm not willing to move forward with merging this PR until we have a lot more certainty about where these issues are coming from.

Another thing. Why do we have to update to the latest Assets-Production upstream master in order to run the latest game engine code? I thought we agreed that the game engine was supposed to remain backwards compatible with the old asset formats?

Finally: A lot of these "JSON errors" seem to come from expected JSON files just not being present at all. Can't we check for those conditions specifically, and display an error message that is more specific about the files being missing? (If we error out at all in that case -- see last paragraph.)

This is partly a limitation of simpleson, the JSON parsing library I've used. I wanted boost, but we did not support this version yet. We need 1.75 for it. I'll open an issue for it and try and get it fixed soon. However, regarding the split to multiple files, this is more difficult. My plan is to make specific files routed to specific constructors. It's very hard to keep the assets stable in such a case.

Backwards compatibility of the asset file formats is a requirement for 0.9.x. If we cannot sustainably support both old and new formats, then we need to go back to the old asset formats only, at least until 0.10.x.

BenjamenMeyer · 2023-09-05T03:04:44Z

Sorry, guys, I've been pretty busy the last few weeks. New job and such. I will try to test these changes some this weekend. With my joystick.

We really need to get the CI fixed. Both in general, and for this PR specifically. I don't have a high degree of confidence in this code without the CI working (even if that's not the fault of these changes). There's just too much uncertainty involved.

I'll try to take a look. Sept is going to be busy for me IRL though so will see what I can do.

Also, if the left-right control cuts in and out, that's concerning to me. I don't think it was like that before. Was it?

I think it may have been; may be not as often but I don't think its new. So I'm not concerned about it for this branch unless others are seeing it.

Another thing. Why do we have to update to the latest Assets-Production upstream master in order to run the latest game engine code? I thought we agreed that the game engine was supposed to remain backwards compatible with the old asset formats?

The point of the Engine Version was for Game Assets to be able to determine if they were compatible. If we need to make a breaking change on master since the last release then we need to rev that version so the Game Assets will error out or at least warn accordingly. We already rev'd it for 0.9.x to Version 2 (https://github.com/vegastrike/Vega-Strike-Engine-Source/blob/master/engine/CMakeLists.txt#L79) - 0.8.x was Version 1 and 0.7.x and earlier is considered Version 0. So we're good to make some breaking changes in 0.9.x. We should update the dev branches (master) our two associated assets (VS:UtCS/Assets Production; PWCU) so they continue to function though as we do.

BenjamenMeyer · 2023-09-05T03:08:55Z

@royfalk The Windows build failures might be specified to this branch:

 D:\a\Vega-Strike-Engine-Source\Vega-Strike-Engine-Source\engine\src\in_kb.h(32,10): fatal error C1083: Cannot open include file: 'SDL2/SDL.h': No such file or directory [D:\a\Vega-Strike-Engine-Source\Vega-Strike-Engine-Source\build\vegastrike-testing.vcxproj]

We probably need to update the Windows dependencies to include SDL2 as well.
See https://github.com/royfalk/Vega-Strike-Engine-Source/blob/master/engine/vcpkg.json#L29

Fortunately looks like we can lookup packages easily enough: https://vcpkg.io/en/packages

- Add SDL2 to the VCPkg configuration used by the Windows CI

BenjamenMeyer · 2023-09-07T04:44:24Z

@royfalk I added SDL2 to the vcpkg config in my PR against your fork/branch.

BenjamenMeyer · 2023-09-17T20:24:24Z

@royfalk please merge royfalk#1 to help complete this branch.

Bug Fixes

BenjamenMeyer · 2023-09-29T14:35:26Z

https://github.com/vegastrike/Vega-Strike-Engine-Source/actions/runs/6352033464/job/17254218942?pr=792#step:6:2846

Seems some of the platforms are failing on:

In file included from /usr/src/Vega-Strike-Engine-Source/engine/src/config_xml.h:35:
/usr/src/Vega-Strike-Engine-Source/engine/src/in_kb.h:32:10: fatal error: 'SDL2/SDL.h' file not found
#include <SDL2/SDL.h>
         ^~~~~~~~~~~~

I wonder why there's an inconsistency since some platforms are building just fine.

Interesting...Mac and Windows are now passing the build, but Linux is failing due to the above. 🤷
We probably should fix this before merging.

BenjamenMeyer

Sadly I think we need to hold this until we resolve the CI issue since this is directly related specifically to this PR and would break our primary platform (Linux)...otherwise we should be good to go.
#792 (comment)

BenjamenMeyer

see my previous couple comments

BenjamenMeyer · 2023-09-30T06:06:04Z

Linux CI will pass once we update the images - currently in progress - vegastrike/build-system-docker-images#43

BenjamenMeyer

Approving now that CI is passing

Upgrade SDL to version 2

e5691f3

royfalk requested review from BenjamenMeyer, stephengtuggy and Loki1950 August 7, 2023 04:02

royfalk self-assigned this Aug 7, 2023

Loki1950 approved these changes Aug 7, 2023

View reviewed changes

BenjamenMeyer reviewed Aug 7, 2023

View reviewed changes

stephengtuggy reviewed Aug 12, 2023

View reviewed changes

stephengtuggy previously requested changes Aug 12, 2023

View reviewed changes

BenjamenMeyer added 2 commits August 13, 2023 02:04

Safe Iterator Removal

ff9bf4e

Bug Fix: Protect & Log

58f8e71

Protect from dividing by zero, and Log when it's attempted

Modify bootstrap to use SDL2

2d6a4cb

stephengtuggy self-requested a review September 3, 2023 02:14

Loki1950 approved these changes Sep 5, 2023

View reviewed changes

Bug Fix: Windows CI Builds

ea61472

- Add SDL2 to the VCPkg configuration used by the Windows CI

royfalk mentioned this pull request Sep 16, 2023

Task aug unit #796

Merged

1 task

Merge pull request #1 from BenjamenMeyer/bmeyer_bugfix_imgui_1

868a1dc

Bug Fixes

BenjamenMeyer approved these changes Sep 29, 2023

View reviewed changes

BenjamenMeyer reviewed Sep 29, 2023

View reviewed changes

BenjamenMeyer requested changes Sep 29, 2023

View reviewed changes

Loki1950 approved these changes Sep 30, 2023

View reviewed changes

BenjamenMeyer mentioned this pull request Sep 30, 2023

Enhancement: SDL2 support vegastrike/build-system-docker-images#43

Merged

2 tasks

BenjamenMeyer approved these changes Oct 5, 2023

View reviewed changes

BenjamenMeyer merged commit a720ce5 into vegastrike:master Oct 5, 2023
53 checks passed

Upgrade SDL to version 2 #792

Upgrade SDL to version 2 #792

Conversation

royfalk commented Aug 7, 2023 • edited Loading

BenjamenMeyer left a comment

Choose a reason for hiding this comment

royfalk commented Aug 9, 2023

royfalk commented Aug 9, 2023

BenjamenMeyer commented Aug 9, 2023 • edited Loading

BenjamenMeyer commented Aug 10, 2023 • edited Loading

royfalk commented Aug 12, 2023

BenjamenMeyer commented Aug 12, 2023

BenjamenMeyer commented Aug 12, 2023

stephengtuggy commented Aug 12, 2023

stephengtuggy commented Aug 12, 2023

stephengtuggy Aug 12, 2023

Choose a reason for hiding this comment

BenjamenMeyer Aug 12, 2023

Choose a reason for hiding this comment

Loki1950 Aug 12, 2023

Choose a reason for hiding this comment

BenjamenMeyer Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

stephengtuggy Aug 14, 2023

Choose a reason for hiding this comment

BenjamenMeyer Aug 14, 2023

Choose a reason for hiding this comment

royfalk Aug 22, 2023

Choose a reason for hiding this comment

stephengtuggy Sep 3, 2023

Choose a reason for hiding this comment

stephengtuggy left a comment

Choose a reason for hiding this comment

royfalk commented Aug 14, 2023

royfalk commented Aug 16, 2023

royfalk commented Aug 22, 2023 • edited Loading

royfalk commented Aug 22, 2023

royfalk commented Aug 23, 2023

BenjamenMeyer commented Aug 23, 2023

Loki1950 commented Aug 23, 2023

stephengtuggy commented Sep 3, 2023

royfalk commented Sep 4, 2023

stephengtuggy commented Sep 4, 2023

BenjamenMeyer commented Sep 5, 2023

BenjamenMeyer commented Sep 5, 2023 • edited Loading

BenjamenMeyer commented Sep 7, 2023

BenjamenMeyer commented Sep 17, 2023

BenjamenMeyer commented Sep 29, 2023 • edited Loading

BenjamenMeyer left a comment

Choose a reason for hiding this comment

BenjamenMeyer left a comment

Choose a reason for hiding this comment

BenjamenMeyer commented Sep 30, 2023

BenjamenMeyer left a comment

Choose a reason for hiding this comment

royfalk commented Aug 7, 2023 •

edited

Loading

BenjamenMeyer commented Aug 9, 2023 •

edited

Loading

BenjamenMeyer commented Aug 10, 2023 •

edited

Loading

BenjamenMeyer Aug 13, 2023 •

edited

Loading

royfalk commented Aug 22, 2023 •

edited

Loading

BenjamenMeyer commented Sep 5, 2023 •

edited

Loading

BenjamenMeyer commented Sep 29, 2023 •

edited

Loading