[Scummvm-devel] Improvements to blitting code.

[Max Horn]

Am 18.08.2008 um 21:13 schrieb carlo.bramix:

> Hello,
> I did a patch, sorry for the delay but I could not do it until now...
> This patch does the algorithm that I described previously.

Thanks, but we applied an alternate version of the patch for now --  
one with comments which explains this now rather obfuscated code (IMO  
a very important thing). :)

Not all of your suggestions are taken up in it, but mostly with good  
reasons, I think:
* Checking for (h && w) is not needed, as the code already does that  
much earlier and never gets to that point if w or h are non-positive.

* using do...while is equivalent to using for(;;) {}; what matters is  
whether you count to 0, which the new code in gfx.cpp indeed does  
(thanks to Robbin).
* removing the increment to the pointers may save cycles on some archs  
-- and wastes some on others. It all depends on whether you have lots  
of registers, or have an efficient "relative address load" instruction.


In the end, I'll prefer readable code that works fine on most systems  
over obfuscated code which *may* be faster on some systems, or not. A  
good optimizer should in the end produce reasonable results for both.  
And while I appreciate that this code is now faster, I do not think we  
should further complicate to squeeze out code, unless we know for sure  
that it's a bottle neck on any system :). Anything else would fall  
into the category "premature optimization".


[...]

> After I downloaded trunk from SVN, I did the fix and then I compiled/ 
> tested SCUMMVM.
> Here you are my little report:
>
> ============================
> I got an error when doing installation:
>
> $ make install
> install -d "/home/carlo_pc/inst_scumm/bin"
> install -c -s -m 755 "/c/Downloads/trunk/scummvm.exe" "/home/ 
> carlo_pc/inst_scumm/bin/scummvm.exe"
> install: cannot stat `/c/Downloads/trunk/scummvm.exe': No such file  
> or directory
> make: *** [install] Error 1
>
> The problem happened because I configured & compiled SCUMMVM from a  
> different directory.
> Since "make install" assumes that scummvm.exe is generated where the  
> sources are, it cannot find it and it fails.
> In my opinion, $EXECUTABLE is always where generated makefile is, so  
> this line of ports.mk:
>
> $(INSTALL) -c -s -m 755 "$(srcdir)/scummvm$(EXEEXT)" "$(DESTDIR)$ 
> (BINDIR)/scummvm$(EXEEXT)"
>
> should be changed to:
>
> $(INSTALL) -c -s -m 755 "./scummvm$(EXEEXT)" "$(DESTDIR)$(BINDIR)/ 
> scummvm$(EXEEXT)"

Well-spotted! Thanks, and fixed.

>
>
> Perhaps the most correct value would be $(top_builddir), but I could  
> not find it...

We do not define that, aye.

> =====================
> It seems to me that asmDrawStripToScreen() function ignores the  
> multiplier, is it known?
> I also found many copies of the some assembly functions, perhaps it  
> could be better if there will be a /arch-common/arm9 (it's just an  
> example) and into the platform files you do an "include arch-common/ 
> arm9/DrawStripToScreen.asm".

Actually, we keep the ASM code right next to the ARM code. So,  
asmDrawStripToScreen is in engines/scumm/gfxARM.s -- there was a  
second copy of the code in the "DS" code, left there by accident. That  
has been resolved now by Robin, though.

See also sound/rate_arm.cpp and sound/rate_arm_asm.s.


> =====================
> My little "feature request":
>
> Into configure script, the selection of an engine is "hardwired",  
> for example:
[...]
>
>
> and allow the use of an option like --enable-sky for activating the  
> engine (those variables are assigned to a default value if no option  
> is given on the command line).

But we already do support "--enable-sky" and "--disable-sky" :). See  
also the output of "--help" :)

Cheers,
Max