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

Vectorio color index #6176

Merged
merged 5 commits into from
Mar 21, 2022
Merged

Conversation

FoamyGuy
Copy link
Collaborator

@FoamyGuy FoamyGuy commented Mar 19, 2022

Implementing color_index argument and property for all of the vectorio shapes. This allows user to specify which color index to fill the shape from the given palette. Previous behavior was always defaulting to the 0 index within the palette.

There is a script illustrating it's usage in this PR inside of tests/vecotrio/color_index.py

If the argument is omitted the previous behavior is the default.

kvc0
kvc0 previously approved these changes Mar 20, 2022
Copy link

@kvc0 kvc0 left a comment

Choose a reason for hiding this comment

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

This looks right to me

import vectorio


def increment_color(shape):
Copy link

Choose a reason for hiding this comment

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

Oh wow this is a fun capability

shared-bindings/vectorio/Polygon.c Show resolved Hide resolved
shared-bindings/vectorio/Circle.c Outdated Show resolved Hide resolved
@gamblor21
Copy link
Member

I didn't test but looked over the code. One question what if I set a color_index outside the range of the palette? I am just unsure if the palette automatically handles and wraps that or if you need a check for the size of the palette vs the color_index.

@FoamyGuy
Copy link
Collaborator Author

@gamblor21 good question on the outside range. There isn't automatic wrapping for values outside the range, however they also don't result in an exception. Current behavior is values outside of the valid range result in the shape being drawn black which is maybe unexpected behavior.

I'll add some more explicit checks that raise an exception for invalid values.

Thanks for having a look over this!

@kvc0
Copy link

kvc0 commented Mar 20, 2022

On valid ranges for color values - it’s really up to the colorconverter/pixelshader/palette to decide what to do with an index out of range, no? I agree with just passing the configured color index straight into the pixel shader as you have it here. Why define a different behavior for invalid indices than the object to which those indices matter?

@FoamyGuy
Copy link
Collaborator Author

That does make sense to me, it would be better to enforce the range within Palette which should then carry over to the usage here if I understand correctly.

Copy link
Member

@gamblor21 gamblor21 left a comment

Choose a reason for hiding this comment

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

I did not test but it looks good to me now.

@gamblor21
Copy link
Member

That does make sense to me, it would be better to enforce the range within Palette which should then carry over to the usage here if I understand correctly.

Makes sense to me. The original was more of a question just to ensure it was thought of (and possibly a long term issue for Palette if it does not check ranges).

@gamblor21 gamblor21 merged commit 8dae57a into adafruit:main Mar 21, 2022
@kvc0
Copy link

kvc0 commented Mar 21, 2022

Thanks folks!

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