Opened 16 years ago

Closed 16 years ago

Last modified 7 months ago

#8225 closed patch

Experimental costume.cpp cleanup

Reported by: eriktorbjorn Owned by: fingolfin
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game:

Description

Something I've been wondering about for a long time now
is whether or not we actually need all the different
proc*() functions in costume.cpp. As far as I can tell,
they're all the same function, optimized for different
circumstances.

But is that optimization noticeable on today's
hardware? If not, all they do is to make the code less
maintainable.

This patch removes all the proc*() functions, except
one which has been slightly extended. I don't know if
making this change is a good idea or not, but I think
it's worth discussing. It would cut the size of
costume.cpp to about half its current size.

Mind you, I haven't tested this very much yet. In
particular, I don't have any of the Amiga data files,
so I may very well have broken those games horribly.

Ticket imported from: #738859. Ticket imported from: patches/330.

Attachments (2)

costume-cleanup.diff (20.2 KB) - added by eriktorbjorn 16 years ago.
Patch against a May 16 CVS snapshot
costume-cleanup2.diff (21.3 KB) - added by eriktorbjorn 16 years ago.
Patch against a May 17 CVS snapshot

Download all attachments as: .zip

Change History (9)

Changed 16 years ago by eriktorbjorn

Attachment: costume-cleanup.diff added

Patch against a May 16 CVS snapshot

comment:1 Changed 16 years ago by fingolfin

Actually I don't understand how the new code is supposed to support the
amiga costumes. Did you notice that the amiga codecs actually are rotated,
i.e. vertical and horizontal axes are exchanged ?!? I see nothing in this patch
to accomodate for it, so I'd be suprised if it worked.

For an explanation of why there are so many functions: that's how the
original did it. But yeah on today's system the speed up probably is
neglectible ("probably" I say, would be good to verify this on e.g. PocketPC/
PalmOS devices...).

Always masking (as your patch does) sounds like a potentially risky change
to me, too. Maybe it works, maybe it was just done as an optimization. But I
wouldn't be surprised if it caused regressions, too

comment:2 Changed 16 years ago by eriktorbjorn

No, I didn't realize the Amiga functions were different in
that way. I thought it was just the handling of the shadow
palette. Thanks for setting me straight!

I guess always masking could theoretically cause
regressions, but don't we already always mask, if there is a
mask where the costume is to be drawn? Except maybe for
Z-plane 0? Anyway, even with just one function we could make
it possible to toggle masking on or off but, as you said, it
would be good to test it on more modest hardware.

I still think it would be nice to at least clean up the code
a bit, because over the time they have grown syntactically a
bit different, even where they are semantically the same,
which makes them a bit harder to maintain.

For instance, it wasn't until after I had made this
experiment that I realized that there probably is a masking
bug in proc_special(). The "black light" in Sam & Max looks
a bit more correct with the patch than without, even if the
colours are still off. I'll submit a separate patch for
that, though -- one that will hopefully improve the colours
a bit, as well.

comment:3 Changed 16 years ago by fingolfin

I am all for cleaning this up and merging the function (if you check cvs
annotate, in fact in the past I made some attempts to at least unifies
the different proc() methods, they used to be even more different than
they are now :-)

It's just that we should be aware that this might cause issues. If we
make the change, we should then proceed to perform tests with all
games and watch out esp. for bugs which could be caused by the
change (or existing bugs which are fixed by it, of course :-)

Changed 16 years ago by eriktorbjorn

Attachment: costume-cleanup2.diff added

Patch against a May 17 CVS snapshot

comment:4 Changed 16 years ago by eriktorbjorn

I've update the patch a bit. This time i kept both proc3()
and proc3_ami(), and added some provisions for turning the
masking off, though at the moment it's always on.

I haven't seen any regressions yet, but of course the only
way to be sure would be to play through all of the games
that use costumes.

comment:5 Changed 16 years ago by fingolfin

Owner: set to fingolfin
Status: newclosed
Summary: Experimental costume.cpp cleanup (don't apply)Experimental costume.cpp cleanup

comment:6 Changed 16 years ago by fingolfin

Looks good to me. I'll check it in and then we can fix any regressions till the
next release.

comment:7 Changed 7 months ago by digitall

Component: Engine: SCUMM
Note: See TracTickets for help on using tickets.