[Scummvm-devel] SDL backend modularization remarks

[Max Horn]

Am 07.06.2010 um 23:38 schrieb vgvgf:

> Hi Max,
> 
>> 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.
> Ok, then I will create a ModularBackend, for not messing with other backends.

Waiting for that then :).

> 
>> 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?
> Yes, some of them are commented in my code, with some FIXME until I
> find the best solution. For example:
> _km (for keyboard mouse pos), used in both events and graphics
> functions.

This is *definitely* part of the event stuff; it's used to emulate a mouse via keyboard controls.

> It's largely used in events, so I will put it in the
> SdlEventManager when done, but it's modified in graphics in
> loadGFXMode. I think that I'll move the code that modifies it in the
> graphics manager to a new resetKM() method in events and call it from
> the graphics manager.

Sounds sensible. I'd suggest using more descriptive names, though, e.g. resetKeyboadEmulation() or so



> _modeChanged from graphics is toggled in pollEvent for
> EVENT_SCREEN_CHANGED. Now I have it on public on the graphics manager,
> but I should change that and use a function instead.

Here's a simpler solution that might work (but you should test it): Use EventManager::pushEvent() to insert the screen update event. Drawback: This could lead to multiple screen change events, which could have negative effects.
Alternative solution: Get rid of _modeChanged. Instead, to determine whether a change occurred, use the getScreenChangeID() method, which is public anyway. A mode change, by definition of that function, occurred if the return value of getScreenChangeID() differs from the previous one. So your event class would store the last value, and the code which checks _modeChange would instead compare this old value to the new getScreenChangeID.

Yet another variation: This logic could be made part of the DefaultEventManager class. That way, backends would only have to implement the getScreenChangeID() API, and the event manager would take care of generating the screen change events.


> Also, the same goes for _forceFull in graphics.

For this I'd add a method to your Graphics subsystem which does that. E.g. "markScreenAsDirty()" or "forceFullRedraw()"

> _mouseCurState from graphics needs to be updated when the cursor pos
> changes in events. Now, the events code call setMousePos in graphics
> manager. I could leave it as it is, or maybe something better can be
> done to update the cursor pos in graphics.

Hmm... Any reason you think one might want to do something "better" ? What's wrong with the current way?

> 
> However, I'm focusing now on finishing the code division between the
> managers first , and then I'll start looking for those methods and
> data that is needed to be shared with the other managers. When
> finishing all managers, it should be easier to relocate and create
> functions for accessing data or telling the other managers to do
> something.


This reminds me: The WinCE backend has a tweaked variant of the _km code, with some small modifications. Are you looking at how to adapt it (and the Symbian backend, any any using SDL parts), too? The way they work may affect various decisions in how your code is going to work...
Actually, do you even have a setup to compile the WinCE, Symbian, etc. ports?

> 
>> 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.
> Right, I have been treating null and default classes as the same
> thing, when I shouldn't. I will then create a BaseGraphicsManager, as
> a pure virtual class,

Just call it GraphicsManager, no need for the "Base" 

> and have the actual DefaultGraphicsManager
> renamed to NullGraphicsManager.

Aye. And make sure that showMouse() default implementation actually returns something, otherwise I get a compile warning about a non-void function not returning anything ;).

> And, for a real DefaultGraphicsManager I could implement the
> fillScreen and displayMessageOnOSD that are now in BaseBackend, and I
> could also implement some basic variables, like _width, _height,
> _overlayVisible, _videoMode, and some others as well as some functions
> that seem to be used in all backends. Or maybe leave it as it is now,
> and make each derived manager to implement all on its own. What do you
> think?

I'd not add a DefaultGraphicsManager for now, just an SDLGraphicsManager, I think, until further analysis of other backends, and maybe discussion with their authors. We can still do this a bit later; you already have quite a bit on your plate that should be cleared first.

But *if* we add a DefaultGraphicsManager, another thing it could do is provide a default implementation of grabPalette: The idea would be this: Provide DefaultGraphicsManager::setPalette, which copies the passed in data to a buffer owned by the DGM. It then proceed to call the real method, setPaletteIntern, which is pure virtual, and subclasses would have to override. And grabPalette then would just return data from that buffer.

This (a) removes some code shared between all backends, and (b) makes sure that grabPalette returns data identical to what was passed to setPalette in the first place. This is currently not the case on several ports, as they internally store the palette in 16bit format, which causes a loss when converting it back to 24bit colors.


Cheers,
Max