Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor screen save&restore logic #646

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

Nathan-Fenner
Copy link
Contributor

@Nathan-Fenner Nathan-Fenner commented Jan 6, 2024

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.

Copy link
Collaborator

@zenzombie zenzombie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Fired up a game and played for a bit. Worked as expected.

@tmewett
Copy link
Owner

tmewett commented Jan 12, 2024

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 👍

@zenzombie zenzombie merged commit 6c310a5 into tmewett:release Jan 12, 2024
1 check passed
@Nathan-Fenner Nathan-Fenner mentioned this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants