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

Don't use refs for unmanaged bitmaps #84

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Nycto
Copy link
Collaborator

@Nycto Nycto commented Oct 19, 2024

This reduces the number of small memory allocations that happens when working with bitmaps. Bitmaps are split into two categories: managed and unmanaged. Unmanaged bitmaps are passed around by copying pointers, managed bitmaps are passed around using nim object refs.

Copy link
Collaborator

@ninovanhooff ninovanhooff left a comment

Choose a reason for hiding this comment

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

Hey thanks, seems this might improve both readability and performance somewhat.

I think I did find a pre-existing bug tho for a bitmap that should be managed

assert(this != nil)

if this.isNil:
assert(not this.isNil)
Copy link
Collaborator

@ninovanhooff ninovanhooff Oct 25, 2024

Choose a reason for hiding this comment

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

Are you forcing a crash here to point debuggers to the cause? That could be a good idea, just wondering

@@ -390,7 +420,7 @@ proc setBitmapMask*(

proc getBitmapMask*(this: LCDBitmap): LCDBitmap =
privateAccess(PlaydateGraphics)
return LCDBitmap(resource: playdate.graphics.getBitmapMask(this.resource), free: false) # Who should manage this memory? Not clear. Not auto-managed right now.
return newUnmanagedBitmap(playdate.graphics.getBitmapMask(this.resource))
Copy link
Collaborator

Choose a reason for hiding this comment

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

LCDBitmap* playdate->graphics->getBitmapMask(LCDBitmap* bitmap);
Gets a mask image for the given bitmap, or returns NULL if the bitmap doesn’t have a mask layer. The returned image points to bitmap's data, so drawing into the mask image affects the source bitmap directly. The caller takes ownership of the returned LCDBitmap and is responsible for freeing it when it’s no longer in use.

I'd say this means the returned bitmap is managed


proc `=copy`(a: var LCDBitmapObj, b: LCDBitmapObj) {.error.}

proc newUnmanagedBitmap(point: LCDBitmapPtr): auto =
Copy link
Collaborator

@ninovanhooff ninovanhooff Oct 25, 2024

Choose a reason for hiding this comment

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

Can you rename this to bitmapPointer. Very strong associations with a 2d point here.

Same for newManagedBitmap

@@ -93,8 +93,8 @@ proc handler(event: PDSystemEvent, keycode: uint) {.raises: [].} =
font = try: playdate.graphics.newFont(FONT_PATH) except: nil
Copy link
Collaborator

@ninovanhooff ninovanhooff Oct 25, 2024

Choose a reason for hiding this comment

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

(not related to this file specifically)

Although we are not changing the API here, I'd still recommend targeting the dev branch for this PR so that we can track any new bugs to a specific release and have a sense of what is in the public release and what isn't

Copy link
Collaborator

Choose a reason for hiding this comment

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

image You can change it by pressing the edit button next to the PR title and then using the dropdown

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.

2 participants