[Scummvm-devel] [Scummvm-cvs-logs] SF.net SVN: scummvm: [25632] scummvm/trunk/engines/agos

[Max Horn]

Am 16.02.2007 um 14:55 schrieb kirben at users.sourceforge.net:

> Revision: 25632
>           http://scummvm.svn.sourceforge.net/scummvm/? 
> rev=25632&view=rev
> Author:   kirben
> Date:     2007-02-16 05:55:01 -0800 (Fri, 16 Feb 2007)
>
> Log Message:
> -----------
> Add support for using soundtrack from the Simon the Sorcerer 1 -  
> Music Enhancement Project.

That's good, I guess.

However, I disagree with the way it is implemented, in particular, I  
object against (ab)using (IMO) the AudioCDManager to do this.

The AudioCDManager was written to do one very specific thing: Support  
playback of Audio CD tracks for games that use this. Including  
playback from the actual Audio CD (which it will always attempt if an  
Audio CD is available -- even though it makes no sense to even *try*  
this for this "Music Enhancement Project"). Or, failing that, via  
loading & playing track1.* files in various formats.

Also, there is usually no clean mapping between "track numbers" and  
song identifiers. Eugene today asked me whether it's OK to add some  
kind of "tracks.toc" index which can be used to map track IDs to nice  
filenames. IMO, the desire to do this is a clear indicator that we  
are trying to bend a piece of code to do things it was never intended  
for.

Furthermore, the AudioCDManager code base is in a rather sorry state  
(or rather, the DigitalTrackInfo subclasses are). Rewriting it  
properly has been on the TODO for a long time now (but nobody was  
either interested enough or dared to attempt it, it seems).

Anyway, adding this all together, I believe that using the  
AudioCDManager to play custom replacement music is, in fact, abuse.

The alternative is clear: Use AudioStream::openStreamFile() to open  
the "track" (you can specify arbitrary file names w/o extension, like  
"track1", or "myCustomSongName"). Then, just play that AudioStream.

Eugene went on to mention that AudioCDManager support looping. My  
reply to that is: Yes it does, but in a rather crude way, which,  
besides other things, relies on the engine to frequently invoke  
AudioCD.updateCD(). Much better to add a proper looping mechanism  
into the various AudioStream clases. To this end, we'd extend  
makeMP3Stream() by a numLoop parameter.

Yes, I know that using AudioCDManager is the easiest and quickest way  
to get this custom "tracks" support up and running. But I think it's  
in fact the quick & dirty "solution". If the current code, as it is  
in the AGOS engine, is enough to completely fulfill all your needs,  
well, then I guess I could live with it. But as soon as people want  
to start adding things like track name mapper tables to  
AudioCDManager, I feel very strongly that we are heading in the wrong  
direction.


Bye,
Max