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

Refactor GameWindow to not extend OpenTK.GameWindow #1420

Merged
merged 7 commits into from Mar 7, 2018

Conversation

3 participants
@swoolcock
Contributor

swoolcock commented Feb 26, 2018

Tested on macOS, please test on Windows and Linux.
Closes #1102

Will require a change to the osu.Desktop project unless we make a wrapper in GameWindow for Icon, Title, and FileDrop that redirect to Implementation.

Edit: Backported commit: c7935ae

currentPosition.Y = ((float)((state.Y - raw_input_resolution / 2f) * sensitivity.Value) + raw_input_resolution / 2f) / raw_input_resolution
* host.Window.Height;
* host.Window.Implementation.Height;

This comment has been minimized.

@smoogipoo

smoogipoo Feb 26, 2018

Contributor

Let's put this on the same line as currentPosition.X does.

namespace osu.Framework.Platform
{
public abstract class GameWindow : OpenTK.GameWindow
public abstract class GameWindow

This comment has been minimized.

@smoogipoo

smoogipoo Feb 26, 2018

Contributor

Just to reduce the sheer number of .Implementation calls around the code, can we make this inherit IGameWindow and act as a proxy to Implementation? Implementation itself should probably be private or protected at most.


Context.MakeCurrent(null);
gw.MouseEnter += (sender, args) => CursorInWindow = true;
gw.MouseLeave += (sender, args) => CursorInWindow = false;

This comment has been minimized.

@smoogipoo

smoogipoo Feb 26, 2018

Contributor

It looks like these should be a part of the INativeWindow interface, which IGameWindow inherits. Can we move this to the other ctor?

protected void OnKeyDown(object sender, KeyboardKeyEventArgs e)
{
if (e.Key == Key.F4 && e.Alt)
Implementation.Exit();

This comment has been minimized.

@smoogipoo

smoogipoo Feb 26, 2018

Contributor

Along with the previous suggestions, we should be able to fix this potential little bit of unsafety - this code previously early-returned on Alt+F4.

Essentially it'd be something like

IGameWindow implementation; // private
implementation.KeyDown += OnKeyDown;

virtual OnKeyDown()
{
    KeyDown?.Invoke();
}

This comment has been minimized.

@swoolcock

swoolcock Feb 27, 2018

Contributor

Can I get a bit of clarification on this point please? I'm not sure how your example differs from the code in the PR.

This comment has been minimized.

@smoogipoo

smoogipoo Feb 27, 2018

Contributor

Then WindowsGameWindow can override it and return after calling .Exit, as it does on master, without then invoking any subscribers of the KeyDown event.

@peppy peppy changed the title from Backport smoogi’s GameWindow refactor from ios branch to Refactor GameWindow to not extend OpenTK.GameWindow Feb 26, 2018

@swoolcock swoolcock force-pushed the swoolcock:refactor-gamewindow branch from 0901748 to 275b42b Feb 27, 2018

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

@peppy peppy added the code quality label Feb 28, 2018

@peppy peppy added resolves issue and removed code quality labels Mar 3, 2018

@peppy peppy removed this from the Candidate Issues milestone Mar 3, 2018

@smoogipoo

LGTM 👍

@smoogipoo smoogipoo merged commit 3cac424 into ppy:master Mar 7, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@smoogipoo smoogipoo modified the milestone: March 2018 Mar 7, 2018

@peppy

This comment has been minimized.

Member

peppy commented Mar 9, 2018

This looks to have not been tested on windows. Causes silent crash on startup.

peppy added a commit to peppy/osu-framework that referenced this pull request Mar 9, 2018

FreezyLemon added a commit to FreezyLemon/osu-framework that referenced this pull request Mar 9, 2018

@swoolcock swoolcock deleted the swoolcock:refactor-gamewindow branch Jul 20, 2018

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