tmewett / BrogueCE

Brogue: Community Edition - a community-lead fork of the much-loved minimalist roguelike game
https://sites.google.com/site/broguegame/
GNU Affero General Public License v3.0
1.03k stars 109 forks source link

Refactor screen save&restore logic #646

Closed Nathan-Fenner closed 10 months ago

Nathan-Fenner commented 10 months ago

This PR is only refactoring; it should not cause any visible changes.


A common pattern in the Brogue codebase is to do the following:

screenDisplayBuffer dbuf;
screenDisplayBuffer rbuf;

clearDisplayBuffer(&dbuf);
fillBufferWithSomeKindOfGraphics(&dbuf); // some arbitrary logic

overlayDisplayBuffer(&dbuf, &rbuf);

// ... wait for events or something ...

overlayDisplayBuffer(&rbuf, NULL); // put the screen back

The second argument to overlayDisplayBuffer is used to store the old screen, so that drawn graphics can be reverted later. This is convenient, but it makes dbuf and rbuf usage look very similar, since they're both just screenDisplayBuffers.

We can make the distinction clearer by introducing new functions:

// A `SavedDisplayBuffer` holds a previous version of the screen. It can be
// Obtain one by calling `saveDisplayBuffer()` and restore it to the screen
// by calling `restoreDisplayBuffer()`.
typedef struct SavedDisplayBuffer {
  screenDisplayBuffer savedScreen;
} SavedDisplayBuffer;
SavedDisplayBuffer saveDisplayBuffer(void);
void restoreDisplayBuffer(const SavedDisplayBuffer *savedBuf);

// Now, `overlayDisplayBuffer` does not have a second reset pattern:
void overlayDisplayBuffer(const screenDisplayBuffer *overBuf);

The common pattern at the top can now be spelled as:

const SavedDisplayBuffer rbuf = saveDisplayBuffer();

screenDisplayBuffer dbuf;
clearDisplayBuffer(&dbuf);
fillBufferWithSomeKindOfGraphics(&dbuf); // some arbitrary logic

overlayDisplayBuffer(&dbuf);

// ... wait for events or something ...

restoreDisplayBuffer(&rbuf); // put the screen back

in particular, with a few exceptions, saveDisplayBuffer() is now always called in the same function that calls restoreDisplayBuffer, on the same variable. This makes it relatively easy to ensure that the screen is always getting restored when we mean it to, and less code-tracing across multiple functions is needed.


From this change, the number of calls of overlayDisplayBuffer drops from 60 to 23. So we see that actually many of its calls were solely for saving/restoring the state of the screen, mixed in with calls that actually draw new graphics to the screen.

tmewett commented 10 months ago

I don't have much of a horse in this race but the extra newtype for a saved screen feels not so necessary - but easy to revert if it becomes an obstacle. Approved 👍