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

Implement the editor's beat snap divisor #2247

Merged
merged 35 commits into from Mar 22, 2018

Conversation

2 participants
@smoogipoo
Contributor

smoogipoo commented Mar 19, 2018

Integrated into editor seeking functionality. The divisor bindable is DI'd into components that need it.

@peppy peppy added this to the March 2018 milestone Mar 19, 2018

@peppy peppy added the editor label Mar 19, 2018

@@ -68,6 +69,7 @@ private void load(OsuGameBase osuGame)
var clock = new DecoupleableInterpolatingFramedClock { IsCoupled = false };
dependencies.CacheAs<IAdjustableClock>(clock);
dependencies.CacheAs<IFrameBasedClock>(clock);
dependencies.Cache(new BindableBeatDivisor());

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

rather than introducing cross-dependencies like this, just set a default value to the bindable on the components being tested and only bind externals if not-null.

This comment has been minimized.

@smoogipoo

smoogipoo Mar 19, 2018

Contributor

You're going to have to define what "cross-dependency" means.

Idk, I feel like that's going to bite us (or someone else reading the code) in the arse when they go "permitNulls is true, so I can just pass a null game and clocks".

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

The idea behind DI/bindables is that when testing, you don't need to provide every component.

If unavailable, the components will still be testable on their own. You should only be providing dependencies if you require them to do further testing by altering/querying them. tests should be as restricted in scope as possible.

This comment has been minimized.

@smoogipoo

smoogipoo Mar 19, 2018

Contributor

It's also the responsibility of testcases to put components in a valid environment, and sometimes that requires also having clocks, the game/osucolour, etc, like most of the editor components currently tested. Sometimes that also requires that these components have a beat snap divisor to look onto.

Personally I think it's actually good that the bindable is defined here in this test case, depending on how we want to continue doing this test in the future we may change the value - it does support seeking after all.

I'm just not sure it's worth it, or is even an issue at all.

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

We may want to consider moving the bindable to ctor in this case... since it becomes a required dependency.

@peppy

This comment has been minimized.

Member

peppy commented Mar 20, 2018

Requesting your review of my changes. Looks good if you're happy!

{
new Drawable[]
{
new TickSliderBar(beatDivisor, 1, 2, 3, 4, 6, 8, 12, 16)

This comment has been minimized.

@smoogipoo

smoogipoo Mar 21, 2018

Contributor

These should not be defined a second time here. Use BindableBeatDivisor.AVAILABLE_DIVISORS

protected override bool OnKeyDown(InputState state, KeyDownEventArgs args)
{
if (!IsHovered || CurrentNumber.Disabled)

This comment has been minimized.

@smoogipoo

smoogipoo Mar 21, 2018

Contributor

HandleKeyboardInput => IsHovered && !CurrentNumber.Disabled instead. Saves from adding this composite to the list of possible input targets.

This comment has been minimized.

@peppy

peppy Mar 21, 2018

Member

This should probably be changed on SliderBar too, if this works (code was c+p from there).

@peppy peppy dismissed their stale review via 4814260 Mar 21, 2018

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Mar 22, 2018

Hmm, doesn't the text look a bit too close to the bottom?

screen shot 2018-03-22 at 8 07 43 pm

@peppy

This comment has been minimized.

Member

peppy commented Mar 22, 2018

Yeah, but it's the best I could come up with in 5pixel increments.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Mar 22, 2018

LGTM after my last change, review required @peppy

@peppy

peppy approved these changes Mar 22, 2018

@peppy peppy merged commit bedb427 into ppy:master Mar 22, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@smoogipoo smoogipoo deleted the smoogipoo:beat-snap-divisor branch Jun 15, 2018

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