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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX] Correct step lengths in x/4 time signatures. #3067

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

[BUGFIX] Correct step lengths in x/4 time signatures. #3067

wants to merge 1 commit into from

Conversation

Keoiki
Copy link
Contributor

@Keoiki Keoiki commented Jul 27, 2024 •
edited
Loading

Briefly describe the issue(s) fixed.

Without changing the BPM, stepLengthMs has a different a different value depending on the x/4 signature.

  • Example: at 100 BPM, in x/4 time a beat is 600ms, so each step is 150ms, in 2/4 that was 300ms instead, 100ms in 6/4 and so on.
  • This affected, for example, how often a Character plays their idle animation.

This PR simply fixes the wrong math.

Briefly describe the issue(s) fixed.

This addresses the 3rd section of #3066

@Keoiki
Stop using the numerator for step length math
4806ee7 
Step length should stay the same if the denominator stays the same.
This is only a temporary fix, probably.
Example: at 100 BPM, in 4/4 a beat is 600ms, so each step is 150ms, in 2/4 that would have been 300ms instead.
@Keoiki Keoiki changed the base branch from main to develop July 28, 2024 01:43
@EliteMasterEric
Copy link
Member

How does this affect things on time signatures with other denominators (8/16)?

@EliteMasterEric EliteMasterEric self-assigned this Jul 28, 2024
@EliteMasterEric EliteMasterEric added type: minor bug Involves a minor bug or issue. status: reviewing internally This PR is under internal review and quality assurance testing chart editor Issue is related to the operation of the Chart Editor. small A small pull request with 10 or fewer changes labels Jul 28, 2024
@Keoiki
Copy link
Contributor Author

Keoiki commented Jul 28, 2024

This shouldn't have any issues with those denominators, here's actually a comparison with the 3/8 beat I've made and been using:
(Notice how the metronome doesn't align with Up (measure beat hits) and Left (other beat hits) notes in the Before clip.)

Before:

2024-07-28.08-52-22.mov

After:

2024-07-28.08-53-11.mov

Cartridge-Man
Cartridge-Man approved these changes Jul 29, 2024
View reviewed changes
Copy link

@Cartridge-Man Cartridge-Man left a comment

Choose a reason for hiding this comment

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

Dude thank god for this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cartridge-Man Cartridge-Man Cartridge-Man approved these changes

Assignees

@EliteMasterEric EliteMasterEric

Labels
chart editor Issue is related to the operation of the Chart Editor. small A small pull request with 10 or fewer changes status: reviewing internally This PR is under internal review and quality assurance testing type: minor bug Involves a minor bug or issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Keoiki @EliteMasterEric @Cartridge-Man