Skip to content

OnDiskBitmap palette getter/setters #4438

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

Closed
wants to merge 2 commits into from

Conversation

kmatch98
Copy link

@kmatch98 kmatch98 commented Mar 19, 2021

This is an addition to add color palette getters/setters for the OnDiskBitmap. This will allow for palete animations/color cycling for bitmaps without having to load them into RAM.

Also, this adds a flag that provides an option to force refresh of an OnDiskBitmap whenever the palette is modified.


This work was spurred out of an exploration of animated icons. @FoamyGuy demonstrated that using OnDiskBitmap allowed loading of significantly more images for the "TouchDeck" project, however currently there is no way of adjusting the color palette. This addition allows modifications of an OnDiskBitmap's color palette to achieve these types of color cycling animations.

This capability probably has a relative small set of use cases, so if feedback is that this should be put on hold, I'm perfectly fine to park this one, and it will reside here for posterity.


This allows OnDiskBitmap to do these kinds of palette animations:
Adafruit_logo  - OnDiskBitmap palette cycling

@dhalbert dhalbert requested a review from tannewt March 27, 2021 03:01
@tannewt tannewt added this to the Long term milestone Mar 29, 2021
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Could we just have a Palette object available as .palette instead? That would allow for transparency and such.

In fact, maybe we should switch to returning the pixel index here so that it's truly a value. Then TileGrid could map that to a color with its pixel_shader (like odb.palette).

@@ -3950,6 +3966,10 @@ msgstr ""
msgid "syntax error in uctypes descriptor"
msgstr ""

#: shared-module/displayio/OnDiskBitmap.c
msgid "this OnDiskBitmap is raw color, not indexed"
Copy link
Member

Choose a reason for hiding this comment

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

Unify this with the similar message above.

@kmatch98
Copy link
Author

Could we just have a Palette object available as .palette instead? That would allow for transparency and such.

In fact, maybe we should switch to returning the pixel index here so that it's truly a value. Then TileGrid could map that to a color with its pixel_shader (like odb.palette).

To be perfectly honest I couldn’t decipher the syntax to make something act as a .palette getter and setter property versus just a plain function. Can you point to a good example? This may be moot if we redefine this to be a palette object.

As for whether it should be redefined as a normal palette object, I’m not sure I understand how this OnDiskBitmap works with ColorConverter, so I’m unsure if making it into a palette object will cause other issues. But I speculate that this OnDiskBitmap was designed primarily for non-color-indexed bitmaps, so a palette is only warranted for the indexed flavor. Maybe it should only create a palette for indexed bitmaps, but then ColorConverter will have to be modified to take a Palette object.

Your suggestions and clarifications are welcome.

@tannewt
Copy link
Member

tannewt commented Mar 30, 2021

To be perfectly honest I couldn’t decipher the syntax to make something act as a .palette getter and setter property versus just a plain function. Can you point to a good example? This may be moot if we redefine this to be a palette object.

Sure! The key thing is that there is an intermediate "property" object that stores pointers to the getter and setter functions. See: https://github.com/adafruit/circuitpython/blob/main/shared-bindings/digitalio/DigitalInOut.c#L224 (This is how properties work internally.)

As for whether it should be redefined as a normal palette object, I’m not sure I understand how this OnDiskBitmap works with ColorConverter, so I’m unsure if making it into a palette object will cause other issues. But I speculate that this OnDiskBitmap was designed primarily for non-color-indexed bitmaps, so a palette is only warranted for the indexed flavor.

Yup, I think palette can be None in the non-indexed case.

Maybe it should only create a palette for indexed bitmaps, but then ColorConverter will have to be modified to take a Palette object.

You don't need ColorConverter if you have a Palette. ColorConverter is just there to change 24 bit "values" (aka colors) into 16 bit colors. Palette does this conversion already internally for its 24 bit colors.

The main change I think is: https://github.com/adafruit/circuitpython/blob/main/shared-module/displayio/OnDiskBitmap.c#L159 It should return the index and then the external palette can map it.

@jposada202020
Copy link

Interesting

@kmatch98
Copy link
Author

@jepler I think your work will supercede or obsolete this PR. I'll close this one for now and await to see if your additions also provide this capability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants