Fix some crashes 2025-01-13 #987

stephengtuggy · 2025-01-13T22:53:58Z

Code Changes:

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

Issues:

Addresses Game crashes upon loading a new campaign. #981

Purpose:

What is this pull request trying to do? Fix Game crashes upon loading a new campaign. #981 , along with several other crashes
What release is this for? 0.10.x
Is there a project or milestone we should apply this to? 0.10.x

…used in one place.

…rData

…(...)`

…..)`

…`.at(...)`

…sh::~Mesh() destructor

engine/src/cmd/basecomputer.cpp

stephengtuggy · 2025-01-14T00:26:44Z

Note that this PR includes most of the changes from #974 , so should probably be tackled after #974 is out of the way.

… those builds are failing anyway

…ystem_error

evertvorster · 2025-01-14T06:25:39Z

Hi there!

This does fix the crashing when loading a campaign #981

Pressing "m" in flight still causes a segfault.

Also, it breaks ship info and purchasing repair and refuel's description, among other things, on a new campaign.

BenjamenMeyer

Is this on top of the other work I reviewed? seems like a lot of duplication.

engine/setup/src/include/display.h

…1-11

…Position: ..." and "Target Position: ..." text string

…so fix a few more references to 8-sided armor

…11' into fix/index_out_of_bounds_2025-01-11

unblocking

evertvorster · 2025-01-15T05:40:27Z

Just ran a little test now, and the python errors have changed somewhat.
When clicking on ship's information, it states that ship_view.py was not found, and when clicking on player info, it states that player_info.py was not found.
In upgrades, it complains about upgrade_view.py not being found.

So, you are on the right track, I believe.

…Python file!

stephengtuggy · 2025-01-15T06:30:27Z

Just ran a little test now, and the python errors have changed somewhat. When clicking on ship's information, it states that ship_view.py was not found, and when clicking on player info, it states that player_info.py was not found. In upgrades, it complains about upgrade_view.py not being found.

So, you are on the right track, I believe.

LOL that's good to hear.

In my very latest changes, I have succeeded in getting the .py files to load. Now it's just a matter of debugging the Python code! 😃

evertvorster · 2025-01-15T07:10:06Z

Much, much improved!

Upgrades now work, as do player information.
The only bit that still throws an error is ship info:

Once this is sorted, I'll test all the major functions of the game on my system. They all worked pretty much ok the last time I tested, but it is best to make sure.

stephengtuggy · 2025-01-15T07:18:09Z

Much, much improved!

Upgrades now work, as do player information. The only bit that still throws an error is ship info:

Once this is sorted, I'll test all the major functions of the game on my system. They all worked pretty much ok the last time I tested, but it is best to make sure.

That's really good to hear! Ship View is now working for me too. For that, I had to make some changes to ship_view.py in the assets. I have a PR open for that here: vegastrike/Assets-Production#144

evertvorster · 2025-01-15T07:38:23Z

Looking VERY good!

Only issue I see now is that in ship info it tells me that there is no shielding:

This may be because of the change in how many facets a shield has, and not related to this PR at all. Once this one is merged, I'll raise an issue on the engine for this one specifically.

evertvorster

Tested this for every major function that I can think of, and everything works.
I did notice a few bugs from long ago, but not relevant to this PR.

kheckwrecker · 2025-01-15T16:36:57Z

I tested this in Windows. I can now launch a new campaign, which is good. When I flew to a nearby mining base and requested docking clearance, however, it crashed to the desktop.

evertvorster · 2025-01-15T16:40:31Z

I tested this in Windows. I can now launch a new campaign, which is good. When I flew to a nearby mining base and requested docking clearance, however, it crashed to the desktop.

Do you have any logs for this? I have also had a crash, but thought it was due to my hitting the "pause" button by mistake. It's right next to the Delete/Insert key on this laptop. Unfortunately I did not grab a log, and it did not crash on the next couple of tries.

kheckwrecker · 2025-01-15T18:22:13Z

The only log I can find (in my .vegastrike/logs directory) has nothing in it related to the crash. And worse than that, when I tried it again, it works fine without any crash. I hate intermittent bugs.

stephengtuggy · 2025-01-15T19:42:45Z

@evertvorster that's awesome!

@kheckwrecker I know, those are the worst. Unfortunately, the game has had intermittent crashes for quite a while. Probably not much we can do about that in this PR. Soon, though. Hopefully.

royfalk · 2025-01-16T05:09:33Z

I'm going to hold off on reviewing this until we merge #974. There are too many changes otherwise.

stephengtuggy · 2025-01-16T05:44:32Z

engine/src/cmd/controls_factory.cpp

-        Font font(font_array[0], font_array[1]);
-        c->setFont(font);
+        if (font_array.size() < 2) {
+            VS_LOG(error, "controls_factory getControl(): font_array doesn't have enough elements");


I don't know why this condition is happening. We will probably want to investigate further. But for now, this keeps the game from crashing at this point.

So, this raises a bigger philosophical question of how much crap are we willing to let the engine take from the assets?

For JSON files, the file obviously needs to be formatted correctly or parsing will fail. Most JSON files are crucial to the game, so may as well crash it with an error.

But this is inside content and for a specific control so it points to an error in how I converted fonts to controls. Best open an issue/bug for this.

Filed as vegastrike/Assets-Masters#56

engine/src/cmd/unit_json_factory.cpp

engine/src/python/init.cpp

royfalk

Still a long review but much better. Thanks.

@BenjamenMeyer we may need to think about syncing our efforts so we don't write the same code twice. At least some of the code is stuff I've written but was waiting for the cmake stuff to get merged and splitting other stuff from it (like #969 ) so the actual review of the PR will be shorter and easier.

royfalk · 2025-01-16T15:22:19Z

engine/src/cmd/basecomputer.cpp

@@ -114,8 +116,8 @@ std::vector<std::string> getWeapFilterVec() {
 std::vector<std::string> weapfiltervec = getWeapFilterVec();

 bool upgradeNotAddedToCargo(std::string category) {
-    for (unsigned int i = 0; i < weapfiltervec.size(); ++i) {
-        if (weapfiltervec[i].find(category) == 0) {
+    for (const auto & i : weapfiltervec) {


I personally don't have a problem with auto, but @BenjamenMeyer frowns upon them.
Therefore, where I can deduce the type, I'll use it.

@BenjamenMeyer has said previously that he's OK with auto for iterators.

royfalk · 2025-01-16T15:23:24Z

engine/src/cmd/basecomputer.cpp

@@ -527,7 +529,7 @@ void BaseComputer::constructControls(void) {
        return;
    }

-    if (m_displayModes.size() != 1 || m_displayModes[0] != NETWORK) {
+    if (m_displayModes.size() != 1 || m_displayModes.at(0) != NETWORK) {


I assume these fail for some OS'es. I have the dangest errors in the CI, where my machine blissfully compiles and runs.

These don't fail to compile on any OS particularly. This is just to add bounds checking, to make sure we're not reading or writing outside the bounds of the collection.

royfalk · 2025-01-16T15:26:41Z

engine/src/cmd/basecomputer.cpp

@@ -3942,7 +3943,7 @@ string buildUpgradeDescription(Cargo &item) {
    const std::string key = item.GetName() + "__upgrades";
    PyObject* args = PyTuple_Pack(1, PyUnicode_FromString(key.c_str()));
    const std::string text = GetString("get_upgrade_info", "upgrade_view",
-        "python/base_computer/upgrade_view.py", args);
+        "upgrade_view.py", args);


Can you expand on why you did this?

You were specifying the subfolder ("python/base_computer") twice. Once here, and once in the Python path initialization code. That's part of why VS was failing to locate these Python scripts.

Now the subfolder is specified in only one place: initpaths(). Which is more or less in keeping with the way existing code did things.

I'm concerned that we could potentially have two utils.py somewhere with unexpected and hard to debug errors.

royfalk · 2025-01-16T15:32:49Z

engine/src/cmd/controls_factory.cpp

-        Font font(font_array[0], font_array[1]);
-        c->setFont(font);
+        if (font_array.size() < 2) {
+            VS_LOG(error, "controls_factory getControl(): font_array doesn't have enough elements");


So, this raises a bigger philosophical question of how much crap are we willing to let the engine take from the assets?

For JSON files, the file obviously needs to be formatted correctly or parsing will fail. Most JSON files are crucial to the game, so may as well crash it with an error.

But this is inside content and for a specific control so it points to an error in how I converted fonts to controls. Best open an issue/bug for this.

royfalk · 2025-01-16T15:35:20Z

engine/src/cmd/damageable.cpp

@@ -524,7 +524,7 @@ float Damageable::RShieldData() const {
 void Damageable::ArmorData(float armor[8]) const {
    Damageable *damageable = const_cast<Damageable *>(this);
    DamageableLayer armor_layer = damageable->GetArmorLayer();
-    for (int i = 0; i < 8; i++) {


Yeah. You caught a bug of mine here. Did this segfault or fill the array with junk?

I'm not sure if I actually hit this section of code during my testing. If I had, though, I think a segfault would have been most likely. The destination array was too small to write 8 elements to.

royfalk · 2025-01-16T15:40:11Z

engine/src/gfx/vdu.cpp

-            (armor[1] + armor[3] + armor[5]
-                    + armor[7]) / (float) (StartArmor[1] + StartArmor[3] + StartArmor[5] + StartArmor[7]),
-            parent->GetHull() / (*maxhull), true, false);
+    DrawHUDSprite(this, draw_damage_sprite ? parent->getHudImage() : nullptr, .6, x, y, w, h,


A lot of the stuff for armor and 8 facets is stuff I'm waiting to submit. It would be interesting to compare and also a bit frustrating.

Yeah. We would do well to communicate a bit better in real time about stuff we're working on. That isn't always feasible though, what with time zone differences and such.

engine/src/python/infra/get_string.cpp

royfalk

LGTM

stephengtuggy · 2025-01-16T21:44:39Z

LGTM

Thanks!

stephengtuggy added 11 commits January 11, 2025 18:23

unit_csv.cpp: Handle shield values that cannot be converted to an int

b2de538

Get rid of vs_vector. It was a bad idea to begin with, and only ever …

0132b05

…used in one place.

Drop reference to vs_vector.h in Stdafx.h

61c58b2

damageable.cpp: Fix index-out-of-bounds condition in Damageable::Armo…

1ab4736

…rData

basecomputer.cpp: Replace all uses of operator[] on vectors with `.at…

21440fd

…(...)`

gl_texture.cpp: Replace all uses of operator[] on vectors with `.at(.…

b1d4927

…..)`

controls_factory.cpp: Replace all uses of operator[] on vectors with …

8988b4c

…`.at(...)`

component_utils.cpp: Fix "Yaw_Governor_Right" and "Yaw_Governor_Left"

fd09cd8

component_utils.cpp: Spiffy up copyright header

af66299

Merge branch 'master' into fix/index_out_of_bounds_2025-01-11

34cb8e1

mesh_gfx.cpp: Use std::stable_partition deletion method (twice) in Me…

7604ba3

…sh::~Mesh() destructor

stephengtuggy added the bug label Jan 13, 2025

stephengtuggy added this to the 0.10.x milestone Jan 13, 2025

stephengtuggy self-assigned this Jan 13, 2025

stephengtuggy added 2 commits January 13, 2025 15:05

mesh_gfx.cpp: Fix data type

c4a2fdc

mesh_gfx.cpp: More logging

0933770

github-advanced-security bot found potential problems Jan 14, 2025

View reviewed changes

engine/src/cmd/basecomputer.cpp Fixed Show fixed Hide fixed

engine/src/cmd/basecomputer.cpp Fixed Show fixed Hide fixed

stephengtuggy added 2 commits January 13, 2025 16:08

controls_factory.cpp: Handle fewer font elements than expected

8d9132d

mesh_gfx.cpp: Log deletions only when 1 or more elements were deleted

8edfc65

stephengtuggy added 6 commits January 13, 2025 16:33

basecomputer.cpp: Fix two CodeQL alerts

2af613f

mesh_gfx.cpp: More fine-tuning

b7e9964

gh-actions-pr.yml: Comment out openSUSE Leap and Rocky Linux 9, since…

afce86c

… those builds are failing anyway

unit_json_factory.cpp: Handle JSON parsing errors

30a7f68

unit_json_factory.cpp: VS_LOG_FLUSH_EXIT on JSON parsing errors

82b09eb

unit_json_factory.cpp: catch std::exception instead of boost::json::s…

3781301

…ystem_error

stephengtuggy marked this pull request as ready for review January 14, 2025 03:41

BenjamenMeyer previously requested changes Jan 14, 2025

View reviewed changes

engine/setup/src/include/display.h Outdated Show resolved Hide resolved

Merge branch 'sgt_mac_2025-01-09' into fix/index_out_of_bounds_2025-0…

4cd3f7d

…1-11

stephengtuggy added 5 commits January 14, 2025 18:07

vsfilesystem.cpp, xml_support.h: Omit some commented-out code

7a92f2a

cockpit.cpp: Use boost::format instead of sprintf to build the "Your …

481a838

…Position: ..." and "Target Position: ..." text string

Add some more Python error handling, logging, and search paths

ce9c51f

basecomputer.cpp, unit_generic.cpp: Fix a few Python module paths; al…

df25d52

…so fix a few more references to 8-sided armor

Merge remote-tracking branch 'origin/fix/index_out_of_bounds_2025-01-…

6dcf929

…11' into fix/index_out_of_bounds_2025-01-11

Got the ship_view Python module to load! Now to fix the code in that …

d597c12

…Python file!

Merge branch 'master' into fix/index_out_of_bounds_2025-01-11

09f516c

evertvorster approved these changes Jan 15, 2025

View reviewed changes

royfalk mentioned this pull request Jan 16, 2025

Enhancement: Clean up the Stdafx.h usage #989

Merged

2 tasks

Merge branch 'master' into fix/index_out_of_bounds_2025-01-11

36c49d7

stephengtuggy commented Jan 16, 2025

View reviewed changes

engine/src/cmd/unit_json_factory.cpp Show resolved Hide resolved

stephengtuggy commented Jan 16, 2025

View reviewed changes

engine/src/python/init.cpp Show resolved Hide resolved

royfalk reviewed Jan 16, 2025

View reviewed changes

stephengtuggy mentioned this pull request Jan 16, 2025

Hitting error in Vega Strike: "controls_factory getControl(): font_array doesn't have enough elements" vegastrike/Assets-Masters#56

Open

royfalk approved these changes Jan 16, 2025

View reviewed changes

stephengtuggy merged commit 871a692 into vegastrike:master Jan 16, 2025
16 checks passed

stephengtuggy deleted the fix/index_out_of_bounds_2025-01-11 branch January 16, 2025 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some crashes 2025-01-13 #987

Fix some crashes 2025-01-13 #987

stephengtuggy commented Jan 13, 2025 •

edited

Loading

stephengtuggy commented Jan 14, 2025

evertvorster commented Jan 14, 2025

BenjamenMeyer left a comment

evertvorster commented Jan 15, 2025

stephengtuggy commented Jan 15, 2025

evertvorster commented Jan 15, 2025

stephengtuggy commented Jan 15, 2025

evertvorster commented Jan 15, 2025

evertvorster left a comment

kheckwrecker commented Jan 15, 2025

evertvorster commented Jan 15, 2025

kheckwrecker commented Jan 15, 2025

stephengtuggy commented Jan 15, 2025

royfalk commented Jan 16, 2025

stephengtuggy Jan 16, 2025

royfalk Jan 16, 2025

stephengtuggy Jan 16, 2025

royfalk left a comment

royfalk Jan 16, 2025

stephengtuggy Jan 16, 2025

royfalk Jan 16, 2025

stephengtuggy Jan 16, 2025

royfalk Jan 16, 2025

stephengtuggy Jan 16, 2025

royfalk Jan 16, 2025

royfalk Jan 16, 2025

royfalk Jan 16, 2025

stephengtuggy Jan 16, 2025

royfalk Jan 16, 2025

stephengtuggy Jan 16, 2025

royfalk left a comment

stephengtuggy commented Jan 16, 2025

Fix some crashes 2025-01-13 #987

Fix some crashes 2025-01-13 #987

Conversation

stephengtuggy commented Jan 13, 2025 • edited Loading

stephengtuggy commented Jan 14, 2025

evertvorster commented Jan 14, 2025

BenjamenMeyer left a comment

Choose a reason for hiding this comment

evertvorster commented Jan 15, 2025

stephengtuggy commented Jan 15, 2025

evertvorster commented Jan 15, 2025

stephengtuggy commented Jan 15, 2025

evertvorster commented Jan 15, 2025

evertvorster left a comment

Choose a reason for hiding this comment

kheckwrecker commented Jan 15, 2025

evertvorster commented Jan 15, 2025

kheckwrecker commented Jan 15, 2025

stephengtuggy commented Jan 15, 2025

royfalk commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

royfalk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

royfalk left a comment

Choose a reason for hiding this comment

stephengtuggy commented Jan 16, 2025

stephengtuggy commented Jan 13, 2025 •

edited

Loading