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

citra_qt, video_core: Screenshot functionality #4164

Merged

Conversation

Projects
None yet
@zhaowenlan1779
Copy link
Member

zhaowenlan1779 commented Aug 31, 2018

Closes #3121

Allows capturing screenshot at the current internal resolution (native for software renderer), but a setting is available to capture it in other resolutions (the internal resolution won't be changed, instead the image captured is upscaled/downscaled). The screenshot is saved to a single PNG in the current layout.

Examples of captured images can be found in the #screenshots-videos channel in Citra discord (they were captured with an older version of the code, but I have confirmed the result is the same). This is one of them.

A menu action is added to the Movie menu (I believe this is the most suitable place) as well as a hotkey. The hotkey is bound to Ctrl+S by default.

I have just started to learn GL stuff, and there may be easier ways to achieve what I'm doing in renderer_opengl.cpp. If that's the case, please point it out. Thanks!

Screenshot:
image


This change is Reviewable

@zhaowenlan1779 zhaowenlan1779 requested a review from jroweboy Aug 31, 2018

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:brand-new-screenshot branch from 7f8aa69 to 3b1bcaa Aug 31, 2018

@FearlessTobi

This comment has been minimized.

Copy link
Contributor

FearlessTobi commented Aug 31, 2018

As for every UI change, can you please provide a screenshot for every changed dialog/setting?

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Aug 31, 2018

@FearlessTobi huh.. I didn't think it was necessary as it is just a simple menu action. But fine :P added it to the description.

@j-selby

This comment has been minimized.

Copy link
Contributor

j-selby commented Aug 31, 2018

Can I suggest renaming the Movie option to something more general? "Capture"? Not sure I like the upscaling/downscaling thing.

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Aug 31, 2018

@j-selby "Capture" does not sound so good... I cannot think of a better one right now though.
Also, the setting does not exist in the UI as I think it would cause confusion if the image captured became pixelated due to upscaling. Normally the image captured would just be at the internal resolution.

@B3n30

This comment has been minimized.

Copy link
Contributor

B3n30 commented Aug 31, 2018

TBH I don't think screenshot should be a sub-option of movie. "Movies" in emulators are known to be input-recordings. And IMO screenshots have not very much in common with such movies.

Maybe do what @jroweboy suggested. Make a option tools and inside tools there is the movie submenu and the screenshot function?

@ghost

This comment has been minimized.

Copy link

ghost commented Aug 31, 2018

Issues:

  • Game is not resumed if you cancel the dialog.
  • Colors issue with FBI (screenshot from my latest commit + this pr + some other changes):
    issue
    edit: ignore this issue, the color seems to be different now.
@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Aug 31, 2018

Color looks correct to me. I believe the blue one was correct?
image

@BreadFish64

This comment has been minimized.

Copy link
Contributor

BreadFish64 commented Aug 31, 2018

The blue is correct yes

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:brand-new-screenshot branch from 1ceb5d4 to 902b0f2 Aug 31, 2018

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Aug 31, 2018

Fixed the first issue.

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Aug 31, 2018

The issue @valentinvanelslande has is probably with the image viewer on linux which does some "color correction". I had the same issue on ubuntu.

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:brand-new-screenshot branch from 7138d1a to 3652e86 Sep 1, 2018

@B3n30

This comment has been minimized.

Copy link
Contributor

B3n30 commented Sep 3, 2018

I didn't make a full review but doesn't that introduce some potential race conditions between the qt_thread and the emulation (video-core) thread?

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Sep 3, 2018

@B3n30 qt thread only sends a request to emu thread (done by an atomic bit) and the real capture-and-save operation is done entirely in the emu thread.

void RequestScreenshot(void* data, std::function<void()> callback,
const Layout::FramebufferLayout& layout) {
if (g_renderer_screenshot_requested) {
LOG_ERROR(Render, "A screenshot is already requested or in progress, ignoring the request");

This comment has been minimized.

Copy link
@baka0815

baka0815 Sep 3, 2018

Contributor

Does this need to be an error? Isn't a warning more appropriate here?

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Sep 8, 2018

Author Member

Not sure about that. IMO this should be an error, as a user request is ignored.

Show resolved Hide resolved src/video_core/renderer_opengl/renderer_opengl.cpp

@wwylele wwylele added canary-merge and removed canary-merge labels Sep 3, 2018

@Leo626

This comment has been minimized.

Copy link

Leo626 commented Sep 4, 2018

I noticed Ctrl + S is the same hotkey for Toggle Status Bar.

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Sep 8, 2018

@Leo626 Thanks for reporting. Will change the hotkey to something else...

@wwylele wwylele added canary-merge and removed canary-merge labels Sep 30, 2018

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:brand-new-screenshot branch from 3652e86 to 046211a Nov 11, 2018

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Nov 11, 2018

Rebased. now let's try tagging this...

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Nov 17, 2018

This PR is now ready for review. CI failure will be fixed soon

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:brand-new-screenshot branch from 0ee4a17 to a21f2da Nov 18, 2018

@adityaruplaha
Copy link
Contributor

adityaruplaha left a comment

LGTM

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Nov 22, 2018

I'll merge in 24 hours if no more comments

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Nov 22, 2018

Oh wait, I still need to change the hotkey.

@bunnei

bunnei approved these changes Nov 24, 2018

@RocketRobz

This comment has been minimized.

Copy link

RocketRobz commented Nov 24, 2018

A minor request: How about an option to set a resolution specific to screenshot captures?
For me, setting the internal resolution to 10x makes the game run a bit slow, and setting it just for hi-res screenshots, and then back to the previous setting for regular usage, can get pretty tiresome.

I'm thinking of having the resolution list shown when hovering over Capture Screenshot.

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Nov 24, 2018

there's already a config entry for it. I can add that to the UI if it's needed.
I think your way is probably better than mine though...

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 29, 2018

Will merge this soon if no more comments

@BreadFish64

This comment has been minimized.

Copy link
Contributor

BreadFish64 commented Nov 29, 2018

I think the screenshot capture resolution warrants a UI option

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Nov 29, 2018

yeah, will add that tomorrow

zhaowenlan1779 added some commits Aug 31, 2018

citra_qt, video_core: Screenshot functionality
Allows capturing screenshot at the current internal resolution (native for software renderer), but a setting is available to capture it in other resolutions. The screenshot is saved to a single PNG in the current layout.
citra_qt: rename the menu to Tools
Also made Movie a sub-menu of Tools.

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:brand-new-screenshot branch from a21f2da to 5a3eb22 Nov 30, 2018

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Nov 30, 2018

Added resolution selection and changed hotkey to CTRL+P.

@Narugakuruga

This comment has been minimized.

Copy link

Narugakuruga commented Nov 30, 2018

This option seems to only change capture resolution but not rendering resolution.

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Nov 30, 2018

@Narugakuruga ah, that's right. I'll fix it soon

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:brand-new-screenshot branch from 5a3eb22 to 1970178 Dec 1, 2018

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Dec 1, 2018

@RocketRobz @Narugakuruga
Sorry, due to technical reasons this is actually harder than it sounds and would require dirty hacks currently. I have dropped the last commit. People are welcome to reinvent it in the future when this becomes easier.

PR should be good to merge now.

@Narugakuruga

This comment has been minimized.

Copy link

Narugakuruga commented Dec 1, 2018

I understood, this option is actually not really necessary, people can change rendering resolution in graphics configure manually. Thanks your attempt anyway.

@zhaowenlan1779

This comment has been minimized.

Copy link
Member Author

zhaowenlan1779 commented Dec 1, 2018

will merge this soon if no more comments

@zhaowenlan1779 zhaowenlan1779 merged commit 58b24b9 into citra-emu:master Dec 2, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/bitrise/4ccd8e5720f0d13b/pr Passed - citra
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zhaowenlan1779 zhaowenlan1779 deleted the zhaowenlan1779:brand-new-screenshot branch Dec 2, 2018

chris062689 pushed a commit to yuzu-emu/yuzu-nightly that referenced this pull request Dec 23, 2018

Merge pull request #1886 from FearlessTobi/port-4164
Port citra-emu/citra#4164: "citra_qt, video_core: Screenshot functionality"
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.