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 inbuilt Opengl renderer to window #922

Merged
merged 14 commits into from Feb 12, 2020
Merged

Conversation

emmauss
Copy link
Contributor

@emmauss emmauss commented Feb 6, 2020

We can now ditch the extra window. Allows rendering in the main window. the old GLScreen is still there, but can not be accessed anymore through. Will remove it if its decided its no longer necessary.
So far, I've not noticed any slowdowns. the rendering logic is more or less the same, and its multithreaded. Contrary to what I thought before, you can modify the underlying context in gtk, with it crashing on you.

This PR is ready for review.

Screenshot_2020-02-06 Screenshot from 2020-02-06 11-38-33 png (PNG Image, 1920 × 1080 pixels) - Scaled (83%)
Screenshot_2020-02-06 Screenshot from 2020-02-06 11-38-52 png (PNG Image, 1920 × 1080 pixels) - Scaled (83%)

@emmauss
Copy link
Contributor Author

emmauss commented Feb 6, 2020

as an aside, code for the widget is here, https://gist.github.com/emmauss/15fef952fe25d7f838ccd6a5f438249b

@AcK77 AcK77 added enhancement New feature or request gui Related to Ryujinx.Ui labels Feb 6, 2020
Copy link
Member

@AcK77 AcK77 left a comment

Choose a reason for hiding this comment

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

Juste some nits. It could be nice to leave some comments in the code too.

Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/MainWindow.cs Show resolved Hide resolved
Thog and others added 3 commits February 6, 2020 14:36
Changelog:

- Reapply some changes that got lost while rebasing from Ryujinx#904
- Make sure to guarantee exclusivity on the GL context (fixing multiple
possible race conditions on Windows)
- Avoid making GLRenderer disposed multiple time
@emmauss
Copy link
Contributor Author

emmauss commented Feb 6, 2020

addressed comments

Copy link
Contributor

@marysaka marysaka left a comment

Choose a reason for hiding this comment

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

lgtm

@gdkchan
Copy link
Member

gdkchan commented Feb 6, 2020

with it crashing on you.

I guess this is a typo?

the old GLScreen is still there, but can not be accessed anymore through. Will remove it if its decided its no longer necessary.

I think it should be removed.

Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Show resolved Hide resolved
Ryujinx/Ui/GLRenderer.cs Show resolved Hide resolved
Ryujinx/Ui/MainWindow.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@Xpl0itR Xpl0itR left a comment

Choose a reason for hiding this comment

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

  • Instead of hiding the game list and games loaded info separately you should just remove the whole MainBox (both the game list and footer are in this box widget). Its currently not exposed to the code, you'll have to add its binding.
  • nit: If you open MainWindow.glade in glade you will see that the footer box has spaces for 4 child widgets but only has 3 child widgets.
  • The custom gl widget has some random console logging. https://gist.github.com/emmauss/15fef952fe25d7f838ccd6a5f438249b#file-glwidget-cs-L278
  • Resizing and fullscreening the window is bugged if the window is not being rendered to

Ryujinx/Ui/ScopedGLContext.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/MainWindow.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/MainWindow.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/MainWindow.cs Outdated Show resolved Hide resolved
Ryujinx/Ui/MainWindow.cs Outdated Show resolved Hide resolved
@emmauss
Copy link
Contributor Author

emmauss commented Feb 7, 2020

* nit: If you open MainWindow.glade in glade you will see that the footer box has spaces for 4 child widgets but only has 3 child widgets.

Done

* The custom gl widget has some random console logging. https://gist.github.com/emmauss/15fef952fe25d7f838ccd6a5f438249b#file-glwidget-cs-L278

Will remove it in an update to the wigdet package

* Resizing and fullscreening the window is bugged if the window is not being rendered to

Noting I can do about this. I've tried clearing the screen after resizing. it did not work.

Addressed comments

Copy link
Member

@gdkchan gdkchan left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request gui Related to Ryujinx.Ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants