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

add slider style from yugecin/opsu-dance #263

Merged
merged 7 commits into from
Mar 9, 2017

Conversation

yugecin
Copy link
Contributor

@yugecin yugecin commented Mar 3, 2017

Didn't know how to name them, so I added it as 'Experimental slider style'

@itdelatrisu
Copy link
Owner

itdelatrisu commented Mar 4, 2017

Thanks so much! 🎉 🎉 🎉

I might not get a chance to really test this for a few days, but my observations so far:

  • You forgot to commit slidergradient_ex.png. :P
  • If experimental sliders are enabled, slider repeats shouldn't ever turn black (you can't see them).
  • Images are really broken when restarting the client. Haven't had a chance to look into the cause, but if you start a game, restart the client (in the options menu), then start another game, slider curves aren't rendered and and the background sometimes turns completely white.
  • Does EXPERIMENTAL_SLIDERS_DRAW_ENDCIRCLES ever look good enabled? If not, it doesn't need to be an option.

Also, what are "merged sliders"? I couldn't notice any difference with them on/off.

@yugecin
Copy link
Contributor Author

yugecin commented Mar 4, 2017

  • Haha I guess it's yugecin's law: 'If you made a pull request, you probably forgot to commit a file'
  • Added a check for the reversearrows
  • Fixed by uncommenting a line that I somehow once commented (bb94617)
  • Yep they look nice. osu! also has this style with endcircles before they added the option to remove them. I sometimes like to play with them enabled, and I can imagine lots of other players like it too
  • Merging sliders are, well, merging sliders :D
    mer
    nmer

@tpenguinltg
Copy link
Contributor

tpenguinltg commented Mar 4, 2017

Ooh, this looks nice.

There's a bug for concurrent sliders on 2B maps (e.g. Lanturn's Endless Tewi-ma Park) with "Merge Sliders" enabled. With "Shrink Sliders" enabled, only the latest slider that started appears. The tracks of any sliders that started earlier and are still running disappear.

From the 2B difficulty (middle started first, top started last):
screenshot_20170304_095656
screenshot_20170304_095742

Example:

  • S1: t=0..10
  • S2: t=2..4
  • S3: t=6..8

t=0: S1 starts
t=2: S2 starts. S1's track disappears
t=4: S2 finishes
t=6: S3 starts
t=8: S3 finishes
t=10: S1 finishes

With "Shrink Sliders" disabled, all concurrent sliders disappear when the shortest one ends, and they'll flash when they end.

Example:

  • S1: t=0..10
  • S2: t=2..4
  • S3: t=6..8

t=0: S1 starts
t=2: S2 starts
t=4: S2 finishes. S1's track disappears with S2
t=6: S3 starts
t=8: S3 finishes
t=10: S1's track flashes

Spinners don't seem to affect slider behaviour. Everything works as expected with "Merge Sliders" off.

@yugecin
Copy link
Contributor Author

yugecin commented Mar 4, 2017

Yep I knew that that is possible, when a slider starts while an other slider is still going, the previous slider disappears when the merge & shrink settings are enabled. I couldn't find a map that had this issue so I left it like that, but I didn't think of the 2B maps...

@yugecin
Copy link
Contributor Author

yugecin commented Mar 5, 2017

Should be fixed :)

@tpenguinltg
Copy link
Contributor

Indeed it is. Thanks!

The option hints can be a bit more descriptive. Maybe something like this:

  • Merge sliders: Show only the outline of overlapping sliders. Only for experimental sliders.
    • Alternatively: For overlapping sliders, don't draw the edges where they cross. Only for experimental sliders.
  • Shrink sliders: Sliders shrink towards their ending point. Only for experimental sliders.

For consistency with "Snaking sliders", perhaps they should also be renamed "Merging sliders" and "Shrinking sliders".

@@ -756,4 +756,14 @@ public static long getUsedMemory() {
Runtime r = Runtime.getRuntime();
return r.totalMemory() - r.freeMemory();
}

public static class Pair<F, S> {
Copy link
Owner

Choose a reason for hiding this comment

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

Eh... not a fan of this abstraction. It's barely more clear than just adding 2 elements in a row to your lists since this still doesn't tell you what they are, plus it's extra overhead.

@yugecin
Copy link
Contributor Author

yugecin commented Mar 5, 2017

Fair point, got rid of the Pair stuff.
Adjusted the option descriptions

@itdelatrisu itdelatrisu merged commit 959f46c into itdelatrisu:master Mar 9, 2017
itdelatrisu added a commit that referenced this pull request Mar 9, 2017
Signed-off-by: Jeffrey Han <itdelatrisu@gmail.com>
@yugecin yugecin deleted the experimentalsliders branch March 19, 2017 12:05
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.

None yet

3 participants