#8225 closed patch
Experimental costume.cpp cleanup
| Reported by: | eriktorbjorn | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | 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)
Change History (9)
by , 22 years ago
| Attachment: | costume-cleanup.diff added |
|---|
comment:1 by , 22 years ago
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 by , 22 years ago
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 by , 22 years ago
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 :-)
comment:4 by , 21 years ago
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 by , 21 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
| Summary: | Experimental costume.cpp cleanup (don't apply) → Experimental costume.cpp cleanup |
comment:6 by , 21 years ago
Looks good to me. I'll check it in and then we can fix any regressions till the next release.
comment:7 by , 6 years ago
| Component: | → Engine: SCUMM |
|---|
Patch against a May 16 CVS snapshot