[m!m]

I have a patch which adds support for customizable cockpit sounds.
The attached patch adds the following options to a ship class:

$MissileTrackingSnd
$MissileLockedSnd
$PrimaryCycleSnd
$SecondaryCycleSnd
$TargetAcquiredSnd
$PrimaryFireFailedSnd
$SecondaryFireFailedSnd
$HeatSeekerLaunchWarningSnd
$AspectSeekerLaunchWarningSnd
$MissileLockWarningSnd
$HeatSeekerProximityWarningSnd
$AspectSeekerProximityWarningSnd
$MissileEvadedSnd
$CargoScanningSnd

These go in that sequence after $AfterburnerFailedSnd and also have the same syntax (number or string to specify an entry in sounds.tbl).

Regards,
m!m

[attachment deleted by a basterd]

[Iss Mneur]

That is certainly a different way of implementing that other assignable sound ids like weapons, work differently.

Do not use Int3, it causes an impossible to debug crash to desktop on machines that don't have a debugger and is ignored in release builds.  Replace the Int3 with a Warning (or an Error if the condition is fatal) that explains how the wrong data would have gotten in there and have the message generated include incorrect value, something like:

Code: [Select]

"Sound index is %d, it must be less than %d and greater than zero", index, Snds.size()


[Dragon]

I love this, can't wait to see it commited.

[m!m]

That is certainly a different way of implementing that other assignable sound ids like weapons, work differently.

Well, it's not actually very different. I just wanted to avoid complicated constructs like

Code: [Select]

&Snds[Ship_info[Ships[Player_obj->instance].ship_info_index].engine_snd_cockpit]to just get a simple sound index.

Do not use Int3, it causes an impossible to debug crash to desktop on machines that don't have a debugger and is ignored in release builds.  Replace the Int3 with a Warning (or an Error if the condition is fatal) that explains how the wrong data would have gotten in there and have the message generated include incorrect value, something like:

Code: [Select]

"Sound index is %d, it must be less than %d and greater than zero", index, Snds.size()


I thought that that Int3() would be impossible to hit and if it was then it would clearly be a coding mistake but using a Warning is surely the better way to do it.

[Eli2]

The patch looks good
and is definitely a step towards a refactored solution.

[m!m]

Thank you. I have also attached a patch with that Int3() changed to a warning.

[attachment deleted by a basterd]

[Eli2]

Another idea, maybe you could store the sound indices in a SCP_Vector<std::pair<int, int> >.

[m!m]

That is a good idea, I also think that using a SCP_map<int, int> would be even better and a bit more straight forward. I attached a patch which uses that approach.

[attachment deleted by a basterd]

[Eli2]

Looks good!

EDIT:
The forum ate the formatting of my diff.

Here is a cleanup for minor stuff:

better formatting of parse function:

Code: [Select]

void parse_ship_sound(char *name, int id, ship_info *sip)
{
Assert( name != NULL );

int temp_index;

parse_sound(name, &temp_index, sip->name);

if (temp_index >= 0)
sip->ship_sounds.insert(std::pair<int, int>(id, temp_index));
}

void parse_ship_sounds(ship_info *sip)
{
parse_ship_sound("$CockpitEngineSnd:",                SND_ENGINE, sip);
parse_ship_sound("$FullThrottleSnd:",                 SND_FULL_THROTTLE, sip);
parse_ship_sound("$FullThrottleSnd:",                 SND_ZERO_THROTTLE, sip);
parse_ship_sound("$ThrottleUpSnd:",                   SND_THROTTLE_UP, sip);
parse_ship_sound("$ThrottleDownSnd:",                 SND_THROTTLE_DOWN, sip);
parse_ship_sound("$AfterburnerSnd:",                  SND_ABURN_ENGAGE, sip);
parse_ship_sound("$AfterburnerEngageSnd:",            SND_ABURN_LOOP, sip);
parse_ship_sound("$AfterburnerFailedSnd:",            SND_ABURN_FAIL, sip);
parse_ship_sound("$MissileTrackingSnd:",              SND_MISSILE_TRACKING, sip);
parse_ship_sound("$MissileLockedSnd:",                SND_MISSILE_LOCK, sip);
parse_ship_sound("$PrimaryCycleSnd:",                 SND_PRIMARY_CYCLE, sip);
parse_ship_sound("$SecondaryCycleSnd:",               SND_SECONDARY_CYCLE, sip);
parse_ship_sound("$TargetAcquiredSnd:",               SND_TARGET_ACQUIRE, sip);
parse_ship_sound("$PrimaryFireFailedSnd:",            SND_OUT_OF_WEAPON_ENERGY, sip);
parse_ship_sound("$SecondaryFireFailedSnd:",          SND_OUT_OF_MISSLES, sip);
parse_ship_sound("$HeatSeekerLaunchWarningSnd:",      SND_HEATLOCK_WARN, sip);
parse_ship_sound("$AspectSeekerLaunchWarningSnd:",    SND_ASPECTLOCK_WARN, sip);
parse_ship_sound("$MissileLockWarningSnd:",           SND_THREAT_FLASH, sip);
parse_ship_sound("$HeatSeekerProximityWarningSnd:",   SND_PROXIMITY_WARNING, sip);
parse_ship_sound("$AspectSeekerProximityWarningSnd:", SND_PROXIMITY_ASPECT_WARNING, sip);
parse_ship_sound("$MissileEvadedSnd:",                SND_MISSILE_EVADED_POPUP, sip);
parse_ship_sound("$CargoScanningSnd:",                SND_CARGO_SCAN, sip);
}

[m!m]

Thank you very much, the only thing that you should take care of is that after calling

Code: [Select]

parse_sound(name, &temp_index, sip->name);temp_index might end up uninitialized when the option string is not present.

That's why temp_index should be initialized with -1  to be sure that there will be no problems.

[Eli2]

parse_sound should be refactored to return the index instead of void.
EDIT: but that is off topic. good catch.

[m!m]

Here is a new patch with the improved formatting from Eli2.

[attachment deleted by a basterd]

[Echelon9]

Here is a new patch with the improved formatting from Eli2.

I can confirm this version compiles fine on OS X. I'll let others comment on the design.

[Iss Mneur]

The design looks okay to me, except you just turned a O(1) lookup into a O(n) lookup. Admittedly, n shouldn't get too large, but I am interested in some algorithm input from the other coders as well.  Do we use the O(1) or the O(n) lookup?

[Eli2]

Wikipedia says the lookup is O(log n)http://en.wikipedia.org/wiki/Map_%28C%2B%2B%29
EDIT: not that it would matter.

[Iss Mneur]

Wikipedia says the lookup is O(log n)http://en.wikipedia.org/wiki/Map_%28C%2B%2B%29
EDIT: not that it would matter.

Hmm, so it is. Reading the code again, I don't know what I was thinking, it is in fact a map not a vector of pairs like I was apparently thinking. I don't have a problem with O(log n) because of the readability gains that this version gains us.

EDIT: Committed in 7923.  m!m please add the new entries to the wiki if you haven't already.

[m!m]

Thank you for the commit, the options have been added to the wiki.