LOADIFF doesn't store palette data in screen memory correctly, fails screen switching

[dansanderson]

Test Environment (required)
You can use MEGA65INFO to retrieve this.

• Platform: MEGA65, xemu
• ROM Release: 920409

Describe the bug
The LOADIFF command loads image data from a file to the currently active draw screen in the bitplane graphics system. It also sets the palette based on the IFF-ILBM file's palette data.

The screen system allows for two or more screens to be open simultaneously, and for one screen to be visible while another non-visible screen is the active draw screen, to support hiding slow draw operations such as LOADIFF. It has been reported that if the draw screen is not also the view screen, LOADIFF does not update the palette correctly.

LOADIFF should always update the palette of the active draw screen. We need to confirm whether the current implementation always updates the palette of a specific screen (e.g. screen 0), always updates the palette of the visible screen (vs. the draw screen), or fails to update the palette under some circumstances.

To Reproduce

10 SCREEN 1,0,0,8: SCREEN 0,0,0,8
20 SCREEN SET 1,0
30 LOADIFF"xxx"
40 SCREEN SET 0,1
50 GETKEY A$
60 SCREEN CLOSE 0: SCREEN CLOSE 1

Expected behavior
In this example, LOADIFF draws to screen 1 and sets its palette, and keeps its slow draw operation invisible (because screen 0 is visible). When loading is complete, the program switches the visible screen to screen 1, displaying the loaded image data with the correct palette instantly.

[dansanderson]

This appears to be a more fundamental issue with LOADIFF either not storing the screen palette data correctly, or SCREEN SET not installing it correctly.

A LOADIFF directly onto the visible screen works because there's extra logic in kg65.setpalette to update the visible palette with the loaded CMAP data during the load (so the palette change happens at the beginning and not the end), as well as loading it into the screen palette data with kg65.s.write.palette. If you subsequently call SCREEN SET 0,0, it attempts to re-install the visible palette from the screen palette data, and ends up showing the same wrong palette as the above example. So either kg65.s.write.palette is failing to store the correct palette data during LOADIFF, or kg65.screenset is failing to install the correct palette data.

So far I've confirmed that the correct screen IDs are being passed around. I have to dig further into how palette data is handled.

[dansanderson]

I'm messing with writing to two different screen palettes and noticing that multi-screen palette handling is wonky in other ways.

100 SCREEN 1,320,200,8:SCREEN 0,320,200,8:SCREEN SET 0,0
101 FOR X=0 TO 255
102 PALETTE 0,X,X AND 15,0,0
103 PALETTE 1,X,0,X AND 15 ,0
104 NEXT X
110 SCREEN SET 0,0
111 PEN 15
112 DOT 100,100
113 GETKEY K$
114 SCREEN SET 1,1
115 DOT 150,100
116 GETKEY K$
180 SCREEN CLOSE 0:SCREEN CLOSE 1

This correctly declares two screens with two switchable draw areas. But lines 102 and 103 should be setting screen 0's palette to all reds and screen 1's palette to all greens, and the actual result is both screens use the all-green palette.

When I add a LOADIFF back in with the view and draw screen set to the same screen, it sets the visible palette correctly and loads in the image. When I switch to the other screen, it has a new incorrect palette nothing like how I initialized it, presumably derived from the LOADIFF (but not matching the IFF's actual palette). When I switch back to the first screen, it also has the incorrect palette.

The palette management code is complicated and will take some work to understand. I'll keep leaving partial findings here. 😅

[dansanderson]

This broke somewhere between ROM 920255 (working) and 920264 (broken), November 2021. Relevant commit range:

• Earliest (changes from "920254" to "920256"): https://github.com/MEGA65/mega65-rom/commit/8c71df55d99757f175d37d8affb926ad4dbe9c68
• Latest (changes from "920263" to "920264"): https://github.com/MEGA65/mega65-rom/commit/30e6d3171492ce400ca79fc09a446900503df63d
• Compare for this range: https://github.com/MEGA65/mega65-rom/compare/8c71df5..30e6d31

There are graphics subsystem changes in here but nothing that would obviously affect palette handling. Still looking.

[dansanderson]

It looks like I can't use the repo history to find the change that broke this because at this time in ROM development history, all of the Graphics subsystem was in a file named graphics.src that was not committed to the repo. Some Graphics subsystem refactoring happened in this commit range, but only the b65.src side of the changes are known to the repo. The graphics.src change history was never recorded. 🫠

I'm currently stepping through the latest version to see how it's handling palettes. I'm making progress but it's slow going because the data structure is complicated. It's not surprising that a refactoring attempt broke it, but without the before-and-after, I just have to review the whole thing.

[dansanderson]

This is now fixed.

This was actually a much larger problem with palette storage, not just with LOADIFF. All attempts to read from or write to a stored Screen palette miscalculated the palette storage address, resulting in all Screens using the same palette storage area, and only storing/reading the first 128 palette entries. PALETTE would update the same memory regardless of the screen number, and switching screens just updated the VIC palette registers with the same first 128 entries.

This was difficult to notice because any changes to the visible screen's palette also take place immediately in the VIC registers, so if you never switch viewscreens you never notice that it's broken.

I'm very glad this is fixed. The multi-screen feature is one of the Screen system's coolest features, and it's been broken in this way for years.