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

Fix Judging For Notes When Using Input Offset #2987

Closed
wants to merge 14 commits into from

Conversation

lemz1
Copy link
Contributor

@lemz1 lemz1 commented Jul 9, 2024

This pr fixes an issue where the notes are wrongly judged when using an input offset.

This #2969 describes also another the problem in detail, which is that seemingly that above the strum is a shorter hit window than below. This is not yet fixed. (Im not sure if we are all just crazy)

To demonstrate the issue more clearly im using extremely large offsets.
Using:
Input Offset: 160ms
Visual Offset: 0ms

Before

Input.Offset.Test.mp4

After

Cool.New.mp4

@Hundrec
Copy link
Contributor

Hundrec commented Jul 9, 2024

Nice and easy fix!

Does this provide more insight into the offset-by-default hit window described in the issue?

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

Does this provide more insight into the offset-by-default hit window described in the issue?

you mean the issue where, it seemed that the hit window above was shorter than below?

@Hundrec
Copy link
Contributor

Hundrec commented Jul 9, 2024

Yep!

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

i haven't tested it out, lemme see

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

no, i don't think it fixes that issue, still seems like below has a larger hit window than above

@Hundrec
Copy link
Contributor

Hundrec commented Jul 9, 2024

That makes sense, I expected the issue to remain.

I think a similar change could be made to address the offset window somewhere in the same file!

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

It might be fixed now, i cant really tell.
The reason for using the abs of the difference, is that the inputLatencyMs seems to be always positive, no matter if you hit it early or later. However because it is like that we need to ensure consistency with the signs.

Wait, actually im not sure if this is correct.

I have reverted it, i thought about it in a wrong way

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

Also another reason why it seems as if the hit window is shorter above, is because when songPosition - strumTime is equal to 0, the notes are a tiny bit below the strum, like 5-10 pixels

@Hundrec
Copy link
Contributor

Hundrec commented Jul 9, 2024

This might be more appropriate in a separate PR, but I think we could probably just bump the notes up by the difference (if we can confirm it's there)

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

Here's a screenshot of that

screenshot-2024-07-09-23-19-27

This might be more appropriate in a separate PR, but I think we could probably just bump the notes up by the difference (if we can confirm it's there)

We could, but maybe the system for note positioning needs to be reworked in general. It seems overly complicated with all the weird variables like INITIAL_OFFSET, NUDGE, etc.

@Hundrec
Copy link
Contributor

Hundrec commented Jul 9, 2024

Good screenshot and good point!

I also noticed the notes seem to be a tiny bit left of the strumline as well.

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

Here is also a demonstration of the the sick hit window
45 is the sick threshold

songPosition - strumTime = -45
screenshot-2024-07-09-23-35-27

songPosition - strumTime = 45
screenshot-2024-07-09-23-38-01

Visually it seems pretty much correct, i dont know if we are all just insane.

@Hundrec
Copy link
Contributor

Hundrec commented Jul 9, 2024

Out of curiosity, how are you pinpointing these values? Are you hitting the notes and getting a perfect score or something else?

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

Out of curiosity, how are you pinpointing these values? Are you hitting the notes and getting a perfect score or something else?

i replace strumTime:

note.y = this.y - INITIAL_OFFSET + calculateNoteYPos(note.strumTime, vwoosh);

with songPosition:

note.y = this.y - INITIAL_OFFSET + calculateNoteYPos(Conductor.instance.songPosition, vwoosh);

in the case of the sick threshold i did:

note.y = this.y - INITIAL_OFFSET + calculateNoteYPos(Conductor.instance.songPosition + 45, vwoosh);

and

note.y = this.y - INITIAL_OFFSET + calculateNoteYPos(Conductor.instance.songPosition - 45, vwoosh);

This works because for the calculation it does songPositon - time. Meaning when time is set to songPosition, it will always equal 0, which is why i can add specific millisecond offsets that i want to test

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

Or did you mean, how i know what time they are. If so in Scoring.hx there is this part:

  /**
   * The time within which a note is considered to have been hit with the Sick judgement.
   * `~25% of the hit window, or 45ms`
   */
  public static final PBOT1_SICK_THRESHOLD:Float = 45.0;

  /**
   * The time within which a note is considered to have been hit with the Good judgement.
   * `~55% of the hit window, or 90ms`
   */
  public static final PBOT1_GOOD_THRESHOLD:Float = 90.0;

  /**
   * The time within which a note is considered to have been hit with the Bad judgement.
   * `~85% of the hit window, or 135ms`
   */
  public static final PBOT1_BAD_THRESHOLD:Float = 135.0;

  /**
   * The time within which a note is considered to have been hit with the Shit judgement.
   * `100% of the hit window, or 160ms`
   */
  public static final PBOT1_SHIT_THRESHOLD:Float = 160.0;

@Hundrec
Copy link
Contributor

Hundrec commented Jul 9, 2024

Out of curiosity, how are you pinpointing these values? Are you hitting the notes and getting a perfect score or something else?

i replace strumTime:

note.y = this.y - INITIAL_OFFSET + calculateNoteYPos(note.strumTime, vwoosh);

with songPosition:

note.y = this.y - INITIAL_OFFSET + calculateNoteYPos(Conductor.instance.songPosition, vwoosh);

in the case of the sick threshold i did:

note.y = this.y - INITIAL_OFFSET + calculateNoteYPos(Conductor.instance.songPosition + 45, vwoosh);

and

note.y = this.y - INITIAL_OFFSET + calculateNoteYPos(Conductor.instance.songPosition - 45, vwoosh);

This works because for the calculation it does songPositon - time. Meaning when time is set to songPosition, it will always equal 0, which is why i can add specific millisecond offsets that i want to test

This is a very clever debugging method! Is it possible for you to use this method to manually adjust the notes to perfect alignment?

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

Is it possible for you to use this method to manually adjust the notes to perfect alignment?

what do you mean with perfect alignment?

@Hundrec
Copy link
Contributor

Hundrec commented Jul 9, 2024

Also another reason why it seems as if the hit window is shorter above, is because when songPosition - strumTime is equal to 0, the notes are a tiny bit below the strum, like 5-10 pixels

I mean changing the positions of the colored notes to match the gray notes at the center of the hit window. This might resolve the offset hit window issue!

@lemz1
Copy link
Contributor Author

lemz1 commented Jul 9, 2024

this is something that probably shouldnt be handled by adjusting the time calculation.
it would be better to use width and height calculations to perfectly center them.
maybe it has something to do with NUDGE, but idk

@Hundrec
Copy link
Contributor

Hundrec commented Jul 9, 2024

I'll tinker around to see if I can move the arrows. Thanks for the info!

@EliteMasterEric EliteMasterEric added status: pending triage The bug or PR has not been reviewed yet. type: minor bug Involves a minor bug or issue. small A small pull request with 10 or fewer changes labels Jul 10, 2024
@EliteMasterEric EliteMasterEric deleted the branch FunkinCrew:develop July 12, 2024 01:00
@github-actions github-actions bot added the haxe Issue/PR modifies game code label Oct 8, 2024
@lemz1 lemz1 closed this Oct 16, 2024
@lemz1 lemz1 deleted the jugde-note-fix branch October 16, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
haxe Issue/PR modifies game code small A small pull request with 10 or fewer changes status: pending triage The bug or PR has not been reviewed yet. type: minor bug Involves a minor bug or issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants