View Issue Details

IDProjectCategoryView StatusLast Update
0001098FSSCPmultiplayerpublic2008-10-23 04:46
ReporterFUBAR-BDHR Assigned Totaylor  
PriorityhighSeveritycrashReproducibilityrandom
Status resolvedResolutionfixed 
Fixed in Version3.6.10 
Summary0001098: Assert: (Weapon_info[Weapons[homing_object->instance].weapon_info_index].wi_flags & WIF_BOMB) || (Weapon_info[Weapons[homing_obj
DescriptionWell that was cut off. Full message is: Assert: (Weapon_info[Weapons[homing_object->instance].weapon_info_index].wi_flags & WIF_BOMB) || (Weapon_info[Weapons[homing_object->instance].weapon_info_index].wi_flags & WIF_CMEASURE)
Second time I've got this error. Might be related to error report 773 that is marked fixed. Received while client during a RI
TagsNo tags attached.

Activities

2006-10-09 02:04

 

Media-10-08-06b.rar (26,996 bytes)

2006-10-09 02:19

 

Media-10-08-06c.rar (26,382 bytes)

FUBAR-BDHR

2006-10-09 02:20

developer   ~0006832

Second time tonight this time in ravana rescue.

2006-10-09 02:37

 

Media-10-08-06d.rar (29,140 bytes)

2006-10-13 22:29

 

Media-10-13-06a.rar (45,518 bytes)

karajorma

2007-09-29 17:47

administrator   ~0008547

I've just seen this one while playing Rebels & Renegades in multiplayer. At first glance it seems to me like someone marked bombs and countermeasures as homing weapons but missed out that missiles can home too.

Right?

FUBAR-BDHR

2007-10-01 07:11

developer   ~0008549

It's been so long since I've played FS2 or any game for that matter I can't remember what the circumstances behind this were.

karajorma

2007-10-04 21:39

administrator   ~0008554

weapon_home() seems to have a similar assertion

Assert(Weapon_info[Weapons[hobjp->instance].weapon_info_index].wi_flags & WIF_BOMB);

karajorma

2007-12-03 01:52

administrator   ~0008722

Hmmmmm. Strange. If I add that assertion to the packet building function it won't assert. Looks like the packet is leaving the server fine but is being built up again incorrectly.

I'll have to look into this further.

FUBAR-BDHR

2008-07-01 01:13

developer   ~0009428

Was doing some testing with the Scoring-Changes build and received this a couple of times. Saved the logs from both the server and client side.

2008-07-01 01:14

 

Core2Quad-06-30-08a.rar (20,866 bytes)

2008-07-01 01:14

 

Media-06-30-08a.rar (13,625 bytes)

2008-09-12 19:07

 

fs2_open.log (66,102 bytes)

FUBAR-BDHR

2008-09-12 19:08

developer   ~0009662

Uploaded a new log file from Kara's TeamChatBuild.7z. Looks like some more debug stuff was added so I hope this helps. Was hosting on a standalone. Standalone did not crash.

chief1983

2008-10-09 16:40

administrator   ~0009896

Hopefully this can be sorted out when Kara or someone can take a look at it, or else bump it back down in priority.

karajorma

2008-10-09 19:44

administrator   ~0009905

This one is causing me real problems because the packet seems to be leaving the server fine but by the time it reaches the client the net_signature is invalid.

I will try to take another look at it soon.

taylor

2008-10-18 18:56

administrator   ~0010035

The net_signature isn't really invalid, it is just assigned to a different object. Every time this Assert() gets hit it's when it's trying to home in on a cmeasure. This really goes back to the fact that counter measures were never valid network objects in the first place. When they were converted to be weapons they started getting valid signatures, but that opens it up to all sorts of problems.

I haven't confirmed it yet, but I think it is due specifically to lifetime of the cmeasure being so short. You can end up with the weapon net signatures getting out of sync. I'm going to attempt to test that out later tonight, but it's going to be difficult to say whether it will be 100% fixed or not. The fact that the cmeasures don't always cause this indicates that it's not lifetime that's the problem, but there is still some situation that causes the signatures to get out of sync, so we just have to start ruling things out.

The Assert() just needs to be followed up with a check to make sure that the game doesn't further pass an invalid object through. This is to prevent badness in weapon_home() (as noted by the Assert() it tends to hit there).

FUBAR-BDHR

2008-10-18 19:25

developer   ~0010038

I remember back in retail there was a bug with dual fired Cycs and Helios. If one of the pair was shot down both would disappear from the client side. Been meaning to see if this still exists. Anyway would something like that cause this? Basically a bomb that still exists on the host side but the client doesn't register it as there anymore? He may be able to see it (can't remember the details) but can't shoot it or anything. I did noticed the missions where this happened the most would tend to have a lot of bombs to shoot down. Of course they have a lot of everything else too.

Again I don't even know if that bug still exists in FS2_Open. Just that WIF_BOMB makes me think of it every time I see it.

taylor

2008-10-18 21:33

administrator   ~0010039

It shouldn't be related. This bug is caused specifically by the new cmeasure code.

I gave the lifetime thing a check, and ruled it out. But I did figure out that indeed the signatures get out of sync, and it appears to be specifically because of the cmeasures. It appears that when a weapon is typically triggered through the network, the server adds the signature to the packet, then the clients will sync that signature before creating the weapon on their end. This makes sure that the weapon should have the same signature on the client that the host has for it. The old cmeasure code didn't give valid signatures to their objects since there wasn't any need to ever send it over the network (there was just a fired/fire packet, very basic). The new code treats it as a real weapon however, which means that it needs to keep track of the signatures now.

The only way that I see if fix this properly is to bump the server/client version for multi and fix the packet to send the signature to sync with. The alternative fix is to create a new packet for cmeasures, but this will still ultimately break older clients since it still won't sync weapon signatures properly (as both client and host must have this fix in order for it to work). The alternative really isn't a fix in other words, it just addresses the cause while ignoring the result.

Do you see another way to deal with this karajorma?

taylor

2008-10-18 23:40

administrator   ~0010041

I think that I might have figured out a way to fix this without breaking anything, without needing a version bump, and without requiring a new packet. You would still need updated builds for client and host in order for the fix to work, but the fix wouldn't break older builds, and older builds wouldn't hurt newer builds.

It might be tomorrow before I can actually code it up and test it out, but if it works then I think that it should effectively solve our problem. :)

taylor

2008-10-19 03:45

administrator   ~0010042

Ok, my initial tests show that it is fixed, but not 100%. The cmeasure signatures do stay in sync across host and client, but there are rare instances of duplicate signatures which can cause the same Assert() due to it getting the wrong object. This duplicate signature issue seems to be due to the fact that primary weapons also don't have signature syncing. Secondary weapons do sync however, and now that cmeasures do as well, the various objects are kept in sync overall if not entirely.

I believe that the duplicate signature problem was there in retail as well, but since secondary weapons don't typically target other weapons, it is something that rarely becomes an issue. So, I guess we should just accept it as-is for now and maybe have another look at fixing the duplicate sig problem when we really decided to break the netcode compatibility. For now I think that we should just go with this fix, remove the Assert(), and handle the case with an early return instead for the rare instances that still require it.

These changes will be in my next multi_test build, so if the testers sign off on it, and no devs disagree, then I'll commit at some point next week.

karajorma

2008-10-19 07:50

administrator   ~0010045

If you can post a diff of the change then I can take a look to see if I can see another way past the problem.

2008-10-19 14:46

 

1098.diff (5,183 bytes)   
Index: code/network/multimsgs.cpp
===================================================================
--- code/network/multimsgs.cpp	(revision 4893)
+++ code/network/multimsgs.cpp	(working copy)
@@ -7774,6 +7786,9 @@
 	if ( !(Weapon_info[wp->weapon_info_index].wi_flags & WIF_HOMING) )
 		return;
 
+	// default the subsystem
+	t_subsys = -1;
+
 	// get the homing signature.  If this weapon isn't homing on anything, then sent 0 as the
 	// homing signature.
 	homing_signature = 0;
@@ -7782,7 +7797,6 @@
 		homing_signature = homing_object->net_signature;
 
 		// get the subsystem index.
-		t_subsys = -1;
 		if ( (homing_object->type == OBJ_SHIP) && (wp->homing_subsys != NULL) ) {
 			int s_index;
 
@@ -7835,7 +7849,14 @@
 	}
 
 	if ( homing_object->type == OBJ_WEAPON ) {
-		Assert((Weapon_info[Weapons[homing_object->instance].weapon_info_index].wi_flags & WIF_BOMB) || (Weapon_info[Weapons[homing_object->instance].weapon_info_index].wi_flags & WIF_CMEASURE));
+		int flags = Weapon_info[Weapons[homing_object->instance].weapon_info_index].wi_flags;
+
+	//	Assert( (flags & WIF_BOMB) || (flags & WIF_CMEASURE) );
+
+		if ( !((flags & WIF_BOMB) || (flags & WIF_CMEASURE)) ) {
+			nprintf(("Network", "Homing object is invalid for homing update\n"));
+			return;
+		}
 	}
 
 	wp->homing_object = homing_object;
@@ -8219,7 +8240,11 @@
 	if((Player_obj != NULL) && (Player_obj == objp)){		
 		return;
 	}
-		
+
+	if ( (rand_val >= NPERM_SIG_MIN) && (rand_val <= NPERM_SIG_MAX) ) {
+		multi_set_network_signature((ushort)rand_val, MULTI_SIG_NON_PERMANENT);
+	}
+
 	// make it so ship can fire right away!
 	Ships[objp->instance].cmeasure_fire_stamp = timestamp(0);
 	if ( objp == Player_obj ){		
Index: code/network/multiutil.cpp
===================================================================
--- code/network/multiutil.cpp	(revision 4893)
+++ code/network/multiutil.cpp	(working copy)
@@ -501,7 +501,7 @@
 	// as are debris and asteroids since they don't die very often.  It would be vary rare for this
 	// value (the permanent signature) to wrap.  For now, this condition is an error condition
 	if ( what_kind == MULTI_SIG_SHIP ) {
-		if ( Next_ship_signature == 0 ){
+		if ( Next_ship_signature < SHIP_SIG_MIN ){
 			Next_ship_signature = SHIP_SIG_MIN;
 		}
 
@@ -514,7 +514,7 @@
 
 		// signature stuff for asteroids.
 	} else if ( what_kind == MULTI_SIG_ASTEROID ) {
-		if ( Next_asteroid_signature == 0 ){
+		if ( Next_asteroid_signature < ASTEROID_SIG_MIN ){
 			Next_asteroid_signature = ASTEROID_SIG_MIN;
 		}
 
@@ -526,7 +526,7 @@
 
 		// signatures for debris
 	} else if ( what_kind == MULTI_SIG_DEBRIS ) {
-		if ( Next_debris_signature == 0 ){
+		if ( Next_debris_signature < DEBRIS_SIG_MIN ){
 			Next_debris_signature = DEBRIS_SIG_MIN;
 		}
 
@@ -538,12 +538,12 @@
 
 		// signature stuff for weapons and other expendable things.
 	} else if ( what_kind == MULTI_SIG_NON_PERMANENT ) {
-		if ( Next_non_perm_signature == 0 ){
+		if ( Next_non_perm_signature < NPERM_SIG_MIN ){
 			Next_non_perm_signature = NPERM_SIG_MIN;
 		}
 
 		sig = Next_non_perm_signature++;
-		if ( Next_non_perm_signature == NPERM_SIG_MAX ){
+		if ( (Next_non_perm_signature < NPERM_SIG_MIN) || (Next_non_perm_signature == NPERM_SIG_MAX) ) {
 			Next_non_perm_signature = NPERM_SIG_MIN;
 		}
 	} else {
@@ -559,22 +559,22 @@
 ushort multi_get_next_network_signature( int what_kind )
 {
 	if ( what_kind == MULTI_SIG_SHIP ) {
-		if ( Next_ship_signature == 0 )
+		if ( Next_ship_signature < SHIP_SIG_MIN )
 			Next_ship_signature = SHIP_SIG_MIN;
 		return Next_ship_signature;
 
 	} else if ( what_kind == MULTI_SIG_DEBRIS ) {
-		if ( Next_debris_signature == 0 )
+		if ( Next_debris_signature < DEBRIS_SIG_MIN)
 			Next_debris_signature = DEBRIS_SIG_MIN;
 		return Next_debris_signature;
 
 	} else if ( what_kind == MULTI_SIG_ASTEROID ) {
-		if ( Next_asteroid_signature == 0 )
+		if ( Next_asteroid_signature < ASTEROID_SIG_MIN )
 			Next_asteroid_signature = ASTEROID_SIG_MIN;
 		return Next_asteroid_signature;
 
 	} else if ( what_kind == MULTI_SIG_NON_PERMANENT ) {
-		if ( Next_non_perm_signature == 0 )
+		if ( Next_non_perm_signature < NPERM_SIG_MIN )
 			Next_non_perm_signature = NPERM_SIG_MIN;
 		return Next_non_perm_signature;
 
@@ -600,7 +600,7 @@
 		Assert( (signature >= ASTEROID_SIG_MIN) && (signature <= ASTEROID_SIG_MAX) );
 		Next_asteroid_signature = signature;
 	} else if ( what_kind == MULTI_SIG_NON_PERMANENT ) {
-		Assert( (signature >= NPERM_SIG_MIN) && (signature <= NPERM_SIG_MAX) );
+		Assert( (signature >= NPERM_SIG_MIN) /*&& (signature <= NPERM_SIG_MAX)*/ );
 		Next_non_perm_signature = signature;
 	} else
 		Int3();			// get Allender
Index: code/ship/ship.cpp
===================================================================
--- code/ship/ship.cpp	(revision 4893)
+++ code/ship/ship.cpp	(working copy)
@@ -10043,7 +10042,7 @@
 		// the new way of doing things
 		// if(Netgame.debug_flags & NETD_FLAG_CLIENT_FIRING){
 		if(Game_mode & GM_MULTIPLAYER){
-			send_NEW_countermeasure_fired_packet( objp, cmeasure_count, arand );
+			send_NEW_countermeasure_fired_packet( objp, cmeasure_count, /*arand*/Objects[cobjnum].net_signature );
 		}
 	}
 
1098.diff (5,183 bytes)   

taylor

2008-10-19 14:53

administrator   ~0010047

Diff attached.

The fix is to use the rand_val int that it sends over with the cmeasure packet to store the net_signature. The actual value of rand_val doesn't matter since it's use as a random value for velocity and lifetime values that can be identical between client and host. The value is just an objnum, which means that it is always lower than MAX_OBJECTS. This allows us to get away with setting it to a larger number (the signature, which is a minimum of 32768) to indicate the fix to new clients, and older clients don't really care what the value is so long as it's >= -1.

The changes to multiutil.cpp are just to fix some bad checks that V made for the signature values and isn't directly related to the fix for this bug.

karajorma

2008-10-20 19:18

administrator   ~0010067

Looks good to me.

taylor

2008-10-23 04:46

administrator   ~0010100

Fixered.

Issue History

Date Modified Username Field Change
2006-10-09 02:04 FUBAR-BDHR New Issue
2006-10-09 02:04 FUBAR-BDHR File Added: Media-10-08-06b.rar
2006-10-09 02:19 FUBAR-BDHR File Added: Media-10-08-06c.rar
2006-10-09 02:20 FUBAR-BDHR Note Added: 0006832
2006-10-09 02:37 FUBAR-BDHR File Added: Media-10-08-06d.rar
2006-10-13 22:29 FUBAR-BDHR File Added: Media-10-13-06a.rar
2007-09-29 17:47 karajorma Note Added: 0008547
2007-10-01 07:11 FUBAR-BDHR Note Added: 0008549
2007-10-04 21:39 karajorma Note Added: 0008554
2007-12-03 01:48 karajorma Assigned To taylor => karajorma
2007-12-03 01:52 karajorma Note Added: 0008722
2008-07-01 01:13 FUBAR-BDHR Note Added: 0009428
2008-07-01 01:14 FUBAR-BDHR File Added: Core2Quad-06-30-08a.rar
2008-07-01 01:14 FUBAR-BDHR File Added: Media-06-30-08a.rar
2008-09-12 19:07 FUBAR-BDHR File Added: fs2_open.log
2008-09-12 19:08 FUBAR-BDHR Note Added: 0009662
2008-10-09 16:40 chief1983 Note Added: 0009896
2008-10-09 16:40 chief1983 Priority normal => high
2008-10-09 19:44 karajorma Note Added: 0009905
2008-10-18 18:56 taylor Note Added: 0010035
2008-10-18 19:25 FUBAR-BDHR Note Added: 0010038
2008-10-18 21:33 taylor Note Added: 0010039
2008-10-18 23:40 taylor Note Added: 0010041
2008-10-19 03:45 taylor Note Added: 0010042
2008-10-19 07:50 karajorma Note Added: 0010045
2008-10-19 14:46 taylor File Added: 1098.diff
2008-10-19 14:53 taylor Note Added: 0010047
2008-10-20 07:40 taylor Assigned To karajorma => taylor
2008-10-20 19:18 karajorma Note Added: 0010067
2008-10-23 04:46 taylor Status assigned => resolved
2008-10-23 04:46 taylor Fixed in Version => 3.6.10
2008-10-23 04:46 taylor Resolution open => fixed
2008-10-23 04:46 taylor Note Added: 0010100