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

Allow previewing background adjustments at player loading screen #2045

Merged
merged 11 commits into from Mar 8, 2018

Conversation

2 participants
@UselessToucan
Copy link
Contributor

UselessToucan commented Feb 8, 2018

I extracted some common stuff from Player/PlayerLoader to new PlayerBase class and changed PlayerLoader's background behavior. Here is how it works now.

@peppy peppy added the gameplay label Feb 9, 2018

@peppy peppy added this to the Candidate Issues milestone Feb 9, 2018

@peppy

This comment has been minimized.

Copy link
Member

peppy commented Feb 14, 2018

You've caused the loading of everything twice and removed async loading. We cannot do either of these things.

@UselessToucan

This comment has been minimized.

Copy link
Contributor Author

UselessToucan commented Feb 14, 2018

I checked PlayerBase, PlayerLoader, Player code and I do not understand what do you mean. Please explain it in more details.

loading of everything twice

If you are about storyboard, then the only call which actually loads something is this one. Note that it was not async before.

removed async loading

Well, I just don't understand which async loading do you mean. The player loading is async, storyboard loading isn't, but you can read about it above.

@peppy

This comment has been minimized.

Copy link
Member

peppy commented Feb 15, 2018

Everything inside that load method was previous async loaded while PlayerLoader is visible. You moved a portion of this to a base class which is loaded twice.

The storyboard is being loaded in the UpdateBackgroundElements call, which means it can potentially be loaded once in PlayerLoader and again when Player loads. Also you are loading samples in PlayerLoader which are not used until Player, moving more of the load process to PlayerLoad.

@UselessToucan

This comment has been minimized.

Copy link
Contributor Author

UselessToucan commented Feb 15, 2018

Everything inside that load method was previous async loaded while PlayerLoader is visible. You moved a portion of this to a base class which is loaded twice.

I need config values on two screens(Player, PlayerLoader). What is wrong with loading things wherever I need them?

The storyboard is being loaded in the UpdateBackgroundElements call, which means it can potentially be loaded once in PlayerLoader

Since PlayerLoader.StoryboardContainer is always null, this condition is always true for PlayerLoader.

Also you are loading samples in PlayerLoader which are not used until Player

Yes, this needs to be moved to Player.

@peppy

This comment has been minimized.

Copy link
Member

peppy commented Feb 15, 2018

Even if that's the case for storyboards, the logic is so convoluted I couldn't see that.

We definitely need some restructuring here. Hopefully moving all storyboard loading to Player itself (make UpdateBackgroundElements virtual?)

Also I'd recommend renaming PlayerBase to ScreenWithBeatmapBackground or similar.

UselessToucan and others added some commits Feb 15, 2018

@peppy peppy changed the title PlayerLoader: visual settings preview Allow previewing background adjustments at player loading screen Feb 16, 2018

@peppy

This comment has been minimized.

Copy link
Member

peppy commented Feb 16, 2018

Adding dependency on #2073.

@peppy peppy requested review from smoogipoo and removed request for smoogipoo Feb 16, 2018

UselessToucan and others added some commits Mar 5, 2018

UserAudioOffset = config.GetBindable<double>(OsuSetting.AudioOffset);
}

protected void ConfigureBackgroundUpdate()

This comment has been minimized.

@peppy

peppy Mar 8, 2018

Member

why is this a thing? it's called from the same place in both consumers of this screen. is there something i'm missing or can this just be moved to OnEntering?

This comment has been minimized.

@UselessToucan

UselessToucan Mar 8, 2018

Author Contributor

You are correct. Can be moved to ScreenWithBeatmapBackground.OnEntering.

@peppy

This comment has been minimized.

Copy link
Member

peppy commented Mar 8, 2018

Made a minor change to avoid spilling unnecesssary bindables into the new class, but looks good otherwise 👍

@peppy

peppy approved these changes Mar 8, 2018

@peppy peppy merged commit c11f6ef into ppy:master Mar 8, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@UselessToucan UselessToucan deleted the UselessToucan:PlayerBase branch Mar 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.