Categories
Week 6

Return to Sender: Fixing Gus’s Post Office

This week was about closing out a drag-and-drop bug that had been bothering me, building test movies in real Director 4, and finally facing the AddressSanitizer. Let me walk through what happened.

The Post Office Bug

The centerpiece of the week. In Gus Goes to Cyberopolis, the post office letter minigame was broken: dragging a stamp onto the letter’s slot made it snap back to its tray every time, making the minigame unwinnable.

Some quick background for this one. A Director movie is composed of sprites, visual objects placed on numbered channels, and each sprite can have a script attached that reacts to events like mouseDown and mouseUp. Lingo (Director’s scripting language) also has a property called the clickOn, which returns the channel number of the sprite the user last clicked.

The game’s logic is simple: the stamp’s script handles mouseDown and starts the drag, and the slot’s script handles mouseUp and places the stamp. On release, the slot’s script asks the clickOn which channel was involved and checks whether it holds an empty slot. So for a drop to work, two things must happen when the mouse is released: the mouseUp event must be delivered to the sprite under the mouse (the slot, not the stamp), and the clickOn must return the slot’s channel.

Neither was happening in ScummVM. For Director 4 movies, mouseUp was being delivered using the sprite remembered from the original mouseDown, so the stamp’s script received the mouseUp. The stamp has no mouseUp handler, so the event fell through to the frame script, whose fallback logic snaps the stamp back to the tray. And the clickOn still pointed at the stamp’s channel from the original click.

Understanding this took a detour through Director’s message hierarchy, events are offered to the sprite’s script first, then the cast script, the frame script, and finally the movie script, with each level able to stop or pass the event along. Reading the game’s four scripts against a debugger session made it click.

The fix: deliver mouseUp to the sprite currently under the mouse for Director 4 movies, and update the clickOn when mouseUp lands on a sprite, so drop-target scripts can identify the target channel. Stamps now stick to letters.

NOTE: the fix is not final yet. Further discussion with sev will decide its final shape.

Building the Regression Test

Sev’s condition for the fix was tests, so I made some test movies in Director 4. The test suite in the director-tests repo uses a plugin (an “XObject” in Director terms) that can inject input events, move the mouse, press and release the button, and assert on the order in which handlers run, using a global counter.

The new test places two sprites on stage, presses the mouse on sprite 1, moves to sprite 2, and releases. Sprite 1’s script asserts it received the mouseDown; sprite 2’s asserts it received the mouseUp. Without the fix, sprite 2’s handler never fires and the assert count comes up short; with the fix, everything passes.

My first version attached the handlers to the cast members instead of the sprites, and the test failed for the wrong reasons, the handlers need to be sprite scripts to exercise the exact dispatch path the fix touches. I also added a screenshot call to last week’s text wrapping test, so that fix now has visual regression coverage against a reference image captured from real Director 4.

Film Loop Position Shift

A film loop is a Director cast member that packages a small animation so it can be placed on a channel like any static image. In the Kooky Carnival’s shooting gallery, animal sprites jumped to the wrong position when clicked.

When a script swaps a sprite’s image for another cast member at runtime, the new member may use a different registration point, the anchor that decides how the image is positioned. Normal bitmaps anchor at the top-left; film loops anchor at their center. ScummVM wasn’t adjusting the sprite’s position for that difference, so the sprite visually shifted by half its size on swap.

The fix saves the sprite’s on-screen bounding box before the swap and adjusts its position afterward, so it stays put. I had pushed a similar solution a long while ago, but it was in the wrong place. This one works.

What not to do

I had some uncommitted changes sitting on the buildbot server, which broke the imagediff tooling.

A refactor by sev had moved code from imagediff.py into main.py, and two things were left broken. First, main.py imported its config before running the sys.path bootstrap, so the dashboard could not load from a fresh start at all (ModuleNotFoundError: config). Second, screenshot_diff.py still imported from the old imagediff/imagediff.py module that no longer existed, so every ScreenshotDiffStep failed.

The fix moved the bootstrap before the config import, pointed the diff step at the new module, and cleaned out leftover debug prints and dead code. Merged.

Fortunately the uncommitted changes on the server were trivial and I was able to revert them.

Lesson learnt: do not leave unfinished work on prod servers (-_-) (use git, github).

My First Use-After-Free

While testing one of the Cyberopolis movies, ScummVM crashed when switching movies, but only in my AddressSanitizer build. This was my first time actively reading an ASAN report. As I was looking into this, I asked sev and after looking at the backtrace he found 2 commits which were the likely cuplrits.

The older commit, related to some changes in movie cast members, was the culprit. As this is a difficult bug, this will be taken care of by sev.

The visual debugger

Some DT changes have been made locally, but will finalize them and push the commits soon. Mostly bugs and crashes, nothing very interesting. And the DT blog is almost ready. Wasn’t able to work on it for a few days. Will publish in a day or two.

Next up:
– work on an animation speed bug
– continue on gus bugs

PRs this week:
Fix mouseUp dispatching to wrong sprite in D4 (not merged, in discussion)
Add D4 test movies for mouseUp dispatch and text wrapping (director-tests)
Fix filmloop position shift when swapping cast member via Lingo
ImageDiff cleanup