2019-10-16 14:48 EDT


View Issue Details Jump to Notes ] Related Changesets ]
IDProjectCategoryView StatusLast Update
0002632FSSCPsoundpublic2012-05-02 07:37
Reporterm_m 
Assigned Tom_m 
PrioritynormalSeverityminorReproducibilityN/A
StatusresolvedResolutionfixed 
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.
Attached Files
  • patch file icon objsnd_dynamic.patch (8,171 bytes) 2012-04-02 12:59 -
    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;
    
    patch file icon objsnd_dynamic.patch (8,171 bytes) 2012-04-02 12:59 +

-Relationships
+Relationships

-Notes

~0013428

CommanderDJ (developer)

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?

~0013429

m_m (developer)

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

~0013431

Echelon9 (developer)

Rather than using the template:

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

Can you write it using STL iterators?

~0013433

m_m (developer)

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.

~0013434

Echelon9 (developer)

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.

~0013436

m_m (developer)

(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.

~0013437

Echelon9 (developer)

(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);'

~0013438

m_m (developer)

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

~0013439

Echelon9 (developer)

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()?

~0013440

m_m (developer)

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.

~0013441

m_m (developer)

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.

~0013442

Echelon9 (developer)

Looks good. Committing to trunk now.

~0013443

Echelon9 (developer)

Fix committed to trunk@8649.

~0013507

niffiwan (developer)

Fix committed to fs2_open_3_6_14@8733.
+Notes

+Related Changesets

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