View Issue Details

IDProjectCategoryView StatusLast Update
0002632FSSCPsoundpublic2012-05-02 11:37
Reporterm_m Assigned Tom_m  
PrioritynormalSeverityminorReproducibilityN/A
Status resolvedResolutionfixed 
Product Version3.6.13 
Target Version3.6.13Fixed in Version3.6.14 RC5 
Summary0002632: Sounds assigned to objects get dropped when limit hardcoded gets reached
DescriptionWhile investigating the cause of a bug in Diaspora where a sound didn't get played when it should I noticed that the cause of this problem was that there was no free object sound slot for that sound. I converted the fixed size objsnd_num array in the object struct to a vector which seems to work. There are some things where I'm not sure if they are done the way they're supposed to, so please take a look at the attached patch.
TagsNo tags attached.

Activities

CommanderDJ

2012-03-31 14:00

developer   ~0013428

Code looks good to me, if that helps at all. One question though: on line 194 of the patch, is there a particular reason you checked the size against 32? Is there a way you could do it without using magic numbers in the code?

m_m

2012-03-31 15:08

developer   ~0013429

Ah, I missed to add a constant for that. New patch is attached.

Echelon9

2012-04-01 12:53

developer   ~0013431

Rather than using the template:

for(idx=0; idx<objp->objsnd_num.size(); idx++)...

Can you write it using STL iterators?

m_m

2012-04-01 13:36

developer   ~0013433

I didn't bother with changing that behavior as performance seems to be approximately the same but it's probably the cleaner and the intended way. New patch uploaded which uses iterators where appropriate.

Echelon9

2012-04-01 14:25

developer   ~0013434

Looking good - a few final changes, then it should be good to commit:

(1) pre vs post increment iterators

Within the fs2open codebase, we've settled on ++i incrementers, so:

for(SCP_vector<int>::iterator iter = objp->objsnd_num.begin(); iter != objp->objsnd_num.end(); ++iter){

rather than...

for(SCP_vector<int>::iterator iter = objp->objsnd_num.begin(); iter != objp->objsnd_num.end(); iter++){

see further: http://stackoverflow.com/questions/24901/is-there-a-performance-difference-between-i-and-i-in-c

(2) minimizing the size of the vector

I can kind of see what you are trying to achieve here, but not sure it is the cleanest way possible. Did you encounter big increases in memory without this code, thus requiring it?

(3) call to obj_snd_delete()

The second parameter to obj_snd_delete() should be an index into the container, not the value at the index into the container from the documentation. Looks like you are passing the value at the index.

m_m

2012-04-01 15:36

developer   ~0013436

(1) Done, I never really knew that there was a performance difference between those two.

(2) No but having multiple entries in the vector containing only unused values wasn't what I wanted so I tried to keep the size of the vector as low as possible without having to change already used indexes.

(3) I'm not sure what you mean here as I didn't change any code involving the second parameter apart from the sanity checking.

Echelon9

2012-04-02 13:48

developer   ~0013437

(3) call to obj_snd_delete()

Phrased a different way, if your adoption of '*iter' replaces 'objp->objsnd_num[idx]' throughout the code, then '*iter' cannot also be the correct replacement for 'idx' in the line 'obj_snd_delete(objnum, idx);'

m_m

2012-04-02 14:15

developer   ~0013438

Now I understand what you mean. Of course that's a mistake on my side, patch changed accordingly.

Echelon9

2012-04-02 14:44

developer   ~0013439

Thanks for persisting - there's still something odd going on with that pseudo-clearing minimise function.

Error(LOCATION, "Object sound index %d is bigger than the actual size %d!", index, (int) objp->objsnd_num.size());

Was easily triggered in the first mission of mediavps_3612, after firing a few missiles and primaries.

Why not just wait until mission end, and add a objp->objsnd_num.clear()?

m_m

2012-04-02 15:25

developer   ~0013440

I never ran into issues with that but I guess you're right. I removed the shrinking code and added a objsnd_num.clear() to the obj_snd_delete_all() function which gets called at the end of the mission.

m_m

2012-04-02 16:59

developer  

objsnd_dynamic.patch (8,171 bytes)   
Index: code/object/object.cpp
===================================================================
--- code/object/object.cpp	(revision 8648)
+++ code/object/object.cpp	(working copy)
@@ -288,7 +288,7 @@
 //sets up the free list & init player & whatever else
 void obj_init()
 {
-	int i, idx;
+	int i;
 	object *objp;
 	
 	Object_inited = 1;
@@ -305,11 +305,6 @@
 		objp->type = OBJ_NONE;
 		objp->signature = i + 100;
 		objp->collision_group_id = 0;
-
-		// zero all object sounds
-		for(idx=0; idx<MAX_OBJECT_SOUNDS; idx++){
-			objp->objsnd_num[idx] = -1;
-		}
 		
 		list_append(&obj_free_list, objp);
 		objp++;
@@ -417,7 +412,7 @@
 int obj_create(ubyte type,int parent_obj,int instance, matrix * orient, 
                vec3d * pos, float radius, uint flags )
 {
-	int objnum,idx;
+	int objnum;
 	object *obj;
 
 	// Find next free object
@@ -460,9 +455,6 @@
 	obj->flags &= ~OF_INVULNERABLE;		//	Make vulnerable.
 	physics_init( &obj->phys_info );
 
-	for(idx=0; idx<MAX_OBJECT_SOUNDS; idx++){
-		obj->objsnd_num[idx] = -1;
-	}
 	obj->num_pairs = 0;
 	obj->net_signature = 0;			// be sure to reset this value so new objects don't take on old signatures.	
 
Index: code/object/object.h
===================================================================
--- code/object/object.h	(revision 8648)
+++ code/object/object.h	(working copy)
@@ -126,11 +126,6 @@
 #define OF_TEMP_MARKED		(1<<30)	// Temporarily marked (Fred).
 #define OF_HIDDEN			(1<<31)	// Object is hidden (not shown) and can't be manipulated
 
-
-// max # of object sounds per object
-//WMC - bumped this to 32 :D
-#define MAX_OBJECT_SOUNDS	32
-
 struct dock_instance;
 
 typedef struct object {
@@ -151,7 +146,7 @@
 	float				shield_quadrant[MAX_SHIELD_SECTIONS];	//	Shield is broken into components.  Quadrants on 4/24/97.
 	float				hull_strength;	//	Remaining hull strength.
 	float				sim_hull_strength;	// Simulated hull strength - used with training weapons.
-	short				objsnd_num[MAX_OBJECT_SOUNDS];		// Index of persistant sound struct.  -1 if no persistant sound assigned.
+	SCP_vector<int> objsnd_num;		// Index of persistant sound struct.
 	ushort			net_signature;
 	int				num_pairs;		// How many object pairs this is associated with.  When 0 then there are no more.
 
Index: code/object/objectsnd.cpp
===================================================================
--- code/object/objectsnd.cpp	(revision 8648)
+++ code/object/objectsnd.cpp	(working copy)
@@ -220,23 +220,22 @@
 void obj_snd_stop(object *objp, int index)
 {
 	obj_snd	*osp;
-	int idx;
 
 	// sanity
-	if(index >= MAX_OBJECT_SOUNDS){
-		Int3();
+	if(index >= (int) objp->objsnd_num.size()){
+		Error(LOCATION, "Object sound index %d is bigger than the actual size %d!", index, (int) objp->objsnd_num.size());
 		return;
 	}
 
 	// if index is -1, kill all sounds for this guy
 	if(index == -1){
 		// kill all sounds for this guy
-		for(idx=0; idx<MAX_OBJECT_SOUNDS; idx++){
-			if ( objp->objsnd_num[idx] == -1 ){
+		for(SCP_vector<int>::iterator iter = objp->objsnd_num.begin(); iter != objp->objsnd_num.end(); ++iter){
+			if ( *iter == -1 ){
 				continue;
 			}
 
-			osp = &Objsnds[objp->objsnd_num[idx]];
+			osp = &Objsnds[*iter];
 
 			if ( osp->instance != -1 ) {
 				snd_stop(osp->instance);
@@ -341,7 +340,6 @@
 	obj_snd			*lowest_vol_osp = NULL;
 	float				lowest_vol;
 	int obj_snd_index = -1;
-	int idx;
 	
 	lowest_vol = 1000.0f;
 	for ( osp = GET_FIRST(&obj_snd_list); osp !=END_OF_LIST(&obj_snd_list); osp = GET_NEXT(osp) ) {
@@ -358,15 +356,16 @@
 	objp = &Objects[lowest_vol_osp->objnum];
 
 	if ( (lowest_vol < new_vol) && (objp != NULL) ) {
+		int idx = 0;
 		// determine what index in this guy the sound is
-		for(idx=0; idx<MAX_OBJECT_SOUNDS; idx++){
-			if(objp->objsnd_num[idx] == (lowest_vol_osp - Objsnds)){
+		for(SCP_vector<int>::iterator iter = objp->objsnd_num.begin(); iter != objp->objsnd_num.end(); ++iter, ++idx){
+			if(*iter == (lowest_vol_osp - Objsnds)){
 				obj_snd_index = idx;
 				break;
 			}
 		}
 
-		if((obj_snd_index == -1) || (obj_snd_index >= MAX_OBJECT_SOUNDS)){
+		if((obj_snd_index == -1) || (obj_snd_index >= (int) objp->objsnd_num.size())){
 			Int3();		// get dave
 		} else {
 			obj_snd_stop(objp, obj_snd_index);
@@ -624,11 +623,11 @@
 		else {
 			if ( distance > Snds[osp->id].max ) {
 				int sound_index = -1;
-				int idx;
+				int idx = 0;
 
 				// determine which sound index it is for this guy
-				for(idx=0; idx<MAX_OBJECT_SOUNDS; idx++){
-					if(objp->objsnd_num[idx] == (osp - Objsnds)){
+				for(SCP_vector<int>::iterator iter = objp->objsnd_num.begin(); iter != objp->objsnd_num.end(); ++iter, ++idx){
+					if(*iter == (osp - Objsnds)){
 						sound_index = idx;
 						break;
 					}
@@ -706,23 +705,25 @@
 
 	obj_snd	*snd = NULL;
 	object	*objp = &Objects[objnum];
-	int idx, sound_index;
+	int sound_index;
+	int idx = 0;
 
 	// try and find a valid objsound index
 	sound_index = -1;
-	for(idx=0; idx<MAX_OBJECT_SOUNDS; idx++){
-		if(objp->objsnd_num[idx] == -1){
+	for(SCP_vector<int>::iterator iter = objp->objsnd_num.begin(); iter != objp->objsnd_num.end(); ++iter, ++idx){
+		if(*iter == -1){
 			sound_index = idx;
 			break;
 		}
 	}
 	
-	// no sound. doh!
+	// no sound slot free, make a new one!
 	if ( sound_index == -1 ){
-		return -1;
+		sound_index = (int) objp->objsnd_num.size();
+		objp->objsnd_num.push_back(-1);
 	}
 
-	objp->objsnd_num[sound_index] = (short)obj_snd_get_slot();
+	objp->objsnd_num[sound_index] = obj_snd_get_slot();
 	if ( objp->objsnd_num[sound_index] == -1 ) {
 		nprintf(("Sound", "SOUND ==> No free object-linked sounds left\n"));
 		return -1;
@@ -767,16 +768,13 @@
 //
 void obj_snd_delete(int objnum, int index)
 {
-	if(objnum < 0 || objnum >= MAX_OBJECTS)
-		return;
-	if(index < 0 || index >= MAX_OBJECT_SOUNDS)
-		return;
-
 	//Sanity checking
 	Assert(objnum > -1 && objnum < MAX_OBJECTS);
-	Assert(index > -1 && index < MAX_OBJECT_SOUNDS);
 
 	object *objp = &Objects[objnum];
+	
+	Assert(index > -1 && index < (int) objp->objsnd_num.size());
+
 	obj_snd *osp = &Objsnds[objp->objsnd_num[index]];
 
 	//Stop the sound
@@ -804,21 +802,21 @@
 {
 	object	*objp;
 	obj_snd	*osp;
-	int idx;
 
 	if(objnum < 0 || objnum >= MAX_OBJECTS)
 		return;
 
 	objp = &Objects[objnum];
 
+	size_t idx = 0;
 	//Go through the list and get sounds that match criteria
-	for(idx=0; idx<MAX_OBJECT_SOUNDS; idx++){
+	for(SCP_vector<int>::iterator iter = objp->objsnd_num.begin(); iter != objp->objsnd_num.end(); ++iter, ++idx){
 		// no sound
-		if ( objp->objsnd_num[idx] == -1 ){
+		if ( *iter == -1 ){
 			continue;
 		}
 
-		osp = &Objsnds[objp->objsnd_num[idx]];
+		osp = &Objsnds[*iter];
 
 		// if we're just deleting a specific sound type
 		// and this is not one of them. skip it.
@@ -839,24 +837,12 @@
 //
 void obj_snd_delete_all()
 {
-	/*
-	obj_snd	*osp, *temp;	
-	
-	osp = GET_FIRST(&obj_snd_list);	
-	while( (osp != NULL) && (osp !=END_OF_LIST(&obj_snd_list)) )	{
-		temp = GET_NEXT(osp);
-		Assert( osp->objnum != -1 );
-
-		obj_snd_delete_type( osp->objnum );
-
-		osp = temp;
-	}
-	*/
-
 	int idx;
 	for(idx=0; idx<MAX_OBJ_SNDS; idx++){
 		if(Objsnds[idx].flags & OS_USED){
 			obj_snd_delete_type(Objsnds[idx].objnum);
+
+			Objects[Objsnds[idx].objnum].objsnd_num.clear();
 		}
 	}
 }
Index: code/ship/ship.cpp
===================================================================
--- code/ship/ship.cpp	(revision 8648)
+++ code/ship/ship.cpp	(working copy)
@@ -13016,7 +13016,8 @@
 void ship_assign_sound_all()
 {
 	object *objp;
-	int idx, has_sounds;
+	size_t idx;
+	int has_sounds;
 
 	if ( !Sound_enabled )
 		return;
@@ -13026,7 +13027,7 @@
 			has_sounds = 0;
 
 			// check to make sure this guy hasn't got sounds already assigned to him
-			for(idx=0; idx<MAX_OBJECT_SOUNDS; idx++){
+			for(idx=0; idx<objp->objsnd_num.size(); idx++){
 				if(objp->objsnd_num[idx] != -1){
 					// skip
 					has_sounds = 1;
objsnd_dynamic.patch (8,171 bytes)   

m_m

2012-04-02 17:00

developer   ~0013441

The problem was again a wrong idx --> *iter conversion. I changed all occurrences of the same mistake and the error doesn't show up anymore.

Echelon9

2012-04-02 21:31

developer   ~0013442

Looks good. Committing to trunk now.

Echelon9

2012-04-02 21:31

developer   ~0013443

Fix committed to trunk@8649.

niffiwan

2012-05-02 11:37

developer   ~0013507

Fix committed to fs2_open_3_6_14@8733.

Related Changesets

fs2open: trunk r8649

2012-04-02 17:32

Echelon9


Ported: N/A

Details Diff
Fix Mantis 2632: Sounds assigned to objects get dropped when limit hardcoded gets reached. Patch from m_m Affected Issues
0002632
mod - /trunk/fs2_open/code/ship/ship.cpp Diff File
mod - /trunk/fs2_open/code/object/objectsnd.cpp Diff File
mod - /trunk/fs2_open/code/object/object.h Diff File
mod - /trunk/fs2_open/code/object/object.cpp Diff File

fs2open: fs2_open_3_6_14 r8733

2012-05-02 07:38

niffiwan


Ported: N/A

Details Diff
Backport: Trunk r8649; Fix Mantis 2632: Sounds assigned to objects get dropped when limit hardcoded gets reached. Patch from m_m Affected Issues
0002632
mod - /branches/fs2_open_3_6_14/code/ship/ship.cpp Diff File
mod - /branches/fs2_open_3_6_14/code/object/objectsnd.cpp Diff File
mod - /branches/fs2_open_3_6_14/code/object/object.h Diff File
mod - /branches/fs2_open_3_6_14/code/object/object.cpp Diff File

Issue History

Date Modified Username Field Change
2012-03-26 08:54 m_m New Issue
2012-03-26 08:54 m_m Status new => assigned
2012-03-26 08:54 m_m Assigned To => m_m
2012-03-26 08:54 m_m File Added: objsnd_dynamic.patch
2012-03-26 08:54 m_m Status assigned => code review
2012-03-31 14:00 CommanderDJ Note Added: 0013428
2012-03-31 15:07 m_m File Deleted: objsnd_dynamic.patch
2012-03-31 15:08 m_m File Added: objsnd_dynamic.patch
2012-03-31 15:08 m_m Note Added: 0013429
2012-04-01 12:53 Echelon9 Note Added: 0013431
2012-04-01 13:36 m_m Note Added: 0013433
2012-04-01 13:36 m_m File Deleted: objsnd_dynamic.patch
2012-04-01 13:36 m_m File Added: objsnd_dynamic.patch
2012-04-01 14:25 Echelon9 Note Added: 0013434
2012-04-01 15:36 m_m Note Added: 0013436
2012-04-01 15:36 m_m File Deleted: objsnd_dynamic.patch
2012-04-01 15:37 m_m File Added: objsnd_dynamic.patch
2012-04-02 13:48 Echelon9 Note Added: 0013437
2012-04-02 14:15 m_m Note Added: 0013438
2012-04-02 14:15 m_m File Deleted: objsnd_dynamic.patch
2012-04-02 14:15 m_m File Added: objsnd_dynamic.patch
2012-04-02 14:44 Echelon9 Note Added: 0013439
2012-04-02 15:25 m_m Note Added: 0013440
2012-04-02 15:25 m_m File Deleted: objsnd_dynamic.patch
2012-04-02 15:25 m_m File Added: objsnd_dynamic.patch
2012-04-02 16:59 m_m File Deleted: objsnd_dynamic.patch
2012-04-02 16:59 m_m File Added: objsnd_dynamic.patch
2012-04-02 17:00 m_m Note Added: 0013441
2012-04-02 21:31 Echelon9 Note Added: 0013442
2012-04-02 21:31 Echelon9 Status code review => resolved
2012-04-02 21:31 Echelon9 Fixed in Version => 3.6.14 RC5
2012-04-02 21:31 Echelon9 Resolution open => fixed
2012-04-02 21:31 Echelon9 Changeset attached => fs2open trunk r8649
2012-04-02 21:31 Echelon9 Note Added: 0013443
2012-05-02 11:37 niffiwan Changeset attached => fs2open fs2_open_3_6_14 r8733
2012-05-02 11:37 niffiwan Note Added: 0013507