Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Snaking sliders initial implementation. #134

Merged
merged 1 commit into from
Sep 18, 2015

Conversation

Bigpet
Copy link
Contributor

@Bigpet Bigpet commented Sep 16, 2015

regarding #133

Not ready for merging yet (mmsliders needs to be optimized). This is mainly for discussion around things like the pop-in of "repeat" arrows and other complications.

Draws curves in a range from 0 to x (where x is a value between 0 and 1), instead of always drawing the full curve

@itdelatrisu
Copy link
Owner

Oh wow, that was fast, thanks!

The repeat arrows in osu! seem to fade in after the curve is fully faded in/expanded, and very quickly (around 100-200ms, but it's kind of hard to tell). The ticks also seem to fade in at the same rate/time as the repeat arrows -- though maybe starting around 50ms later (again, it's hard to tell). While you're at it, if you want, the ticks also get scaled from half size to full size while fading in, and with the AnimationEquation.OUT_BACK easing. Everything else looks fine to me.

There should be an on/off flag in the game options for this, so just add this line:

SNAKING_SLIDERS ("Snaking sliders", "SnakingSliders", "Sliders gradually snake out from their starting point.\nThis should run fine unless you have a low-end PC.", true),

And I'd add it right below the IGNORE_BEATMAP_SKINS option in OptionsMenu.java.

Lastly, I've just been calling every [0,1] parameter t, and I'd clamp all the drawUpTo definitions to the interval be safe. There's also a Javadoc warning on CurveRenderState.java:84 in a @link tag since you changed a method signature.

@Bigpet
Copy link
Contributor Author

Bigpet commented Sep 17, 2015

I'm not so sure about the performance impact with optimizations.

It might be a toss up or a trade-off. Since now the spike in draw calls when a new curve is rendered is lessened a bit (since the whole curve does not need to be drawn in 1 frame), but there is more frames in which the framebuffer needs to be switched to draw the next few segments of the slider.

snakingperformance

(btw. I still need to do some cleanup and the return arrow blend in, so not ready for merging)

@itdelatrisu
Copy link
Owner

Do you think performance will actually be an issue? And I don't think it's much of a problem anyway, since we'll have the on/off switch.

Let me know if there's any part of this you want me to do, too.

@Bigpet
Copy link
Contributor Author

Bigpet commented Sep 17, 2015

@itdelatrisu no, I don't think performance will be an issue, hence my comment. I was just trying to say that "This should run fine unless you have a low-end PC" might be inaccurate and there might actually be some more stuttering without snaking slider in some cases.

@itdelatrisu
Copy link
Owner

Oh, sorry, I misread that then. We can take that line out; I just copied the text from osu!.

@Bigpet
Copy link
Contributor Author

Bigpet commented Sep 18, 2015

Alright, I think I got the most important things down.

You can start reviewing I guess.

Just one question, do you want
0.5f + 0.5f*AnimationEquation.OUT_BACK.calc(decorationsAlpha); or AnimationEquation.OUT_BACK.calc(0.5f+0.5f*decorationsAlpha); ?

Also, I might have asked this once already, but do you prefer things squashed into one commit?

edit: oh and I set the blend-in for ticks and return arrows as twice as fast as fadeInTime for now to test it

@itdelatrisu
Copy link
Owner

I think it looks good. I'd just rewrite the tick code to avoid recalculating values:

        if (ticksT != null) {
            float tickScale = 0.5f + 0.5f * AnimationEquation.OUT_BACK.calc(decorationsAlpha);
            Image tick = GameImage.SLIDER_TICK.getImage().getScaledCopy(tickScale);
            Colors.WHITE_FADE.a = decorationsAlpha;
            for (int i = 0; i < ticksT.length; i++) {
                Vec2f c = curve.pointAt(ticksT[i]);
                tick.drawCentered(c.x, c.y, Colors.WHITE_FADE);
            }
            Colors.WHITE_FADE.a = alpha;
        }

I don't really care if you squash or not. I'll merge this shortly if you're done. Thanks for doing this!

@Bigpet Bigpet force-pushed the snakeslider branch 2 times, most recently from c054d50 to 9d708e1 Compare September 18, 2015 18:20
Draws curves in a range from 0 to x (where x is a value between 0 and 1) while blending them in (if enabled in the options)
Cache a unit-cone once and re-use that with scaling and translation instead of generating it each time again.
This is more for readability than performance (since presumably feeding it mostly constants like before would be faster)

Only draws the cones in the curve once if possible
(only possible if consective calls to draw with the intervals x_1 before x_2 are made and [0, x_1] [0, x_2] with x_1 < x_2 holds true)

minor refactoring and cleanup

Omg my code is such a mess. I feel like I've committed a crime against humanity by just randomly putting that
random vbo id into the class called "Rendertarget". But there's really no good place for it now (that has a way to clean it up).
But if "Rendertarget" will ever be used by anything else but the sliders I'm gonna pull that out.

added blending in for return arrows and ticks
@Bigpet
Copy link
Contributor Author

Bigpet commented Sep 18, 2015

@itdelatrisu yeah, I think that should be it for now.

itdelatrisu added a commit that referenced this pull request Sep 18, 2015
Snaking sliders implementation.
@itdelatrisu itdelatrisu merged commit daf3a2a into itdelatrisu:master Sep 18, 2015
itdelatrisu added a commit that referenced this pull request Sep 18, 2015
Signed-off-by: Jeffrey Han <itdelatrisu@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants