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

Added event support (except for storyboards) #69

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

kszlim
Copy link

@kszlim kszlim commented May 5, 2020

Added event support (breaks, videos, background)

I don't expect this to be accepted as is, but wanted to see if this is roughly along the lines of what you expect.

Missing:
Docstrings
Tests (1 simple test case is available)

@llllllllll llllllllll self-requested a review May 5, 2020 18:07
slider/beatmap.py Outdated Show resolved Hide resolved
slider/beatmap.py Outdated Show resolved Hide resolved
slider/beatmap.py Outdated Show resolved Hide resolved
slider/beatmap.py Outdated Show resolved Hide resolved
event_type, start_time, *event_params = data.split(',')
try:
start_time = int(start_time)
start_time = timedelta(milliseconds=start_time)
Copy link
Author

Choose a reason for hiding this comment

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

I'm assuming timedelta in this lib is used to refer to delta from beginning vs delta from last event?

slider/beatmap.py Outdated Show resolved Hide resolved
slider/beatmap.py Outdated Show resolved Hide resolved
@tybug
Copy link
Collaborator

tybug commented May 12, 2022

not sure why I never reviewed this PR properly but I'll try and get this in once my exams are over (only 2 years late...)

slider/beatmap.py Outdated Show resolved Hide resolved
@tybug
Copy link
Collaborator

tybug commented May 18, 2022

I pushed some significant structure changes to try and get this into a better state. I also removed the unimplemented events (sprite/animation) instead of half-implementing them; was more trouble than it's worth. Let me know if you disagree with any of my changes.

@kszlim
Copy link
Author

kszlim commented May 19, 2022

Honestly I've forgotten about how this PR mostly works, so feel free to make whatever changes you like.

@tybug
Copy link
Collaborator

tybug commented May 19, 2022

no worries, I kinda figured that was the case

@tybug
Copy link
Collaborator

tybug commented May 30, 2022

for anyone following along - I'm not happy with how this PR deals with not-yet-implemented events, so I need to work on that before pushing this along. I have some local changes to that effect but I'm not happy with them either. I would say it would be less effort to just fully implement the rest of the events, but storyboard events are so underdocumented that I don't really want to touch it yet.

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