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

Adding circle bitmaptools #7782

Merged
merged 13 commits into from
Apr 1, 2023
Merged

Adding circle bitmaptools #7782

merged 13 commits into from
Apr 1, 2023

Conversation

jposada202020
Copy link

@jposada202020 jposada202020 commented Mar 22, 2023

Sorry about the previous PR, badly done on my part.

@tannewt tannewt requested a review from FoamyGuy March 23, 2023 17:05
shared-module/bitmaptools/__init__.c Outdated Show resolved Hide resolved
shared-module/bitmaptools/__init__.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I tested this successfully on Feather ESP32-S2 TFT. I did have to merge main into this branch in order to get a build that worked (prior to merging main I got successful builds, but device bootlooped without running code.py or printing any error messages. Seems like some other commit in main fixed that.)

Current functionality looks good to me.

I think it's worth considering tweaking the name of the x/y point arguments to not have a numberin them, but beyond that this seems good to me.

shared-bindings/bitmaptools/__init__.c Outdated Show resolved Hide resolved
shared-module/bitmaptools/__init__.c Outdated Show resolved Hide resolved
@jposada202020
Copy link
Author

Thank you @gamblor21 and @FoamyGuy for the reviews ... probably ill will do the changes later the week, need some headspace to work in C 🍼

@dhalbert dhalbert requested review from FoamyGuy and gamblor21 March 30, 2023 13:27
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Finish renaming x0 and y0 to x and y.

shared-bindings/bitmaptools/__init__.c Outdated Show resolved Hide resolved
shared-bindings/bitmaptools/__init__.h Outdated Show resolved Hide resolved
shared-module/bitmaptools/__init__.c Outdated Show resolved Hide resolved
shared-module/bitmaptools/__init__.c Outdated Show resolved Hide resolved
jposada202020 and others added 4 commits March 30, 2023 15:28
Co-authored-by: Dan Halbert <halbert@halwitz.org>
Co-authored-by: Dan Halbert <halbert@halwitz.org>
Co-authored-by: Dan Halbert <halbert@halwitz.org>
Co-authored-by: Dan Halbert <halbert@halwitz.org>
@jposada202020
Copy link
Author

Thank you very much @dhalbert, Thank you for taking the time to put the explanations. It helps a lot!

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Could you retest a build to make sure it still works? I did not re-test, I just renamed, and if I made a typo it might have stopped working. Thanks.

shared-bindings/bitmaptools/__init__.c Outdated Show resolved Hide resolved
shared-module/bitmaptools/__init__.c Outdated Show resolved Hide resolved
@jposada202020
Copy link
Author

Yes, will do.

jposada202020 and others added 2 commits March 30, 2023 15:43
Co-authored-by: Dan Halbert <halbert@halwitz.org>
Co-authored-by: Dan Halbert <halbert@halwitz.org>
@dhalbert
Copy link
Collaborator

@jposada202020 You can collect all the suggestions into a single batch by clicking "Add suggestion to batch". Then after you do all the ones you want in a single commit, you can click the appropriate button again to apply them all at once.

image

@jposada202020
Copy link
Author

Thanks @dhalbert . Sorry 👍🏼

@jposada202020
Copy link
Author

@dhalbert It builds no problem, but the now is not drawing a circle, need to troubleshoot why the change in behaviour. Going to put it draft and troubleshoot tonight. Thanks

@jposada202020 jposada202020 marked this pull request as draft March 30, 2023 19:58
@dhalbert
Copy link
Collaborator

dhalbert commented Mar 30, 2023

My x0 -> x and y0 -> y changes might not have been completely consistent.

@jposada202020 jposada202020 marked this pull request as ready for review March 30, 2023 22:09
@jposada202020
Copy link
Author

jposada202020 commented Mar 30, 2023

Test Code

import time
import displayio
import board
import bitmaptools


display = board.DISPLAY

palette = displayio.Palette(4)
palette.make_transparent(0)
palette[1] = 0xFFFFFF
palette[2] = 0x440044
bitmap = displayio.Bitmap(display.width, display.height, 3)
bg = displayio.TileGrid(bitmap, pixel_shader=palette)

group = displayio.Group()
group.append(bg)


radius = 50

for i in range(25, 480, 100):
    for j in [19, 119, 219, 319]:
        bitmaptools.draw_circle(bitmap, i, j, radius, 1)

display.show(group)

Result

image

@jposada202020 jposada202020 requested a review from dhalbert March 30, 2023 23:22
dhalbert
dhalbert previously approved these changes Mar 31, 2023
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

Copy link
Collaborator

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I re-tested the functionality and that part is still looking good to me.

I do think it might be best to use the mp_arg_validate functions instead of "manually" having if statements to validate the range. I've added seperate comments in the specific sections where that is applicable.

shared-bindings/bitmaptools/__init__.c Outdated Show resolved Hide resolved
shared-bindings/bitmaptools/__init__.c Outdated Show resolved Hide resolved
shared-bindings/bitmaptools/__init__.c Outdated Show resolved Hide resolved
@jposada202020
Copy link
Author

jposada202020 commented Apr 1, 2023

Changes in 2f3ea81

@jposada202020 jposada202020 requested a review from FoamyGuy April 1, 2023 01:03
Copy link
Collaborator

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for adding this functionality @jposada202020!

I tested the latest version successfully with Feather ESP32-S2 TFT and confirmed the bounds validation for x,y and radius. All appears good to me.

@dhalbert dhalbert merged commit 4a14e4b into adafruit:main Apr 1, 2023
@jposada202020 jposada202020 deleted the adding_circle_bitmaptools branch April 1, 2023 21:11
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.

4 participants