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

Update set_timer patch #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

e-dong
Copy link

@e-dong e-dong commented Jan 3, 2024

This is a follow up to pygame-web/pygbag#16

Summary of changes

  • Handles termination of timer event for a event type when millis is 0
  • Implemented loops handling
  • Duplicated timer handling when set_timer is called multiple times

Reference:
https://pyga.me/docs/ref/time.html#pygame.time.set_timer

Not Implemented

when event param is an object

When this function is called with an Event object, the event(s) received on the event queue will be a shallow copy; the dict attribute of the event object passed as an argument and the dict attributes of the event objects received on timer will be references to the same dict object in memory. Modifications on one dict can affect another, use deepcopy operations on the dict object if you don't want this behaviour. However, calling this function with an integer event type would place event objects on the queue that don't have a common dict reference.

@e-dong
Copy link
Author

e-dong commented Jan 3, 2024

TODOs

@e-dong e-dong marked this pull request as draft January 3, 2024 17:19
@e-dong
Copy link
Author

e-dong commented Jan 5, 2024

@pmp-p Just for more context, I discovered the bug in my game because I use set_timer extensively.
e.g. https://github.com/e-dong/space-war-rl/blob/add-weapons/space_war/sim/ship.py#L335

You can test my game if you want here:
https://e-dong.itch.io/spacewar-dev

@pmp-p
Copy link
Member

pmp-p commented Feb 19, 2024

current implementation does not always handle event type correctly see #2

@e-dong e-dong force-pushed the fix-set-timer-patch branch from 444f765 to ae23564 Compare February 22, 2024 11:48
@pmp-p pmp-p marked this pull request as ready for review February 22, 2024 12:05
"""Patches the pygame.time.set_timer function to use gthreads"""

def patch_set_timer(
event: Union[int, pygame.event.Event],
Copy link
Author

Choose a reason for hiding this comment

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

I updated the args to match the pygame-ce api docs if that's okay with you @pmp-p 😄

@e-dong
Copy link
Author

e-dong commented Feb 22, 2024

@pmp-p I believe this is your last comment from discord:

could you simplify the thread id stuff with "ident" or "native_id" ? see https://docs.python.org/3/library/threading.html#threading.Thread.native_id

I will see if I can replace the usage of uuid lol

@e-dong
Copy link
Author

e-dong commented Feb 22, 2024

@pmp-p I took a look at your Thread patch:
https://github.com/pygame-web/pygbag/blob/93a0eacd47f89db2c414a3e3688365f41865dac6/src/pygbag/support/cross/aio/gthread.py#L179

and the ids are only assigned in the start() call.

The current implementation of the coroutine target function needs the id to be passed in as the parameter. The global THREADS dict also needs the id.

So if the assignment of the ids was moved to the constructor, I could do something like this:

thread = Thread(target=fire_event)
thread.args = thread.native_id
THREADS[event_type] = thread.native_id
thread.start()

I did notice you have a TODO here:
https://github.com/pygame-web/pygbag/blob/main/src/pygbag/support/cross/aio/gthread.py#L133

Is the goal to get access to the thread instance in the target coroutine? Then I could access the native_id.

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