Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#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 rwtodd)

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. ( 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)

comparison.png (20.1 KB ) - added by rwtodd 4 years ago.

Download all attachments as: .zip

Change History (13)

by rwtodd, 4 years ago

Attachment: comparison.png added

comment:1 by rwtodd, 4 years ago

Description: modified (diff)

comment:2 by m-kiewitz, 4 years ago

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.


comment:3 by m-kiewitz, 4 years ago

Oops I meant @sev-

comment:4 by m-kiewitz, 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 rwtodd, 3 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 m-kiewitz, 3 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.

Last edited 3 years ago by m-kiewitz (previous) (diff)

comment:7 by rwtodd, 3 years ago

Ok, I'll leave it to you then. Good luck!

comment:8 by sev-, 3 years ago

Priority: normalhigh

This bug is nice to get fixed before the release.

comment:9 by sev-, 3 years ago

Owner: set to sev-
Resolution: fixed
Status: newclosed

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:10 by m-kiewitz, 3 years ago

Have you checked preAGI as well?

comment:11 by sev-, 3 years ago

Yes, I saw no difference and no floodfill leaks.

comment:12 by rwtodd, 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!

Note: See TracTickets for help on using tickets.