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

Hit object stacking algorithm #61

Merged
merged 5 commits into from
Mar 31, 2015

Conversation

DarkTigrus
Copy link
Contributor

Move slider.sliderTime calculation from update() to constructor. Necessarily for slider.getPointAt() to work properly.
To order to get accurate slider end position, need access to both HitObject and OsuHitObject at same time.
HitObject.updatePosition() is a workaround since we can't calculate stack when hit object initializates.
Algorithm is pretty solid. Some numbers (STACK_LENIENCE, STACK_TIMEOUT, stackModifier etc) can be a little bit off.

@itdelatrisu
Copy link
Owner

This looks really good in-game -- thanks so much for tackling this!

Some comments:

  • Why is HitObject.getHitObject() needed? From the Game class, you can access the OsuHitObject array through osu.objects.
  • Why can't you perform the stack calculations when initializing the HitObject arrays? It doesn't look like you're getting any slider points except the start/end points (correct me if I'm wrong), so all coordinates and fields should be accessible through the OsuHitObject array, right?
  • (Related to the point above.) The main issue I have with the current implementation is that you're calling updatePosition() on every object, even if its position isn't being changed. In particular, you're creating all the curves twice, which is expensive. Can you do anything about this? (If you fix the point above, this should become a non-issue.)
  • Performing the timing point calculations in the Slider constructor is a good call; thanks for cleaning that up. This is just me being picky, but I'd include a function like this instead of duplicating the code inside the two timing point loops (in the Game class):
    /**
     * Sets the beat length fields based on a given timing point.
     */
    private void setBeatLength(OsuTimingPoint timingPoint) {
        if (!timingPoint.isInherited())
            beatLengthBase = beatLength = timingPoint.getBeatLength();
        else
            beatLength = beatLengthBase * timingPoint.getSliderMultiplier();
    }
  • It'd be better to add a link to peppy's code in a comment, I think.

@DarkTigrus
Copy link
Contributor Author

Thanks for suggestions.

  1. You are right.
  2. Snaped to grid raw slider end point provided by OsuHitObject is not accurate. Curve should be inizalizated to get real position. But i have one idea how to fix that.
    Another minor complain - curve points initially scaled to display resolution.
  3. ,5 It should be better now.

@DarkTigrus
Copy link
Contributor Author

Unfortunately i didn't manage to do calculation in one loop pass. Problems:

  • Getting slider end point. Doable. My workaround was creating Curve and then pass in to Slider as argument. Overhead - sometimes we inizalizating curve for N object and then wasting it since we can't properly pass it to anything without other arguments (data, color, comboEnd, etc). Another concern is that curve points scaled to display size. Not a problem because OsuHitObject that return scaled values as well.
  • Applying X,Y right away can cause errors in algorithm. That is why i created third loop just for update coordinates. Probably fixable by spliting getScaledX() method or changing algorithm itself.
  • Real problem is getting slider end time. Slider can be not init yet. We need to manualy calculate endTime every time objN is slider.

Fixed crazy updatePosition() overhead.

@DarkTigrus DarkTigrus changed the title Hit object stacking algorithm [WIP ]Hit object stacking algorithm Mar 30, 2015
@DarkTigrus DarkTigrus changed the title [WIP ]Hit object stacking algorithm [WIP] Hit object stacking algorithm Mar 30, 2015
Revert previous commit. Curve should be re-created or updated.
No more extra updatePosition() calls.
@DarkTigrus DarkTigrus changed the title [WIP] Hit object stacking algorithm Hit object stacking algorithm Mar 30, 2015
@itdelatrisu
Copy link
Owner

Wait, I think I'm still missing something...

  • Slider end point: this is the last control point, which is in OsuHitObject.getSliderX() (or Y, or the scaled versions).
  • Slider end time: this can be calculated without the Curve object, since it's just:
float sliderTime = game.getBeatLength() * (hitObject.getPixelLength() / osu.sliderMultiplier) / 100f;
float sliderTimeTotal = sliderTime * hitObject.getRepeatCount();
float endTime = hitObject.getTime() + sliderTimeTotal;

So as long as you're passing in the correct timing points (might be tricky, but certainly doable), you can get the end time without creating a Slider object at all.

Isn't this possible in one pass, then?

@DarkTigrus
Copy link
Contributor Author

  • Slider end point not always last control point. Usually opposite

    And we need Curve everytime objN is curve. In two pass loop we have at least relevant Slider which can be not exist in one pass loop.
  • Yes. it is possible.

One pass loop don't really have any pros. I think current two pass version is fine.

@itdelatrisu
Copy link
Owner

Ah, I didn't realize that control points weren't necessarily the curve endpoints -- I haven't kept up with fluddokt's slider changes. Thanks a lot for pointing that out (and sorry for the confusion!). In that case, what you're doing is fine. I'll try to have this merged in the next few hours. Thanks again!

itdelatrisu added a commit that referenced this pull request Mar 31, 2015
@itdelatrisu itdelatrisu merged commit f63be81 into itdelatrisu:master Mar 31, 2015
itdelatrisu added a commit that referenced this pull request Mar 31, 2015
Signed-off-by: Jeffrey Han <itdelatrisu@gmail.com>
itdelatrisu added a commit that referenced this pull request Mar 31, 2015
Also some additional tweaks to #61.

Signed-off-by: Jeffrey Han <itdelatrisu@gmail.com>
@DarkTigrus DarkTigrus deleted the hitobject-stacking branch April 1, 2015 12:52
itdelatrisu added a commit that referenced this pull request Apr 10, 2015
- Actually provide relevant information when an audio file can't be found.
- When loading timing points, don't reset the start index every time.

Signed-off-by: Jeffrey Han <itdelatrisu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants