#11923 closed defect (fixed)
AGI Splatter patterns appear to be incorrect
Reported by: | rwtodd | Owned by: | sev- |
---|---|---|---|
Priority: | high | Component: | Engine: AGI |
Version: | Keywords: | agi pic | |
Cc: | rwtodd | Game: |
Description (last modified by )
When I take, for example, Space Quest 2 room 16 or room 22, and compare the ScummVM image to the game running in DosBox, I can see that the splatter patterns are all wrong in the trees in the background.
I think way back in github commmit 0d279e8de07fc5 when the AGI splatter plotting was redone, the texture_number variable accidentally got hard-coded to 0. In fact, the texture number should start as the pattern number pulled in plotBrush() (notice: the code at present does nothing with _patNum, which is a clue something isn't right).
I think maybe the resulting plot errors were why a subsequent commit changed the ditherCond from 2 to 1. I believe that was incorrect and should be reverted.
To make the games I have look like the original, I believe the following changes are needed:
1) make plotBrush() store the raw byte it reads in _patNum (forget the >>1 and &0x7f part... that's left over from an older table-based approach to splatter).
2) in plotPattern(), initialize t
to (_patNum | 0x01) and forget the zero texture_num.
3) in plotPattern(), change ditherCond back to 2 like it was originally.
I don't have an exhaustive list of games to test this with, but the few AGI games I have which use splatter plots look more accurate to me after these changes. If people agree it's an improvement, I can offer a pull request on github, or I'm just as happy if one of the usual devs makes the fix.
As further evidence, I believe nagi's pic_render.c agrees with the changes I'm suggesting. (https://github.com/sonneveld/nagi/blob/master/src/picture/pic_render.c). It sets texture_num in plot_with_pen(), and I think in the translation to scummvm, texture_num got converted to a local var and always set to zero.
Attachments (1)
Change History (13)
by , 4 years ago
Attachment: | comparison.png added |
---|
comment:1 by , 4 years ago
Description: | modified (diff) |
---|
comment:2 by , 4 years ago
comment:4 by , 4 years ago
Wait, this is very weird. It seems I didn't touch this code at all during my graphics rewrite.
I will have to check my own notes from back then.
The original commit by @sev- is from 2007.
Very weird that I left that one as it was.
comment:5 by , 4 years ago
Well it's been 5 months. Maybe I will submit a pull request with the changes that work for me. Would that be the next thing to try?
The code that's in there is clearly wrong, and the changes I'm suggesting seem to fix the games I have, and as a bonus match more closely with what the nagi engine does. My only hesitation is I don't have every game (and zero preAGI games) to test against.
comment:6 by , 4 years ago
Please don't, this needs to get reverse engineered properly.
Changes may cause issues down the line. The current code is not accurate, but it may be accurate for some version of agi, maybe even (and that's mostly like it) for preAGI.
And thus it needs to figured out for all AGI versions and preAGI out there, and not by checking visually, but the code itself needs to be compared.
I'm kinda busy, but maybe I will lose my job in around 3 months and at least then I will have as much time as needed for stuff like this.
Maybe I will even go through it this extended weekend, at least for regular AGI. I got IDA disassemblies for most if not even all AGI games and checking for code differences shouldn't take too long. I myself don't own all preAGI games, but sev has some, if not even all. I think the code is accurate for preAGI and that's how it got in there, but we really have to see.
comment:8 by , 3 years ago
Priority: | normal → high |
---|
This bug is nice to get fixed before the release.
comment:9 by , 3 years ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Thank you for your report. I double-checked again against the original and indeed, your notes were on the spot. I now committed a fix.
comment:12 by , 3 years ago
I built HEAD and all the games I have look great now. Thanks for your help. It's amazing the defect lasted 14 years!
This seems to be a difference between preAGI and AGI.
I would propose reverting 0d279e8de07fc5137266fb8793dba05e14fccd0b / making it so that the new method is used for preAGI and the original one is used for AGI.
Originally I rewrote AGI to be as accurate as possible and that was all based on reverse engineered work. Please no guesswork, because that could introduce other issues.
@sev