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

get_just_pressed and get_just_released #1912

Merged
merged 12 commits into from
Sep 16, 2023

Conversation

ScriptLineStudios
Copy link
Member

duplicate of my old pr - pygame/pygame#3237

@ScriptLineStudios ScriptLineStudios requested a review from a team as a code owner February 20, 2023 19:53
@Matiiss Matiiss added New API This pull request may need extra debate as it adds a new class or function to pygame key pygame.key labels Feb 21, 2023
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

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

I'd be happy to approve this once you update the versionadded tags to 2.1.4

docs/reST/ref/key.rst Outdated Show resolved Hide resolved
docs/reST/ref/key.rst Outdated Show resolved Hide resolved
@robertpfeiffer
Copy link
Contributor

I really want this functionality merged, because it's something that exists in HaxeFlixel and Unity and everywhere. My main concern is what API/behaviour/guarantees this should give. I think as is, it wouldn't work in any of my games that get() different event types. I think the reset should only be triggered when dopump is true, or better yet, the reset code should live inside _pg_event_pump. You have my approval conditional on these changes.

I know this change will make the docs slightly more unwieldy, and I don't know how your code will interact with keyboard events introduced by pygame.event.post, but I think this is worth including now.

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.

Updating the version strings

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

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@robertpfeiffer robertpfeiffer left a comment

Choose a reason for hiding this comment

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

I don't think resetting on pygame.event.get() only is a good idea, and resetting on pygame.event.get(pump=False) is a very bad idea. I think it should reset on calls to pump.

I like the idea in general, I like the API, but I think we need to talk about this a little more. I'd also be open to implementing the change myself.

src_c/event.c Outdated Show resolved Hide resolved
src_c/event.c Outdated Show resolved Hide resolved
src_c/key.c Show resolved Hide resolved
@robertpfeiffer
Copy link
Contributor

Say the word and I push the changes I'd like to your branch

Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Directly pushed fixes for couple small issues I saw with it.

I like your changes @robertpfeiffer, this seems like a good use for an event filter.

This has got to be a record for the most people who've committed to a single PR branch?

src_c/event.c Outdated Show resolved Hide resolved
src_c/key.c Outdated Show resolved Hide resolved
@Starbuck5
Copy link
Member

@ankith26 Thanks for taking a look at this.

About your reviews on this PR, you should push those changes directly. Scriptline seems busy right now, and more focused on their recent PRs. Plus, we've got a record to set with the committers on this branch!

@MyreMylar
Copy link
Member

This has developed a small conflict. Looks like it could be ready to go for 2.4.0 with a tiny bit of love as it already has three approvals.

@MyreMylar MyreMylar added this to the 2.4.0 milestone Aug 14, 2023
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

Some reviews (mostly notes to self) that I will address when I get back to this PR later today

src_c/event.c Outdated Show resolved Hide resolved
src_c/key.c Outdated Show resolved Hide resolved
src_c/key.c Outdated Show resolved Hide resolved
src_c/key.c Show resolved Hide resolved
src_c/key.c Show resolved Hide resolved
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

I have pushed some changes, and now this PR looks good to go for me.

I also wrote a little test script to validate that my changes don't break anything, here is it for anyone else interested in testing things out

import pygame

win = pygame.display.set_mode((500, 500))

letters = [pygame.K_a + i for i in range(26)]

while True:
    cur_pressed = set()
    cur_released = set()
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            raise SystemExit

        if event.type == pygame.KEYDOWN:
            cur_pressed.add(event.key)

        if event.type == pygame.KEYUP:
            cur_released.add(event.key)
    
    for i in letters:
        if (i in cur_pressed) != pygame.key.get_just_pressed()[i]:
            raise RuntimeError(f"{i}")

        if (i in cur_released) != pygame.key.get_just_released()[i]:
            raise RuntimeError(f"{i}")

    pygame.time.wait(10)

Just run this script and spam letters randomly. There should be no error raised if the implementation is alright.

I'll wait for someone else to merge this PR though, because the last change (as of now) is mine

@Starbuck5 Starbuck5 merged commit bf6f577 into pygame-community:main Sep 16, 2023
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
key pygame.key 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.

None yet

8 participants