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

Add pygame.mouse.get_just_pressed/released() #2836

Merged

Conversation

damusss
Copy link
Member

@damusss damusss commented May 3, 2024

I added the functions in the title + stubs, tests and docs.
I also added 2 function to the C API and I documented them.
I looked at the same functions in key.c to work myself into implementing them for the mouse.
The suggestions comes from clear code on #contributing

If those functions exist for the keys, I think it makes perfect sense to implement them for the mouse as well.
Tell me what you think and if I messed up something on the C side.

@damusss damusss requested a review from a team as a code owner May 3, 2024 15:00
@damusss damusss closed this May 3, 2024
@damusss damusss reopened this May 3, 2024
@Starbuck5 Starbuck5 added New API This pull request may need extra debate as it adds a new class or function to pygame mouse pygame.mouse labels May 4, 2024
@MyreMylar
Copy link
Member

IIRC the only reason get_pressed() has the awkward num_buttons parameter is for backwards compatibility as originally there was only support for three buttons in pygame 1 so when five button support came along with SDL2 I added this parameter.

I would be tempted to make a new function just always return a 5-tuple of all the potential buttons. If a mouse doesn't have x1 or x2 then they would just always be false in the 5-tuple. Simpler API and maybe with the pygame 3/SDL 3 API break we could align .get_pressed() in the same way.

I also think, now I'm looking over it, the docs could be rejigged to list what the buttons are with terminology that matches the SDL & pygame constants (and common usage) e.g.:

get_pressed() -> (left_button, middle_button, right_button, x1_button, x2_button)

...

get_just_pressed() -> (left_button, middle_button, right_button, x1_button, x2_button)

because I know I'm constantly forgetting the order of the buttons.

@damusss
Copy link
Member Author

damusss commented May 6, 2024

@MyreMylar thanks for the review, and I agree on all. I used num_buttons when looking at the old function, but I can see that they aren't really necessary as this is a new one. I also almost always forget if the right button is the second or third entry in the tuple, so an update of the docs feels necessary. I'll take the freedom to change the docs of the normal get_pressed as well as it would make it clearer. On my way to update the pr :)

@damusss
Copy link
Member Author

damusss commented May 6, 2024

That change is not only better on a python level but reduced massively the test/stub sizes and the API sizes completely removing the logic for parsing arguments and returning different sized tuples.

docs/reST/ref/mouse.rst Outdated Show resolved Hide resolved
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

OK, this works as I would expect in my testing. I just stuck this in a standard pygame-ce game loop:

    if any(pygame.mouse.get_just_pressed()):
        print("pressed: ", pygame.mouse.get_just_pressed())

    if any(pygame.mouse.get_just_released()):
        print("released: ", pygame.mouse.get_just_released())

I expect some users will find it useful, I think we added the keyboard version because it was a similar interface as offered in some other popular game development libraries. I think that still stands as a reason to add this here, along with it being more consistent with the keyboard.

src_c/mouse.c Outdated

/* create the module */
return PyModule_Create(&_module);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

missing newline here

@@ -658,4 +664,4 @@ pg_tuple_couple_from_values_double(double val1, double val2)
PyTuple_SET_ITEM(tuple, 1, tmp);

return tuple;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

as well as here

src_c/event.c Outdated
@@ -2322,4 +2348,4 @@ MODINIT_DEFINE(event)

SDL_RegisterEvents(PG_NUMEVENTS - SDL_USEREVENT);
return module;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

and here

Comment on lines +172 to +177
if (!(tuple = PyTuple_New(5)))
return NULL;

for (int i = 0; i < 5; i++) {
PyTuple_SET_ITEM(tuple, i, PyBool_FromLong(pressed_buttons[i]));
}
Copy link
Member

Choose a reason for hiding this comment

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

What if the mouse doesn't support 5 buttons here? Just being curious here. And also what if we made these function configurable concerning the number of buttons we want to return? like by passing in an optional int like num_buttons.
Would require METH_FASTCALL tho.

Copy link
Member Author

@damusss damusss May 12, 2024

Choose a reason for hiding this comment

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

What if the mouse doesn't support 5 buttons here? Just being curious here. And also what if we made these function configurable concerning the number of buttons we want to return? like by passing in an optional int like num_buttons. Would require METH_FASTCALL tho.

Hi,
That's what MyreMylar talked about in the first comment. If the mouse doesn't support the buttons, the entries will always be False, and I personally don't see an issue with that. The tuple values come from a function that I created (edit: don't want to remove credits to who made it for the key module as I used it as a base) which memsets to 0, and if no events with those buttons are passed, it will never be set to 1.
The num_buttons added to get_pressed was for compatibility between pygame 1 and pygame 2 (added by MyreMylar) and MyreMylar even suggested that it could be removed with pygame 3. This is new API, so it doesn't need it. Removing that argument also cleaned up the code a lot.

@damusss
Copy link
Member Author

damusss commented May 12, 2024

@itzpr3d4t0r about the newlines, in my defense, it was clang-format as I didn't edit the end of the files. I also wonder why setup.py format is not an option anymore

@ankith26
Copy link
Member

It was removed in favour of pre-commit. You can achieve the same affect as python setup.py format by running pre-commit run -a now

Copy link
Member

@itzpr3d4t0r itzpr3d4t0r left a comment

Choose a reason for hiding this comment

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

Minor stuff aside this LGTM thanks!

I tested the functionality with this code btw:

import pygame
from random import random
pygame.init()

screen_size = 1000

screen = pygame.display.set_mode((screen_size, screen_size))

clock = pygame.time.Clock()
keep_going = True

particles = []

class Particle:
    def __init__(self, x, y, vx, vy):
        self.x = x
        self.y = y
        self.vx = vx
        self.vy = vy

    def update(self, dt):
        self.x += self.vx * dt
        self.y += self.vy * dt

        if self.x < 0:
            self.x = 0
            self.vx *= -1
        elif self.x > screen_size:
            self.x = screen_size
            self.vx *= -1

        if self.y < 0:
            self.y = 0
            self.vy *= -1
        elif self.y > screen_size:
            self.y = screen_size
            self.vy *= -1

        return self.x, self.y

while keep_going:
    dt = clock.tick_busy_loop(60) * 60 / 1000
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            keep_going = False

    screen.fill((0, 0, 0))

    for p in particles:
        x, y = p.update(dt)
        pygame.draw.circle(screen, (255, 255, 255), (int(x), int(y)), 5)

    if any(pygame.mouse.get_just_released()):
        particles.append(Particle(*pygame.mouse.get_pos(), random() * 2 - 1, random() * 2 - 1))

    pygame.display.flip()

docs/reST/ref/mouse.rst Outdated Show resolved Hide resolved
docs/reST/ref/mouse.rst Outdated Show resolved Hide resolved
@itzpr3d4t0r itzpr3d4t0r merged commit a39d374 into pygame-community:main May 18, 2024
38 of 39 checks passed
@itzpr3d4t0r itzpr3d4t0r added this to the 2.5.0 milestone May 18, 2024
@damusss damusss deleted the mouse-get-just-pressed-released branch May 19, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mouse pygame.mouse New API This pull request may need extra debate as it adds a new class or function to pygame
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants