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 intersects() #9483

Closed
wants to merge 19 commits into from
Closed

Conversation

FoamyGuy
Copy link
Collaborator

@FoamyGuy FoamyGuy commented Aug 6, 2024

This change is an initial working implementation of collision detection for vectorio. Specifically it supports Circle and Rectangle only right now, and currently the is_colliding() function has been added to Rectangle only but it accepts either another Rectangle or a Circle to test against. But my intention would be to also this function to the Circle as well so that the test can be done either way.

I wanted to open this up to get feedback before I went further adding it to Circle though, mainly on the following points:

  • Do we want to have this functionality in the core? It can be done in python too, but my hope is that moving it to the core will speed it up for cases where a program wants to test collision between a large number of objects. I have not done specific bench-marking or comparison yet.
  • Is there a more preferred name than is_colliding()? That was the first name that came to mind for me when I started writing it, but now that it's implemented I am thinking that overlaps() might be a better function name for it because I believe the implementation is technically testing for at least 1 pixel of overlap, not just two adjacent pixels "colliding".
  • Is it okay to start with implementations for just Circle and Rectangle, or would there be a desire to have an implementation Polygon as well before considering merging?
  • Would we want to support other types of arguments for other_shape? Theoretically it should be able to test against Tilegrid also to allow for testing images against vectorio shapes.

I tested this on a Pygamer with this code:

import time

import board
import displayio
import vectorio
from game_controls import game_controls

time.sleep(5)

main_group = displayio.Group()

palette = displayio.Palette(3)
palette[0] = 0x125690
palette[1] = 0xbb4444
palette[2] = 0x44bb44

static_rect = vectorio.Rectangle(pixel_shader=palette, width=40, height=30, x=55, y=45)
main_group.append(static_rect)

# moving_rect = vectorio.Rectangle(pixel_shader=palette, width=10, height=10, x=0, y=0)
# moving_rect.color_index = 1
# main_group.append(moving_rect)

circle = vectorio.Circle(pixel_shader=palette, radius=7, x=15, y=15)
circle.color_index = 1
main_group.append(circle)


board.DISPLAY.root_group = main_group
while True:
    cur_state = game_controls.button

    if (cur_state['up']):
        #moving_rect.y -= 1
        circle.y -= 1

    if (cur_state['down']):
        #moving_rect.y += 1
        circle.y += 1


    if (cur_state['right']):
        #moving_rect.x += 1
        circle.x += 1

    if (cur_state['left']):
        #moving_rect.x -= 1
        circle.x -= 1

    # if moving_rect.is_colliding(static_rectangle):
    #     moving_rect.color_index = 2
    # else:
    #     moving_rect.color_index = 1

    if static_rect.is_colliding(circle):
        circle.color_index = 2
    else:
        circle.color_index = 1

    time.sleep(0.01)

@dhalbert
Copy link
Collaborator

dhalbert commented Aug 6, 2024

I do prefer overlaps, and maybe intersects is even more common. is_colliding implies motion, and that's not necessarily true.

@FoamyGuy FoamyGuy changed the title Vectorio is_colliding() Vectorio intersects() Aug 7, 2024
@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Aug 7, 2024

the latest commits have renamed the function to intersects() and disabled the vectorio module on the mini_sam_m4 which overflowed it's size with these changes.

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.

Changed intersecting to intersects: I think that's what you wanted?

shared-bindings/vectorio/Rectangle.c Outdated Show resolved Hide resolved
shared-bindings/vectorio/Rectangle.c Outdated Show resolved Hide resolved
shared-bindings/vectorio/Rectangle.h Outdated Show resolved Hide resolved
shared-module/vectorio/Rectangle.c Outdated Show resolved Hide resolved
shared-bindings/vectorio/Circle.c Outdated Show resolved Hide resolved
@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Aug 7, 2024

Yes, thank you! The recent commits have changed everything over to intersects.

I've also added the function implementation on the Circle class now. I performed minimal testing only so far, with an updated version of the script above to switch around which shapes are tested, and tested against.

@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Aug 7, 2024

The addition of intersects() function on Circle class pushed a few other devices over the storage limit. The latest commit disables vectorio on all of the ones that failed during the last action run.

It appears the docs build failed too, but it may have been an intermittent issue with the network in the container, or the server that hosts the svg file that dogs build attempts to fetch. I found these warnings in the action log:

WARNING: Could not fetch remote image: https://github.com/adafruit/circuitpython/workflows/Build%20CI/badge.svg [HTTPSConnectionPool(host='github.com', port=443): Max retries exceeded with url: /adafruit/circuitpython/workflows/Build%20CI/badge.svg (Caused by ConnectTimeoutError(<urllib3.connection.HTTPSConnection object at 0x7f081e2897c0>, 'Connection to github.com timed out. (connect timeout=None)'))]

@dhalbert
Copy link
Collaborator

dhalbert commented Aug 7, 2024

The addition of intersects() function on Circle class pushed a few other devices over the storage limit. The latest commit disables vectorio on all of the ones that failed during the last action run.

These failures are not due to overflow, I believe, but instead are multiple GitHub actions network issues. See https://github.com/adafruit/circuitpython/actions/runs/10277146885. I think you should revert f11cd0c and try again.

@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Aug 7, 2024

I'm curious about how to tell the difference between those types of actions failures? I noticed after the latest commit reverting those disables it seems to have skipped building the ports so it didn't re-attempt any of the builds from last time that didn't succeed.

In the actions log from the prior attempt where failures were reported, I found a few of them with errors like this: https://github.com/adafruit/circuitpython/actions/runs/10277146885/job/28438883955#step:10:1361 and 20 lines down from here at line 1381 it shows the negative space readout:

1441808 bytes used,     -16 bytes free in flash firmware space out of 1441792 bytes (1408.0kB).

Are those builds failing like a knock-on effect that occurred down the line afterward, but due to the network issue?

@dhalbert
Copy link
Collaborator

dhalbert commented Aug 7, 2024

It is a mix of actual storage overflows and transient CI failures. The C3 failures look like actual overflow. The nordic failures are not. I was able to see the transient failures easily in the list at the link I gave above: https://github.com/adafruit/circuitpython/actions/runs/10277146885. Some of these even succeeded after retries, but the transient errors still show up in that log.

It's worth considering whether to turn something else off to make things fit on the C3 builds, especially those with displays. For instance, the OHS2020 is a badge with a display, so I'd prioritize vectorio over something else on that board.

@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Aug 7, 2024

Ahhh I see the logs for the transient ones now. Thank you. I was confused and only seeing a few of them before.

And that does make sense to me as well to leave vectorio enabled for devices that have a built-in display.

@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Aug 9, 2024

Maybe it would make sense to move this out of vectorio, or at least out of the Rectangle and Circle classes. If they were separated from those it would mean we could have a single something.cirlce_rectangle_intersects(rect_x, rect_y, rect_w, rect_h, circle_x, circle_y, circle_r) or similar. It wouldn't accept vectorio.Rectangle or Circle directly, but instead just the relevant coordinates and sizes. That way user code can use it with a variety of different objects rather than just Rectangle and Circle specifically.

With the current implementation on Rectangle and Circle we basically end up having mostly duplicated copies of these functions because there is one circle compared to rectange, and one rectangle compared to circle. If Polygon were to end up getting similar functionality it would mean then essentially 3 copies of each function if all permutations were supported.

I'm not sure where would be the best place to move it to if it does get moved. Maybe still inside vectorio, but just as module functions instead of functions on each class would make sense?

@FoamyGuy FoamyGuy mentioned this pull request Aug 12, 2024
@FoamyGuy FoamyGuy marked this pull request as draft August 30, 2024 23:01
@FoamyGuy
Copy link
Collaborator Author

Converting to draft for now. The latest commit has started refactoring these to be module functions instead of class functions on Circle and Rectangle, that will ultimately lead to de-duplication of this logic.

As of now only the new implementations are added, the old class functions are not removed yet. Once I do remove them and any other requisite changes I'll mark this ready.

@FoamyGuy
Copy link
Collaborator Author

closing this in favor or #9753

@FoamyGuy FoamyGuy closed this Oct 23, 2024
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