[Scummvm-devel] SDL backend modularization remarks
yotam barnoy
yotambarnoy at gmail.com
Tue Jun 1 13:50:37 CEST 2010
I believe you meant Alejandro's project.
On Tue, Jun 1, 2010 at 2:32 PM, Max Horn <max at quendi.de> wrote:
> Hi Tony, hi ScummVM developers,
>
> some remarks on Tony's project (which is about modularizing the SDL backend
> and providing OpenGL (ES) support) and recent commits... I was not really
> privy of the design discussion(s) with your mentors, so I am sorry if some
> of these are redundant remarks, but here they are anyway. Note: This is not
> meant to "shoot down" your project. I have my clear cut opinions on how the
> backend code should look and work, but I am always willing to listen to
> other points of views and reasons; but so far I heard none on this matter,
> and I feel it is important that we resolve this before you spend even more
> time on an approach that may need serious changes...
>
> I am also not saying that you must implement *all* of the things I describe
> below, after all, some of them may go beyond your task. But I thought it's
> best to describe my vision for this in more generality, to help bring across
> my point of view :)
>
>
> The first thing that I noticed is that you introduce lots of intermediate
> classes, and also heavy use of multiple inheritance and diamond shaped
> inheritance. That is a feature that has quite some pitfalls and makes things
> complicated, and I really question that design choice. I really would prefer
> to avoid this as much as possible. Luckily, it seems we can avoid it almost
> completely :).
>
> Our current approach to modularize the backend code is based on component
> aggregation to model subsystems. The idea is that we provide a set of
> components (modules), and then an actual backend can easily pick and combine
> components as it likes to, even dynamically (in theory at least, we don't
> really need this ability right now). This is the point behind timer
> managers, event managers, file system factories etc.
>
> IMO this makes it easier to combine submodules than the inheritance model
> you are now introducing, which open up all sorts of potential pitfalls for
> backend authors. Moreover, the new classes now introduce a duplication of
> functionality with the old component based approach; so if we go with it, it
> seems to me that we should at the same time drop the old component based
> stuff (but I am right now very skeptical that this is the right way to go).
> Let me clarify with concrete examples:
>
> Timer code
> ==========
>
> In the current code base, it is possible for a backend to use a custom
> "TimerManager" component. If desired, though, it can just use the default
> timer manager, and be done with it (almost all backends do that these days;
> we used to have a custom MorphoOS timer manager, and now there is a custom
> one for PSP). All it needs to do for that is to provide functionality for a
> timer callback. I.e. just instantiate DefaultTimerManager, hook it up with
> your timer callback, and you are done.
>
> In the case of SDL timers, a natural way to provide an "SDL Timer Manager"
> hence would be to either subclass DefaultTimerManager, adding code to hook
> up the timer handler inside this subclass; or alternative provide the code
> to do this in another easily sharable location, such as a function or class
> that does it and which can be optionally compiled.
>
> Instead, you now introduced BaseSubSys_Timer to model the "timer" module
> (while retaining the TimerManager component, and also the
> DefaultTimerManager). This seems to me to add no useful functionality, but
> *does* now require you to use virtual inheritance, and diamond shaped
> subclassing. Granted, this class (or rather its subclass for SDL) provides
> some extra functionality, but I will argue below why it can go.
>
> My recommendation:
> * Scrap BaseSubSys_Timer (side note: it currently contains no functionality
> anyway, and provides only init/deinit methods, so nothing lost)
>
> * Scrap SdlSubSys_Timer
>
> * Use the code from SdlSubSys_Timer::timerInit and timerDone() to make a
> new class SDLTimerManager, which subclasses DefaultTimerManager and has this
> in its constructor / destructor
>
> * the various *feature* methods should be removed: they are (a) useless
> (contain no code), and (b) dangerous (as you overload them in several class
> in the middle of a diamond shaped inheritance.
>
> * The check for a custom _timer object in SdlSubSys_Timer::timerInit
> becomes redundant this way -- if a backends wants to use a custom timer, it
> won't use this TimerManager subclass *at all*
>
> * The other functions in SdlSubSys_Timer are either trivial (getMillis,
> delayMillis), or not generic to all SDL ports (getTimeAndDate, which uses
> time() and localtime(), which do not exist on all SDL platforms). The
> following alternatives all seem viable to me:
> - just live with the minor code duplication
> - for getMillis & delayMillis, we could add the code directly to the
> "base" SDL backend class (from which all SDL based backends derive,
> including the primary "real" SDL backend). The methods they call are present
> anyway, and if a subclass needs to customize them, it can just overload
> them; we just loose a few dozen bytes of binary size this way, nothing worse
> - we could add some or all of these to TimerManager. Drawback: This would
> force all backends to subclass TimerManager or DefaultTimerManager. Not sure
> this is worth it.
>
> File code
> =========
>
> I can say similar things about some of the other "modules, e.g. the file
> module
>
> * BaseSubSys_File can be scrapped
>
> * SdlSubSys_File can be scrapped, because the following holds for its
> methods
> - fileInit only creates the _savefile object. Just put this into the base
> SDL backend's init() method:
> if (_savefile == 0) {
> _savefile = new DefaultSaveFileManager();
> }
> Then the "POSIX" subclass of the SDL backend simply overloads init with
> something like this
> void POSIX_SDL_OSystem::init() {
> if (_savefile == 0) {
> _savefile = new POSIXSaveFileManager();
> }
> Base_SDL_OSystem::init();
> }
>
> - fileDone can be put into the destructor or "deinit" method of the "base"
> SDL backend class
> - the *feature* methods are useless (see above)
> - getFilesystemFactory & getSavefileManager can just be directly moved
> into the base SDL backend, together with the member variables _savefile and
> _fsFactory
>
> - addSysArchivesToSearchSet: add an empty implementation to the base SDL
> backend class; and move the code in
> SdlSubSys_File::addSysArchivesToSearchSet to the POSIX SDL backend class.
> The OS X specific one could be moved to an according class, too, of course.
>
> - make getDefaultConfigFileName() a pure virtual method of the base SDL
> backend; move createConfigReadStream & createConfigWriteStream to this
>
>
> Mutex code
> ==========
>
> It seems to me that SdlSubSys_Mutex (and by extension BaseSubSys_Mutex) can
> be scrapped directly. Just provide this functionality directly in the SDL
> base backend. As with getMillis(), this code is tiny and will compile on any
> SDL based backend; it just may not work correctly, but if a sub-backend
> overloads the mutex methods, then they never get called, so no harm done.
>
> Also, having _graphicsMutex in there seems completely wrong to me, even if
> we retain SdlSubSys_Mutex. The graphics mutex is an implementation detail of
> the *graphics* subsystem, so it should be contained there, and the mutex
> should be used via the mutex subsystem (or, better, using the mutex code in
> common/mutex.h).
>
>
> Feature methods
> ===============
> As already mentioned, I think the hasFeature / setFeatureState /
> getFeatureState methods in the various intermediate classes are dangerous;
> moreover, I think they are difficult to use correctly. And in most (all?)
> cases useless, as they do nothing.
>
> If a subsystem really needs to provide custom feature flags, we can find
> another way. E.g. you could simply add specific feature methods for these,
> e.g. hasGraphicsFeature(), and then let the actual OSystem::hasFeature()
> implementation invoke those, and bit-OR the results together. Likewise for
> setFeatureState, it could call through to setGraphicsFeatureState, if
> desired.
>
>
> Audio code
> ==========
>
> This could be achieved similarly to the TimerManager stuff: Just make a
> SDLMixerHandler class which contains the mixer specific code, like this:
> class SDLMixerHandler {
> public:
> SDLMixerHandler();
> ~SDLMixerHandler();
>
> init();
> deinit();
> protected:
> mixerProducerThreadEntry(...);
> mixCallback(...)
>
> Audio::MixerImpl *_mixer;
> ....
> }
>
> Then, when an SDL backend wants to use the "normal" audio code, it can
> just instantiate this component, and use it.
> Alternatively (maybe more elegant), subclass Audio::MixerImpl directly,
> and add the required init/deinit methods and code directly in there.
>
> Note that in either way, the current code in setupMixer() should be
> reviewed; many backends use very similar but not completely identical
> versions of that; you may need to split this method further to allow useful
> overriding (no matter where the method(s) live in the end). Or not.
>
> This only covers the mixer related code and leaves the Audio CD code. For
> many backends, it makes absolutely *no* sense to have this code! Indeed, the
> IMHO correct approach here would be the following: Turn AudioCDManager from
> a singleton into another backend component (similar to TimerManager and
> EventManager). Remove the whole Audio CD code from OSystem, instead add
> virtual Common::AudioCDManager *get AudioCDManager() = 0;
> to OSystem. Turn the current AudioCDManager into a new
> DefaultAudioCDManager which does *not* try to invoke the (now removed) Audio
> CD APIs in OSystem. Instead, backends which want to provide "real" audio CD
> support need to provide their own AudioCDManager subclass. AFAIK only three
> backends do this:
>
> 1) The SDL backend (and even here it only makes sense for a very few
> ports).
> 2) The Nintendo DS backend (has no real CD, but allows playing DVI ADPCM
> compressed WAVE files for audio tracks, as the regular compression based
> AudioCDManager code apparently is to resource intensive for the NDS?!)
> 3) The Dreamcast backend (can actually play CDs)
>
> These could subclass DefaultAudioCDManager to restore the current
> functionality (or in the case of the NDS port, if it really cannot use the
> regular CD "emulation" code, it could directly subclass AudioCDManager).
>
>
> Graphics & event code
> =====================
> I can't really comment on these yet, as there is no code there yet, only
> the empty BaseSubSys_Graphics and BaseSubSys_Events shells which don't even
> specify an API. However, I already now wonder if class BaseSubSys_Events
> could maybe be replaced by using / subclassing EventManager appropriately
>
>
>
>
> Cheers,
> Max
>
> ------------------------------------------------------------------------------
>
> _______________________________________________
> Scummvm-devel mailing list
> Scummvm-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/scummvm-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20100601/54bed540/attachment.html>
More information about the Scummvm-devel
mailing list