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

Export BitmapData.read #25

Closed
wants to merge 1 commit into from
Closed

Conversation

ninovanhooff
Copy link
Collaborator

@ninovanhooff ninovanhooff commented Nov 19, 2023

LCDBitmap.get(x,y) is very slow to sample pixel data, presumably because it extracts the bitmap data on every invocation.

When caching the BitmapData with LCDBitmap.getData and then invoking read on the BitmapData, we can sample pixel data at 93x the speed of LCDBitmap.get(x,y)

benchmark, invocations per 100 ms. Higher is better:

LCDBitmap.get(0,0) - 513
BitmapData.read(0,0) - 47716

LCDBitmap.get(x,y) is very slow to sample pixel data, presumably because it extracts the bitmap data on every invocation.

When caching the BitmapData with LCDBitmap.getData and then invoking read on the BitmapData, we can sample pixel data at 19x the speed of  LCDBitmap.get(x,y)

benchmark, invocations per 100 ms. Higher is better:

LCDBitmap.get(0,0) - Slow 188637
 BitmapData.read(0,0) - Fast 3570158
@samdze
Copy link
Owner

samdze commented Nov 19, 2023

Right, a way to get/set pixel data in a more performant manner would be welcome.

The read template is meant to be private, but only because there's BitmapView (type BitmapView* = DisplayFrame | BitmapData) that implements all the methods valid for both DisplayFrame and BitmapData.

So, an efficient (and unsafe? Right now get and set/clear do many safety checks) way to access pixel data should be added to BitmapView!

Maybe @Nycto has been doing something similar to optimize performance on his game?

@Nycto
Copy link
Collaborator

Nycto commented Nov 19, 2023

Ah, yes! I actually have a function for bulk setting pixels that is much faster. It’s not super optimized, but it’s a lot faster than manually setting each pixel.

I’ll submit a PR

@Nycto
Copy link
Collaborator

Nycto commented Nov 19, 2023

In terms of this specific CR, exposing read has two downsides:

  1. it doesn’t do bounds checking. My subjective opinion is that it should be done as an assert so it disappears in release mode.
  2. read returns the whole byte, so the users of still need to do their own twiddling to extract the actual coordinate they are looking for. This should be the job of the library, imo

@Nycto
Copy link
Collaborator

Nycto commented Nov 19, 2023

A PR with the faster bulk pixel setting code: #26

@ninovanhooff
Copy link
Collaborator Author

For reading, I just discovered that get(x,y) can also be used, because BitmapData is a BitmapView and proc get*(view: BitmapView, x, y: int): LCDSolidColor

It seems plenty fast, so I guess that existing function covers the reading scenario?

@ninovanhooff ninovanhooff deleted the patch-1 branch November 19, 2023 22:08
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