Source Code Project Mantis - FSSCP
View Issue Details
0002326FSSCPmultiplayerpublic2010-10-14 16:172013-01-30 02:19
Assigned ToThe_E 
PlatformOSOS Version
Product Version3.6.13 
Target Version3.6.14Fixed in Version3.6.13 
Summary0002326: Assert: hull_pct > 0.0f && hull_pct <= 1.0f in ship.cpp line 8298
DescriptionStandalone again. This one has happened several times now and twice in the last week. Happens at player respawn. Attaching call stack and variables. Will attach log as soon as I get out of the debugger.
TagsNo tags attached.
related to 0002783resolved Goober5000 Multiplayer respawn code can't keep its ship classes straight 
related to 0002378assigned karajorma Ships assigned in loadout respawn as default ships. 
Attached Filestxt hull_over_100.txt (111,408) 2010-10-14 16:17
rar hull_over_100.rar (391,787) 2010-10-14 16:17
patch 2326.patch (6,129) 2011-06-21 17:47
patch 2326-2.patch (2,252) 2012-12-01 12:32
rar 2326_new.rar (78,349) 2013-01-16 01:30

2010-11-11 03:41   
Why are we asserting there, when we can simply CLAMP() the values to valid ranges? In this case, it looks to me like a case of float imprecision.

Fix committed in revision 6724
2010-11-11 04:51   
Reopening, as this does not address the root cause.

Judging by what is printed in the log and callstack, the Objects[] and Ships[] arrays seem to have gotten out of sync somehow. Note that I couldn't reproduce this in single player.
2010-11-13 21:20   
We're asserting there because those values should never be outside the range in those locations. :p

It's not a bad thing to have Assert and Int3 in the codebase. They indicate a failure of design-by-contract, i.e. a bug in the actual program, in contrast to Warning or Error which indicate bugs caused by invalid game data.
2010-11-13 22:56   
Yeah, I know, Which is why I reverted it.
2010-11-13 22:56   
And I know which is why I told him the problem was deeper the he though.
2010-12-03 18:51   
Just had it happen with shields as well which is around 13 lines later. Pretty much identical other then hull vs shields.
2011-01-12 17:03   
Is r6429 not a likely culprit for this? It was committed only a month and a half before this bug was filed. After seeing FUBAR's comments in IRC today:

[15:46] <FUBAR> it seems to happen anytime the player chooses a ship other then default
[15:47] <FUBAR> respawns and it appears to try to calculate hull strength based on the original ship not the one he selected in loadout
[15:47] <FUBAR> which at respawn do we even care about anything besides special hits? You have a brand new ship it should be at 100%
[15:48] <FUBAR> and if it gets by that it crashes on shields down below

Then seeing the commit message:
"maintain the original hull, shield, and subsystem percentages when changing ship class, since we might be changing it from a briefing loadout (Mantis 0002301)"

This seems a bit too coincidental to ignore. Goober, you want to take another look at that commit maybe?
2011-01-12 17:27   
OK the rabbit hole runs deep on this one. I couldn't understand why it wasn't happening in single so I made a simple test mission. Alpha and Beta wings with beta being 4 Uly by default. I changed them to 4 different ships and let them get slaughtered by a few dozen Maras. Beta respawed as all Uly instead of the ships they were assigned in loadout. That explains why no one hits it in single. Another bug.
2011-04-10 21:02   
I just looked at r6429 and I couldn't figure out any way for it to cause this error. The timing fits, but the code logic seems bulletproof. Changing a class while respawning should have exactly the same effect as changing a class in the team loadout.

Now it could be the case that FUBAR's latest note, 0012596, is the true cause of the bug, and that my commit simply opened a way for it to manifest. In that case we'd have to track down the original bug.

It sounds like someone would have to do some debugging during an active multi session. Either that, or run a build with some logging statements to print out hull and ship class values.
2011-06-21 17:57   
Okay, had another look at this. While trying to debug this with Zacam, I had an idea about how this came to be. One thing I noticed was that while playing NWTR, if I chose a Herc instead of the default Uly, my HP were knocked back to 72% upon respawn. Given that a Uly has exactly 72% of the hitpoints of a Herc, this could not be a coincidence.

So, I dug around a bit, and stumbled across something interesting. parse_object::ship_max_shield_strength and ship_max_hull_strength were defined as absolute values, and were never changed after the initial parsing. So, I wrote the attached patch, which changes both values to multipliers. This had the desired effect of getting correct health values again.

It should be noted that we didn't trigger the assertion mentioned in the bug report here, I hope we won't with the patch attached. Further testing required.
2011-06-25 04:04   
Committed to trunk in 7268. See discussion on internal.
2012-11-30 00:03   
Seems this issue has awaken from the dead. I ran into the shield version of it trying to test another issue in TBP. Tested that with modified retail data and it was reproducible. Tried with hull and that still exists as well.

Very easy to reproduce. Quick edit to ships.tbl to change a herc's hitpoints to a low value like 25 or shields to 20. Load up FRED, add a herc, go to ships editor, change ship class from herc to uly and watch the assert.

Cut and paste if you want a .tbm instead:

#Ship Classes
$Name: GTF Hercules
$Hitpoints: 25
;;$Shields: 20

2012-12-01 12:13   
Actually, it's a completely different issue that looks slightly similar, but whatever.
2012-12-01 12:32   
I've attached another patch that should address this.
2012-12-01 19:13   
So 2 separate bugs on the same 2 asserts.

Anyway patch seems to work just fine.
2012-12-02 00:03   
Looks fine code-wise too. Go ahead and commit/resolve.
2012-12-02 04:55   
FUBAR: The original issue was abo9ut something going wrong when parsing a mission file and adapting the parsed data to what the player(s) were actually using. This new crash, although it had the same assert, actually happened in a different part of the code (ship.cpp vs missionparse.cpp), for different reasons.
2013-01-16 01:17   
(Last edited: 2013-01-16 02:31)
Reopening due to my change plus FUBAR's report...

01:22:54 FUBAR: GOOBER!!!!! I just got a stanalone crash in change_ship_class!!!!!!!
01:25:01 FUBAR|away2__: r9503
01:25:51 FUBAR|away2__: got a 275/250 for a hull_pct of 1.1
01:26:14 FUBAR|away2__: line 8995
01:28:58 FUBAR|away2__: second ship is a loki
01:31:38 FUBAR|away2__: trainingmt-12.fs2 (you probably don't have that)
01:32:44 FUBAR|away2__: Received respawn request ASSERTION: "hull_pct > 0.0f && hull_pct <= 1.0f" at ship.cpp:8995 Int3(): From c:\fs2_open\code\globalincs\windebug.cpp at line 963
01:33:47 FUBAR|away2__: there were at least 2 ships destroyed in those few frames so not certain which one it was
01:35:55 FUBAR|away2__: default player starts are herc2's
01:42:12 FUBAR|away2__: crash was on alpha 1 but not sure if alpha 1 was hosting or client as that mission has alpha 2 as wing leader
01:47:23 FUBAR|away2__: looks like shields would have asserted to with a 410/400 if it had got that far
01:48:39 FUBAR|away2__: sorry 610/400 apparently I can't multiply by 4 correctly anymore

02:26:28 FUBAR: alpha 1
02:32:41 FUBAR: loki
02:54:58 FUBAR: reproducable on a standalone even with 1 player
02:55:20 FUBAR: m-01 just started with a uly instead of a herc2

2013-01-30 01:19   
Fix committed to trunk@9515.
2013-01-30 01:20   
(Last edited: 2013-01-30 01:52)
For some reason, client parse objects already have the correct ship class of the ship they're respawning into. That seems to be causing trouble with change_ship_type, although upon reflection it really shouldn't. Nevertheless, skipping the ship class change seems to fix it. (Strangely, server parse objects retain their parsed ship class as expected.)

EDIT: For posterity, the problem in change_ship_type is most likely due to the parse object's ship_class being assigned to the new class, while its ship_max_hull_strength (and ship_max_shield_strength, and who knows what else) stayed at the original, parsed values.

2013-01-30 01:20   
Assigning to The_E because he fixed two out of three bugs this ticket actually references.
2013-01-30 02:19   
This deserves another ticket. See 0002783.

Issue History
2010-10-14 16:17FUBAR-BDHRNew Issue
2010-10-14 16:17FUBAR-BDHRFile Added: hull_over_100.txt
2010-10-14 16:17FUBAR-BDHRFile Added: hull_over_100.rar
2010-11-11 03:41The_ENote Added: 0012447
2010-11-11 03:41The_EStatusnew => resolved
2010-11-11 03:41The_EFixed in Version => 3.6.13
2010-11-11 03:41The_EResolutionopen => fixed
2010-11-11 03:41The_EAssigned To => The_E
2010-11-11 04:51The_ENote Added: 0012448
2010-11-11 04:51The_EStatusresolved => feedback
2010-11-11 04:51The_EResolutionfixed => reopened
2010-11-11 04:51The_EStatusfeedback => assigned
2010-11-13 21:20Goober5000Note Added: 0012461
2010-11-13 22:56The_ENote Added: 0012462
2010-11-13 22:56FUBAR-BDHRNote Added: 0012463
2010-12-03 18:51FUBAR-BDHRNote Added: 0012499
2011-01-12 17:03chief1983Note Added: 0012595
2011-01-12 17:27FUBAR-BDHRNote Added: 0012596
2011-01-12 17:31FUBAR-BDHRRelationship addedrelated to 0002378
2011-03-31 13:26chief1983Fixed in Version3.6.13 =>
2011-03-31 13:26chief1983Target Version => 3.6.14
2011-04-10 21:02Goober5000Note Added: 0012655
2011-05-25 12:54chief1983Prioritynormal => urgent
2011-06-21 17:47The_EFile Added: 2326.patch
2011-06-21 17:57The_ENote Added: 0012721
2011-06-25 04:04The_ENote Added: 0012726
2011-06-25 04:04The_EStatusassigned => resolved
2011-06-25 04:04The_EFixed in Version => 3.6.13
2011-06-25 04:04The_EResolutionreopened => fixed
2011-09-14 05:29karajormaStatusresolved => assigned
2011-09-14 05:29karajormaAssigned ToThe_E => karajorma
2011-09-14 07:24The_EAssigned Tokarajorma => The_E
2011-09-14 07:24The_EStatusassigned => resolved
2012-11-30 00:03FUBAR-BDHRNote Added: 0014233
2012-11-30 00:03FUBAR-BDHRStatusresolved => feedback
2012-11-30 00:03FUBAR-BDHRResolutionfixed => reopened
2012-12-01 12:13The_ENote Added: 0014242
2012-12-01 12:31The_EStatusfeedback => assigned
2012-12-01 12:32The_ENote Added: 0014243
2012-12-01 12:32The_EStatusassigned => code review
2012-12-01 12:32The_EFile Added: 2326-2.patch
2012-12-01 19:13FUBAR-BDHRNote Added: 0014244
2012-12-02 00:03Goober5000Note Added: 0014248
2012-12-02 04:55The_ENote Added: 0014257
2012-12-02 04:55The_EStatuscode review => resolved
2012-12-02 04:55The_EResolutionreopened => fixed
2013-01-13 03:51Goober5000Changeset attached => fs2open trunk r9502
2013-01-13 04:00Goober5000Changeset attached => fs2open trunk r7268
2013-01-16 01:17Goober5000Note Added: 0014651
2013-01-16 01:17Goober5000Assigned ToThe_E => Goober5000
2013-01-16 01:17Goober5000Statusresolved => assigned
2013-01-16 01:17Goober5000Resolutionfixed => reopened
2013-01-16 01:30FUBAR-BDHRFile Added: 2326_new.rar
2013-01-16 01:58Goober5000Note Edited: 0014651bug_revision_view_page.php?bugnote_id=14651#r376
2013-01-16 02:31Goober5000Note Edited: 0014651bug_revision_view_page.php?bugnote_id=14651#r377
2013-01-30 01:19Goober5000Changeset attached => fs2open trunk r9515
2013-01-30 01:19Goober5000Note Added: 0014668
2013-01-30 01:19Goober5000Statusassigned => resolved
2013-01-30 01:20Goober5000Note Added: 0014669
2013-01-30 01:20Goober5000Note Added: 0014670
2013-01-30 01:20Goober5000Assigned ToGoober5000 => The_E
2013-01-30 01:47Goober5000Note Edited: 0014669bug_revision_view_page.php?bugnote_id=14669#r379
2013-01-30 01:52Goober5000Note Edited: 0014669bug_revision_view_page.php?bugnote_id=14669#r380
2013-01-30 02:19Goober5000Note Added: 0014671
2013-01-30 02:19Goober5000Relationship addedrelated to 0002783