View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002632 | FSSCP | sound | public | 2012-03-26 08:54 | 2012-05-02 11:37 |
Reporter | m_m | Assigned To | m_m | ||
Priority | normal | Severity | minor | Reproducibility | N/A |
Status | resolved | Resolution | fixed | ||
Product Version | 3.6.13 | ||||
Target Version | 3.6.13 | Fixed in Version | 3.6.14 RC5 | ||
Summary | 0002632: Sounds assigned to objects get dropped when limit hardcoded gets reached | ||||
Description | While 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. | ||||
Tags | No tags attached. | ||||
|
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? |
|
Ah, I missed to add a constant for that. New patch is attached. |
|
Rather than using the template: for(idx=0; idx<objp->objsnd_num.size(); idx++)... Can you write it using STL iterators? |
|
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. |
|
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. |
|
(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. |
|
(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);' |
|
Now I understand what you mean. Of course that's a mistake on my side, patch changed accordingly. |
|
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()? |
|
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. |
|
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; |
|
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. |
|
Looks good. Committing to trunk now. |
|
Fix committed to trunk@8649. |
|
Fix committed to fs2_open_3_6_14@8733. |
fs2open: trunk r8649 2012-04-02 17:32 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 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 |
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 |