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

Rewrite GLInterface #764

Merged
merged 19 commits into from Aug 21, 2014
Merged

Rewrite GLInterface #764

merged 19 commits into from Aug 21, 2014

Conversation

magcius
Copy link
Member

@magcius magcius commented Aug 9, 2014

Remove Wayland support to clean up the code -- it's bitrotten and doesn't work anymore on my system.

Clean up a lot of our GL interfaces and rewrite MainNoGUI so that it's not an ifdef soup.

@lioncash
Copy link
Member

lioncash commented Aug 9, 2014

Looks good to me as far as I can tell.

@delroth
Copy link
Member

delroth commented Aug 9, 2014

I thought @Sonicadvance1 said removing EGL/X11 support was a no-go since some ARM GPU drivers only support this configuration on X11?

@magcius
Copy link
Member Author

magcius commented Aug 9, 2014

See the last commit :)

@delroth
Copy link
Member

delroth commented Aug 9, 2014

Ok, that's confusing :) Any reason why you remove it and re-add it?

@magcius
Copy link
Member Author

magcius commented Aug 9, 2014

Because the old implementation was an ifdef soup, and I didn't want to perform cleanups based on that. It's effectively a whole new implementation.

@delroth
Copy link
Member

delroth commented Aug 9, 2014

SGTM, how much testing have you done on that? Won't be able to test it until I get back home a week from now, maybe @Sonicadvance1 has an X11/EGL device with him.

@magcius
Copy link
Member Author

magcius commented Aug 9, 2014

I tested EGL/X11 under NoGUI on my local machine and it worked fine. There's a bit of flickering, but I'm quite sure it's not my fault.

@Sonicadvance1
Copy link
Contributor

GUI-less build the title bar isn't updated with FPS information any longer.
GUI-less build I don't get any video output with my Intel HD GPU with mesa.
GUI build I also get no video output
Tested only X11+GL and EGL+GL, no video output with either of these.

@Sonicadvance1
Copy link
Contributor

I'll review this branch more once these issues are fixed.

@magcius
Copy link
Member Author

magcius commented Aug 16, 2014

Did you try resizing the window a bit after starting up GUI / GUI-less? This is a bug, but not one caused by this branch. There are patches to fix it in another bug, waiting on the branch.

I'll look into the FPS display, but it was working for me last I tried.

@archshift
Copy link
Contributor

The fix for your second issue would be here.

@Sonicadvance1
Copy link
Contributor

Video output works fine in GUI-less build for me on latest master.
So this does introduce a bug somewhere.

@magcius
Copy link
Member Author

magcius commented Aug 16, 2014

With EGL or GLX?

@Sonicadvance1
Copy link
Contributor

Both.

@magcius
Copy link
Member Author

magcius commented Aug 16, 2014

I do get FPS in the titlebar on nogui: http://i.imgur.com/fRML2Mr.png

Video output did not work on git master for me.

@Sonicadvance1
Copy link
Contributor

http://gdurl.com/TW1B
No FPS display on my window manager.
So you're doing something differently that doesn't apply on my WM.

@magcius
Copy link
Member Author

magcius commented Aug 19, 2014

Can you run xprop on the window?

@Sonicadvance1
Copy link
Contributor

_NET_WM_STATE(ATOM) =
WM_NAME(STRING) = "Dolphin [HEAD] 4.0-2500"
WM_STATE(WM_STATE):
window state: Normal
icon window: 0x0
WM_PROTOCOLS(ATOM): protocols WM_DELETE_WINDOW

@magcius
Copy link
Member Author

magcius commented Aug 19, 2014

Bizarre. Can you check if Host_UpdateTitle is getting called, and if MainNoGUI is receiving it properly? It might be related to RenderToMain or InterfaceStatusbar being enabled even if it's in NoGUI.

@Sonicadvance1
Copy link
Contributor

Host_UpdateTitle is being called with that string that it is set to.

@magcius
Copy link
Member Author

magcius commented Aug 19, 2014

So yeah, it sounds like both the RenderToMain and InterfaceStatusbar config options are set. Can you try disabling one of them?

@Sonicadvance1
Copy link
Contributor

RenderToMain disabled causes the FPS information to appear.

@magcius
Copy link
Member Author

magcius commented Aug 19, 2014

Are you OK with this workaround for now, or do you want me to fix this? It's an existing bug in Core...

@Sonicadvance1
Copy link
Contributor

It's a regression that your branch introduces.
RenderToMain enabled in master the titlebar in noGui still gets set.
Why allow a regression in from a PR?

@magcius
Copy link
Member Author

magcius commented Aug 19, 2014

Because the way it worked was bizarre, hacky, and was only really there for NoGUI. I'll fix it tomorrow, though, by moving it from Core to DolphinWX. I don't even know why we're checking RenderToMain / InterfaceStatusbar in Core...

@Sonicadvance1
Copy link
Contributor

LGTM aside from the FPS display nonsense.

Yes, this is a fancy new feature, but our Wayland support was
particularly bitrotten, and ideally this would be handled by a platform
layer like SDL. If not, we can always add this back in when GLInterface
has caught up. We might be able to even support wxWidgets and GL
together with subsurfaces!
Now, the only supported EGL platform is Android. We might eventually add
back support for EGL/X11 or EGL/Wayland, but it will have to be
architected differently.
Move to one display. There's no reason to have two displays here -- the
comment stated that one should touch GLX and one should touch window
events, and that they should be touched from different threads, but the
current code wasn't this careful.

Just use one Display connection.
Since we only use the toggle action, only keep that.
MainNoGUI is going to create its own window soon, and we need to
fullscreen that one instead of the GLX window.
Our existing code was relying on the GLX backend to create the GLX
window properly, and for the rest of the code to patch that up, sort
of. If we rely on Host_GetRenderHandle() returning a valid window, we
can do a lot better about this.

Create a simple window inside MainNoGUI to make this happen.
The only reason the GLX backend handled this at all was because
MainNoGUI didn't make its own window before. This is unused in DolphinWX
builds.
We now have two cases: the GLX window is parented into a frame, or it's
parented into the MainNoGUI host. In both cases, the GLX window should
be locked to the size of the parent, so just sync it up based on that.
Finally, it's unused. Whoa.
Now that MainNoGUI is properly architected and GLX doesn't need to
sometimes craft its own windows sometimes which we have to thread back
into MainNoGUI, we don't need to thread the window handle that GLX
creates at all.

This removes the reference to pass back here, and the g_pWindowHandle
always be the same as the window returned by Host_GetRenderHandle().

A future cleanup could remove g_pWindowHandle entirely.
This is effectively unused, as the window handles that we pass to the
GLInterface are window handles for the frame which isn't ever a real
toplevel window. Host_UpdateTitle is what actually sets the proper title
on the render window.
Everything is now safely tucked inside each individual GLInterface.
This causes flickering when resizing the window, which looks horrid
and we shouldn't do it.
Refactor the EGL backend to provide a platform separation here, which is
better abstracted away than the old EGL/X11 implementation.
We already put them on-screen, that should be good enough.
These don't really help anybody. We don't even have a status bar
in MainNoGUI -- status bar text should be controlled by the UI, not the
core code!
The concept of a "title bar" / "status bar" shouldn't be a core concept,
so remove the Host_UpdateStatusBar function, and move the code handles
whether to update the status bar or titlebar into DolphinWX.
@Sonicadvance1
Copy link
Contributor

LGTM.
Woops, apparently was rebased.

lioncash added a commit that referenced this pull request Aug 21, 2014
@lioncash lioncash merged commit f17dcd2 into dolphin-emu:master Aug 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants