robhagemans / pcbasic

PC-BASIC - A free, cross-platform emulator for the GW-BASIC family of interpreters
http://www.pc-basic.org
Other
397 stars 48 forks source link

[v1.2 only] PAINT (flood fill) halts execution filling a circle containing dots #120

Closed mrclay closed 2 years ago

mrclay commented 4 years ago

Make a circle, put a few dots inside. Flood filling with black (goal to remove the dots) fails: Execution stops on the line, and breaking reports the PAINT line. Occasionally it completely freezes the app.

Program

10 SCREEN 1:CLS
20 CIRCLE(15,15),4,1
30 PSET(16,16),1
40 PAINT(13,14),3,1
50 LOCATE 5: PRINT "You should see 'PASS!'"
60 LOCATE 6: INPUT "after hitting Enter...", F
70 PAINT(13,14),0,1
80 PRINT "PASS!"

But painting with white works! Change the 0 in line 70 to 3.

PC-BASIC version: 1.2.14 Operating system version: OSX

robhagemans commented 4 years ago

Thanks for the report, I can reproduce in both 1.2.14 and 2.0.2. Interesting one, it looks like the flood-fill algorithm doesn't detect that it is finished with the black fill.

Note for future reference - with a cyan, magenta, or white fill the program finishes correctly, so the problem only occurs with black, which happens to be the background colour.

robhagemans commented 4 years ago

Further note: I checked this on GW-BASIC and what happens is that it simply does nothing - both coloured dots remain in place. This contrasts with filling with anything but the background colour, where all colours but the border colour get overwritten in the paint colour. This explains how it avoids having to detect it is finished - it doesn't start.

mrclay commented 4 years ago

More info after some sleep: This was pulled from a larger Tandy 1000SX program--where it did work, um, 33 years ago--but the behavior didn't seem different between the default preset and tandy preset. More importantly, I swear I witnessed a few other cases where other fill colors failed, but it was getting late and I was getting sloppier.

Tonight I think I can simplify the test case a lot more, ideally a single dot in a simpler shape.

Side note: I was shocked to find this wasn't emulation. It's damn impressive.

mrclay commented 4 years ago

on GW-BASIC and what happens is that it simply does nothing - both coloured dots remain in place.

Huh, I bet I'm wrong about it working; in the program the cleared area gets immediately blasted with other shapes so really I just know it didn't halt execution on the Tandy.

In the fuller program I did notice the fill starts and outlines a small rectangle before halting. Another thing to try to get in the test case.

robhagemans commented 4 years ago

Thanks for extracting test cases! For what it's worth I suspect the minimal case has two dots, as with a single dot the flood fill is more straightforward and just runs down from the top to the bottom of the circle, whereas with two dots there is a section in the middle that needs to be revisited separately, and the fill flows around and never stops. I'm sure there's some topological insight there but anyway.

Also, to your point, it's totally possible that this works differently on the Tandy than on PC GW-BASIC, they're slightly different code bases especially for graphics and sound. I may have to get the Tandy emulator running again to try.

mrclay commented 4 years ago

Updated with a hopefully more helpful test case.

robhagemans commented 4 years ago

OK, fixed now on the 2.0 branch. As expected, this was a bit involved - turns out it's an exceptional case of an exceptional case. Flood-filling with a pattern background should break off if a row is encountered that matches the fill pattern, except if that row is solidly attribute 0 (the background). However, if the fill itself is a solid background-attribute fill, this should not apply.

robhagemans commented 4 years ago

Fixed on 2.0 by commit 01ca799. Adjusting title to reflect that bug remains on 1.2 branch for now.

mrclay commented 4 years ago

Nice! I'll test soon.

robhagemans commented 2 years ago

Fix implemented on release-1.2 branch by commit d6e7dbe7