[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 13:27:09 CET 2009
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.
"addSysArchivesToSearchSet"
I setup to current dir. tv doesn't have full POSIX directories and due
nature how
custom programs are executed.
Would be nice have DATA_PATH and others like for config and save path
possible customized in configure not in source code.
"getDefaultConfigFileName"
due static DEFAULT_CONFIG_FILE
and "createConfigReadStream" and createConfigWriteStream" due static
"getDefaultConfigFileName"
"hasFeature" and "setFeatureState" and "getFeatureState"
I'm limiting features like fullscreen switch.
"quit"
Can't use exit() as it's not normal program but library.
same thing is in util.cpp in error function.
That is all for now, I'll post a bit later more related backend.
Pawel
>
>
> Bye,
> Max
>
> ------------------------------------------------------------------------------
> Let Crystal Reports handle the reporting - Free Crystal Reports 2008
> 30-Day
> trial. Simplify your report design, integration and deployment - and
> focus on
> what you do best, core application coding. Discover what's new with
> Crystal Reports now. http://p.sf.net/sfu/bobj-july
> _______________________________________________
> Scummvm-devel mailing list
> Scummvm-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/scummvm-devel
>
More information about the Scummvm-devel
mailing list