View Issue Details [ Jump to Notes ] [ Related Changesets ] | [ Issue History ] [ Print ] | ||||||||
ID | Project | Category | View Status | Date Submitted | Last Update | ||||
---|---|---|---|---|---|---|---|---|---|
0002632 | FSSCP | sound | public | 2012-03-26 04:54 | 2012-05-02 07: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. | ||||||||
Attached Files |
|
![]() |
|
CommanderDJ (developer) 2012-03-31 10:00 |
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 (developer) 2012-03-31 11:08 |
Ah, I missed to add a constant for that. New patch is attached. |
Echelon9 (developer) 2012-04-01 08:53 |
Rather than using the template: for(idx=0; idx<objp->objsnd_num.size(); idx++)... Can you write it using STL iterators? |
m_m (developer) 2012-04-01 09:36 |
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 (developer) 2012-04-01 10:25 |
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 (developer) 2012-04-01 11:36 |
(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 (developer) 2012-04-02 09:48 |
(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 (developer) 2012-04-02 10:15 |
Now I understand what you mean. Of course that's a mistake on my side, patch changed accordingly. |
Echelon9 (developer) 2012-04-02 10:44 |
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 (developer) 2012-04-02 11:25 |
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 (developer) 2012-04-02 13:00 |
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 (developer) 2012-04-02 17:31 |
Looks good. Committing to trunk now. |
Echelon9 (developer) 2012-04-02 17:31 |
Fix committed to trunk@8649. |
niffiwan (developer) 2012-05-02 07:37 |
Fix committed to fs2_open_3_6_14@8733. |
![]() |
|||
fs2open: trunk r8649
Timestamp: 2012-04-02 17:32:47 Author: Echelon9 Ported: N/A [ Details ] [ Diff ] |
Fix Mantis 2632: Sounds assigned to objects get dropped when limit hardcoded gets reached. Patch from m_m | ||
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
Timestamp: 2012-05-02 07:38:25 Author: 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 | ||
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 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 |