[Scummvm-devel] SDL backend modularization remarks

Max Horn max at quendi.de
Mon Jun 7 12:06:57 CEST 2010


Hi Alejandro!

Am 06.06.2010 um 20:33 schrieb vgvgf:

> Hi Max,
> 
>> If the classes involved are completely independent from each other (seems to be the case
>> to me), then of course there is no need to rename the feature methods for each. However, I
>> still wonder whether we really need to have empty feature check methods in every
>> "Manager" ? I'd prefer adding them only if we determine that we *really* need them ;). In
>> particular in the "mutex manager", I have a hard time to imagine we'd ever need them --
>> and if we *do* need them, we can still add them, in a way that suits our hypothetical future
>> needs optimally.
> I added those feature methods just for the case they are needed in a
> future, I'll delete them from the managers like mutex and others that
> have it not probable for using them.

Thanks, looking forward to those changes :). And note: We can always re-add the feature method should we discover we need them.


> 
>> Likewise, I am not quite sure what purpose DefaultAudioManager serves, it seems to be an
>> empty class inserted between Audio::MixerImpl and hypothetical subclasses of it. Again, I
>> fail to see the purpose of this -- note, I am not saying there *isn't* one, maybe, I just don't
>> see if, and your code and its comments don't really give one, either :).
> Maybe I should get rid of it, and stick with the actual
> Audio::MixerImpl, as the main purpose was to provide the feature
> methods, and making it a non copyable class, as other managers.

You are most welcome to make MixerImpl or Mixer subclass from NonCopyable. Actually, I think it would make a lot of sense to do so.


> 
>> The long term plan would in fact be to get rid of the Audio CD methods in OSystem
>> completely, in favor of a revised AudioCDManager, as described in my previous post :).
> I have been looking in to the code, and it seems that the actual
> AudioCDManager singleton isn't used largely, and the calls to it can
> be easily redirected to a improved AudioCDManager in OSystem, so I'll
> be working on this way better.

Great :)

> I have also been thinking that the base backend could be extended,
> adding initialization code for the default managers, and creating
> wrapping functionsin the backend for the managers functions. This way,
> a backend would only need to initializate its personal backends and
> some other code, while it won't need to override all functions from
> OSystem and oly a few.

That might be helpful, aye! The less common code backend authors have to re-implement, the easier it gets to write and maintain backends, and to adapt them to future API changes.
However, for now it is important that this is not forced upon devs, i.e., I'd put this into a "ModularOSystem" or "ModularBackend" class, derived from OSystem; this will make transition to the new modular system easier, and for ports which already work well and are not going to share much code anyway, it means that they are not forced to suddenly modularize everything.


Now something else: On your wiki, you wrote: "There are some problems I still need to fix, like where to place shared code between the subsystems or how is the better way for the managers to interact with each other." Well spotted, this is indeed one of the things that need some thought. Have you already identified specific things that may need sharing?


Finally, some remarks on DefaultGraphicsManager: Its destructor should be made virtual. And I actually think it would be better to have a GraphicsManager() base class, with all those mandator methods made pure virtual, and let SdlGraphicsManager inherit from that. And DefaultGraphicsManager should be renamed to NullGraphicsManager, as that is what it does: Implement an "empty" graphics manager, suitable for a modularized version of our "null" backend. It is *not* a "default graphics manager" in the sense that a real backend would actually want to use it, unlike e.g. the DefaultTimerManager.

Bye,
Max



More information about the Scummvm-devel mailing list