lanceewing / agile

Sierra AGI (Adventure Game Interpreter) engine written in C#
15 stars 7 forks source link

Gold Rush room10 is broken #64

Closed vafada closed 1 year ago

vafada commented 1 year ago

side-by-side comparison with WinAGI for reference

maybe a bug in the fill method when drawing a picture?

looks like the brown (visual) / green (priority) is leaking

Capture Capture2

lanceewing commented 1 year ago

It looks like the issue is with the "Plot" command that is drawn before the Fill. There are two black pixels that are not drawn, and those two pixels are what should stop the flood from leaking.

lanceewing commented 1 year ago

Yeah, the second and third plot points are not drawn, i.e. the 0, 58 and 0, 60 points.

I wonder if there is something stopping points from being drawn in the 0 column of the picture...

lanceewing commented 1 year ago

The data for the plot points are:

0x01 0x3A  (1, 58)
0x00 0x3A  (0, 58)
0x00 0x3C  (0, 60)
0x01 0x3C  (1, 60)
0x07 0x3B  (7, 59)
0x07 0x3A  (7, 58)
0x06 0x35  (6, 53)

The second and third ones are not being drawn.

lanceewing commented 1 year ago

It looks like the X position for the pixel is getting 1 added to it, and so is being drawn in the same positions as two of the other plot points (i.e. (1, 58) and (1, 60)).

I think it is a bug in the code that does an adjustment if the plot would go outside the bounds of the picture. It is erroneously being applied for these pixels.

As WinAGI describes it: "If the coordinates passed to the plot action would result in the selected pen shape extending past any edge of the picture (for example, a size >1 with x value = 0 and/or y value = 0), AGI will automatically adjust the coordinate position so the entire brush shape will fit."

...but this is not the case with these pixels. They are 1x1 pixels, i.e. plots of pensize 0, so they will never overlap the edge of the picture in the way that a pensize of greater than 0 might.

lanceewing commented 1 year ago

This bug doesn't affect only pensize 0. It is also affecting pensize 2, 4, and 6. The calculation is wrong. Those 4 pensizes are all adding one more than they should, which leaves a gap on the left.

lanceewing commented 1 year ago

This is the problem:

           if (x < ((penSize / 2) + 1))
            {
                x = ((penSize / 2) + 1);
            }

It should instead be:

           if (x < ((penSize + 1) / 2))
            {
                x = ((penSize + 1) / 2);
            }

This bug is also in PICEDIT, and even in my original SHOWPIC.C code that predates PICEDIT. I'm guessing that the picture code in the AGI Library is based on the code in PICEDIT or SHOWPIC, so even though I didn't write the picture bit of the AGI Library, this is probably still a bug that I introduced, probably 25 years ago!!

lanceewing commented 1 year ago

@vafada , this is now fixed. Can you test again to confirm?

vafada commented 1 year ago

Fix confirmed.. Closing issue.. thanks!

Capture

vafada commented 1 year ago

This is the problem:

           if (x < ((penSize / 2) + 1))
            {
                x = ((penSize / 2) + 1);
            }

It should instead be:

           if (x < ((penSize + 1) / 2))
            {
                x = ((penSize + 1) / 2);
            }

This bug is also in PICEDIT, and even in my original SHOWPIC.C code that predates PICEDIT. I'm guessing that the picture code in the AGI Library is based on the code in PICEDIT or SHOWPIC, so even though I didn't write the picture bit of the AGI Library, this is probably still a bug that I introduced, probably 25 years ago!!

Can't see the code in BitBucket. I think JAGI does the same thing:

https://github.com/vafada/jagi/blob/24ed9ce7bba43f31c45c55f9dfc1527df2e7906a/src/com/sierra/agi/pic/PictureEntryPlot.java#L99

Surprisingly, JAGI renders that room with no issue: Capture

lanceewing commented 1 year ago

Looking at that JAGI code, I think this bit is the equivalent adjustment:

https://github.com/vafada/jagi/blob/24ed9ce7bba43f31c45c55f9dfc1527df2e7906a/src/com/sierra/agi/pic/PictureEntryPlot.java#L90

        if (x < penSize) {
            x = penSize - 1;
        }

This is making me wonder now if maybe I have the wrong fix.

This adjustment is not the same, or equivalent, to what I have done, and would give different results. I think I'll need to do some tests with the original AGI interpreter to determine which adjustment is correct. The above one from JAGI would mean that sometimes it is adjusted such that it isn't "touching" the edge, but rather introduces a gap. That doesn't sounds right to me, but that doesn't mean it isn't how the original AGI interpreter did things.

lanceewing commented 1 year ago

@vafada , I have tested with the original AGI interpreter and believe that my fix is correct. The plotted pen shape should be repositioned such that the left edge of the shape touches the left side of the screen, i.e. the x position is 0 for that left side of the shape. The JAGI code doesn't appear to do that.... unless penSize represents something else. I haven't looked back to see if that is true. But if penSize is a value from 0 to 7, then I don't think it is right that the adjustment would always be penSize - 1.