[Scummvm-devel] Fwd: [Scummvm-cvs-logs] SF.net SVN: scummvm:[45775] scummvm/trunk/backends/platform

Pawel Kolodziejski aquadran at xtr.net.pl
Tue Nov 10 18:21:27 CET 2009


On 2009-11-10, at 13:27, Pawel Kolodziejski wrote:

>
> On 2009-11-09, at 23:15, Max Horn wrote:
>
>>
>> Am 09.11.2009 um 22:35 schrieb Max Horn:
>>
>>>
>>>
>>> Anfang der weitergeleiteten E-Mail:
>>>
>>>> Von: Pawel Kolodziejski <aquadran at xtr.net.pl>
>>>> Datum: 9. November 2009 17:21:14 MEZ
>>>> An: Max Horn <max at quendi.de>
>>>> Betreff: Re: [Scummvm-cvs-logs] SF.net SVN: scummvm:[45775]  
>>>> scummvm/
>>>> trunk/backends/platform
>>>>
>>>>
>>>> On 2009-11-09, at 15:58, Max Horn wrote:
>>>>
>>>>>
>>>>> Am 09.11.2009 um 15:29 schrieb aquadran at users.sourceforge.net:
>>>>>
>>>>>> Revision: 45775
>>>>>>     http://scummvm.svn.sourceforge.net/scummvm/?
>>>>>> rev=45775&view=rev
>>>>>> Author:   aquadran
>>>>>> Date:     2009-11-09 14:29:53 +0000 (Mon, 09 Nov 2009)
>>>>>>
>>>>>> Log Message:
>>>>>> -----------
>>>>>> added samsung tv backend
>>>>>>
>>>>>> Added Paths:
>>>>>> -----------
>>>>>> scummvm/trunk/backends/platform/samsungtv/
>>>>>> scummvm/trunk/backends/platform/samsungtv/events.cpp
>>>>>> scummvm/trunk/backends/platform/samsungtv/graphics.cpp
>>>>>> scummvm/trunk/backends/platform/samsungtv/hardwarekeys.cpp
>>>>>> scummvm/trunk/backends/platform/samsungtv/main.cpp
>>>>>> scummvm/trunk/backends/platform/samsungtv/module.mk
>>>>>> scummvm/trunk/backends/platform/samsungtv/sdl.cpp
>>>>>> scummvm/trunk/backends/platform/samsungtv/sdl.h
>>>>>
>>>>> This backend seems to duplicate most of the SDL backend -- very
>>>>> ugly. Any particular reason it couldn't subclass the SDL backend,
>>>>> instead of duplicating all that code?
>>>>
>>>> It's in progress, so subclass not yet, and there are small changes
>>>> which does not allow much of methods as subclass. Also I don't want
>>>> put hacks into main sdl code.
>>
>> Well, this new backend is still 95% identical to the SDL backend. I
>> find this rather aggravating :/. As we make changes to the SDL
>> backend, they will start to deviate further, though. I am scared by
>> the prospect of having this SDL backend clone in there and starting  
>> to
>> bitrot, just like ther old sdl-opengl backend. The SDL backend  
>> already
>> contains tons of #ifdefs etc., so on the one hand it is not nice to
>> had more, but on the other hand, it's not really making things
>> worse...
>>
>> But looking at your changes specifically, it seems to me that it  
>> would
>> still be far preferable and indeed quite doable to realize this as a
>> subclassed backend, by turning only a few key methods into virtual
>> ones, resp. factoring out some code that you have to implement
>> differently for your backend into new virtual methods. This will lead
>> to much better code on the long run.
>>
>> Such a refactoring would be a nice small step towards refactoring the
>> SDL backend to make life better for backends derived from it, such as
>> the WinCE & Symbian backends.
>>
>>
>> Disregarding the issues I have with the actual code, there is one
>> other thing: I really really *really* dislike learning about a new
>> backend being added via the commit message. This is just as bad as if
>> it happens for an engine. This is really not good practice. In the
>> future, I expect everybody who adds major new components like  
>> backends
>> or engines to announce these *beforehand* on -devel, and wait for
>> approval. Getting approval from either me or Eugene alone is not
>> enough, this must go over -devel. Anyway, it's too late for that now;
>> but please at the very least, write an email to -devel now which
>> explains your new backend, what it is good for, and what your plans
>> are to address the concerns I voiced above.
>
>
> Ok, I'll start review source code about code duplication and why.
>
> events.cpp:
>
> "handleKbdMouse" use SDL_WarpMouse which I can't use, this
> shows SDL mouse cursor, only avoid it it's simply not call it.
> I tried change cursor to transparent but I still getting some
> flickering.
> You may wonder why not disable show mouse cursor. Unfortunately
> SDL_ShowCursor crash SDL.
>
> "pollEvent" looks the same for now, but later it will handle at here
> while
>  (SDL_PollEvent(&ev))  check for events from usb mouse and keyboard.
> This code will be implemented in custom way by reading Linux input
> events
>  from Linux device.
>
> "remapKey" not need much comments. Using remote TV controller is not
> so nice,
> but still want it as fallback due, usb mouse and keyboard support in
> TV is not enabled by default.
> It's needed load kernel modules and create device nodes in filesystem
> for use.
> And only by custom program who handle it.
>
> graphics.cpp:
>
> "scalersMagn" array is static in main sdl code, and later code
> blitCursor use it.
> similiar things is with "cursorStretch200To240" func
>
> "getDefaultGraphicsMode"
> It's temporary instad scaler without filtering, I can't change from
> gui to diffrent
> scaler, selecting mode from listview required press two buttons at  
> once.
> Most probably remote controller can't send two keycode.
>
> "getSupportedFormats"
> has fixed list of pixel format. It's needed for emulating _hwscreen
> surface.
>
> "initSize"
> hmm, forgot to remove
>
> "fixupResolutionForAspectRatio"
> static and need to do fixed list of supported resolutions.
> samsung sdl has empty list defined.
> So I need my self create such list.
>
> "loadGFXMode"
> Whole thing depend on emulating _hwscreen surface.
> SDL tv support practicaly only 32 bit video mode.
> So I can't use 16 bit mode.
> I can't image adding support pixel format conversion on scalers and
> other "memcpy" code.
> I think it's too much complicate, so I decided by simply adding
> compatible surface.
> In this case I changed _hwscreen to _prehwscreen. However this could
> be reverted in
> some methods where that is only thing changed, and methods would not
> need "subclassed".
> So SDL_SetVideoMode would return to _prehwscreen (propably better name
> like _videoscreen).
> And below instead _prehwscreen replace to _hwscreen.
> Maybe you would better solution, but propably that will need change
> code in main sdl.
>
> "unloadGFXMode" and "hotswapGFXMode"
> due freeing _prehwscreen
>
> "internUpdateScreen"
> due _prehwscreen instead _hwscreen, but names could be reverted as I
> mention above.
> and because emulating additional surface I used this:
>         SDL_BlitSurface(_prehwscreen, 0, _hwscreen, 0);
>         SDL_UpdateRect(_hwscreen, 0, 0, _hwscreen->w, _hwscreen->h);
>
> There is still unknown thing regarding update rects in SDL code.
> Code ignore rects and blit whole surface. I need look close into SDL
> source code
> to know if any benefits still using rects or simply dropping.
>
> "setFullscreenMode"
> Samsung tv support only fullscreen code.
> Regarding this It's not clear to me why gui still allow set fullscreen
> mode.
>
> "setMouseCursor" and "blitCursor"
> due fixed pixelformat
>
> "drawMouse"
> due usage _prehwscreen
>
> hardwarekeys.cpp:
>
> Whole code I don't know what to do yet. I keep normal full keyboard
> behavio
>  and map remote controller keys too.
> I don't understand that code yet, so I don't know if I'll change this
> yet or not.
>
> main.cpp
>
> there is not main() here only Game_Main()
> samsung tv software call custom programs and expect it as shared
> library with
> exported entry Game_Main. Also no params for scummvm_main too.
>
> sdl.cpp
>
> timer_handler, AspectRatio and other related arrays needed by
> initBackend()
>
> "initBackend"
> can't call SDL_ShowCursor as I mentioned before.
> Setting scaler to different is temporary as I mentioned before.



> Now I see I need remove  OSystem::initBackend(); from here too.

Of course that need stay, I miss read it.

Pawel





More information about the Scummvm-devel mailing list