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

[WIP] Re-add the monitor resolution mode setting in the Advanced tab #6208

Closed
wants to merge 2 commits into from

Conversation

JosJuice
Copy link
Member

This reverts PR #6196.

We've been getting complaint comments on that PR unusually quickly compared to other feature removal PRs (such at fractional IR). While I don't agree with all the points they make, the one about resolutions that can't run over 30 Hz makes sense.

@Helios747
Copy link
Contributor

Helios747 commented Nov 19, 2017

No. 4k 30hz is such a small number of users that they can use the INI option I specifically left there.

Additionally, all technical users will moan about losing options not really being aware (or caring) that having a ton of options that the vast majority of users don't need to touch just confuses people.

@JosJuice
Copy link
Member Author

I think you're overestimating how confused people got by the setting back when we had it.

@Helios747
Copy link
Contributor

I'd be ok with moving it to advanced tab though

@JosJuice
Copy link
Member Author

@MayImilae Are you okay with putting it in the Advanced tab?

@JosJuice
Copy link
Member Author

Maybe we could also rename the GUI label to something like "Screen Mode" so that people will be less attracted to adjusting this setting instead of the IR setting.

@mbc07
Copy link
Contributor

mbc07 commented Nov 19, 2017

There'll always be complains on feature removals, and on this particular case, the feature still is available through the INI, it's just not exposed on the UI anymore. Said that, I don't think the PR should be reverted...

@DaRkL3AD3R
Copy link

I have to agree with @JosJuice , I think you might be overestimating user confusion on this topic @Helios747 . I'm fairly active on the forums and I can't think of any posts in my 8 years on there where a user mistook the fullscreen resolution for IR. I'm not saying it hasn't happened, but I really think it's added "difficulty" is being overblown here. It is a very functional, neat setting that I truly don't believe causes any harm.

If moving it to another tab would placate anyone who is against keeping it, I am all for that.

@MayImilae
Copy link
Contributor

MayImilae commented Nov 19, 2017

So in the previous PR someone brought up users using DVI on 4k displays (topping out at 4k30) wanting to run Dolphin at 1080p60. That's a really, REALLY niche usecase! And quite frankly, connecting your screen up properly is the answer to that, not hacking around it with software controls. Almost any display with 4k capability will have either proper HDMI or DisplayPort that can support 4k60, or will allow using two cables to reach 4k60. The most common issues I found when searching for this was user error, or being surprised that an old video card didn't work with 4k60.

Also I certainly noticed that the user who brought it up did so theoretically - they didn't actually experience the issue.

For the vast majority of uses, Fullscreen Resolution was an option that users didn't need to touch, but touched regardless because a lot of games have taught them to change it! This has resulted in weird forum threads, dumb user configurations, and bad advice from confused users. This was bad UX. In the extremely rare case where fixing a users connection or a video card upgrade won't solve a users problem, we have the GameINI setting still. I think we're covered on this and we do not need to revert the original PR.

@JosJuice
Copy link
Member Author

Okay, I'll close this PR then. If anyone has anything more to add, feel free to comment despite the PR being closed.

@JosJuice JosJuice closed this Nov 19, 2017
@M-a-r-k
Copy link

M-a-r-k commented Nov 21, 2017

Some older 4K screens only work at 30Hz at native res. And older GPUs/systems may not have 4K/60-capable outputs (e.g. my Dell laptop docking station has DVI, no DisplayPort).

But really, I'd like to see more configurability for the full-screen mode. Allow the user to select resolution, refresh rate and which monitor to display on. And even an option to blank other monitors when in full-screen mode could be useful for people with multiple displays.

I quite often change full-screen resolution from game to game, dependent on internal resolution setting. For "heavier" titles I reduce IR to 3× and reduce full-screen resolution too, to avoid wasting GPU resources on scaling to 4K. (There is surely some GPU overhead if the driver has to scale up to 4K/5K/8K native output resolution.)

@DaRkL3AD3R
Copy link

I absolutely agree with the notion that there should be ^more^ options. How many games do you want to play that don't have the ability to choose which monitor you want to play on? Or refresh rate? These, and resolution, are options that should be absolutely baseline for all PC gaming applications. Very disappointed to see this direction the emulator is taking.

@MayImilae
Copy link
Contributor

MayImilae commented Nov 21, 2017

Remember that you can still control the fullscreen resolution through INIs. The option isn't gone, it's just not as convenient - and thus harder to use if you don't know what you are doing. All these theoretical and extremely rare use cases are all still covered.

Also there is no performance drop for using the GPU scaler, and Dolphin's rendering resolution is solely controlled by the Internal Resolution option as it always has been.

@Helios747
Copy link
Contributor

Helios747 commented Nov 22, 2017

Again. UX is important. Having a "resolution" option that isn't IR front and center when you open the graphics UI is really dumb and I've seen it confuse users a fair bit on the forums.

When the people who actually need that option are less than a single % of our userbase, yeah I'm going to care about improving the UX for the rest.

And on top of that, just like May said. INI still exists, and can be placed in dolphin.ini.

Additionally, GPU upscaling is trivial. This doesn't affect GPU load.

@DaRkL3AD3R
Copy link

What about refresh rate control? Monitor choice? Often I will play at native IR, 640x480 60hz and want to push Dolphin over to my CRT TV. The only way to get it to go into exclusive fullscreen on the TV is to change my Primary display to the TV and then launch the emulator. This is obviously not ideal and annoying since my primary right now is a 1440p 165Hz display.

This is just an example of a very real situation that I face regularly, that would be totally alleviated by having hard control over these parameters. Ideally every application should, as a baseline, allow control over these specific 3 settings. I will never agree with this dumbing down and simplification of the control aspect over such basic features.

I think it's clear at this point the direction the emulator is going. Sad to see it, but it is what it is.

@Helios747
Copy link
Contributor

Refresh rate control, how would that be helpful? Almost all games run at 30 or 60 frames. There are only a small few which run at a variable framerate.

Monitor selection shouldn't be needed. Drag the window over to the display you want, hit fullscreen. It should fullscreen there. Perhaps this doesn't work if you're using D3D+Exclusive FS? Not sure.

I can see more of a case for monitor selection than refresh rate selection. (frontends and automation)

Sorry you don't agree, you're free to fork dolphin without this change though.

@cammelspit
Copy link

cammelspit commented Nov 22, 2017

I have a mixed multi-monitor setup too. Respectfully, why would you remove something that is useful and hide it behind INI files when it was already there? Also, I think it is pretty obvious that a lot of people use that feature or at least have had need of it at some point. If it isn't harming anyone, why not leave it there? If it makes sense, put it under a different heading like advanced with a tooltip letting people know it is not IR. Make an auto default that uses the current screen resolution, which is obviously good enough for the majority of users, but still allowing those of us that actually use it to have slightly more convenient use of it aside from manually editing INI files. Manual INI tweaking should be avoided at all costs if there is any possible way to do so. UX isn't going to suffer if it is left there and is only going to suffer if it left removed. I love the project and all the work, especially recently, being done but this is just not a direction I feel is good to go.

Thank you for your time.

@DaRkL3AD3R
Copy link

Refresh rate was never an issue, until I upgraded my 60hz monitor.

And multi-monitor control does not work as you explained. It always chooses my primary display when entering fullscreen. I use Vulkan, but the same behavior is present in OpenGL and D3D as well.

@bb010g
Copy link
Contributor

bb010g commented Nov 26, 2017

@DaRkL3AD3R Looks like the last issue on this on the bug tracker was 1433. If you could open a new issue for fullscreen always going to your primary, that would be great.

@dolphin-emu dolphin-emu deleted a comment Nov 27, 2017
@JosJuice
Copy link
Member Author

JosJuice commented Nov 27, 2017

Reopening.

Compared to other recent removals such as fractional IR, this change has generated much more backlash (I don't agree with all of the reasons people have for using the setting, but some of them do make sense), has essentially no benefit for maintenance (since the setting still can be used through the INI) and does not impact the emulation stability or accuracy at all (the worst thing that can happen is that the resolution gets decreased to... well, exactly what the user entered!). The reason I LGTMed the removal was because I thought that basically nobody found it useful, but that has been proven wrong by comments here and in the now locked forum topic.

Just reverting the removal is going to re-introduce the problem that new users might think that adjusting the fullscreen resolution is enough to get high-res gameplay, making them not look for the internal resolution setting. So I've added a commit that moves the setting to the Advanced tab and puts it behind a checkbox, making it look more like something you normally shouldn't adjust:

override-monitor-mode-advanced

I tried to design it so that if we later on want to add an option to force the selection of a specific monitor of a multi-monitor setup, it would just require the addition of a new label+dropdown. Maybe another design would be better, though?

I have not implemented this in DolphinQt2 yet, so this is marked WIP for now. I'll probably hold off with implementing it until I've received enough feedback to know what chances this has of getting merged.

@JosJuice JosJuice reopened this Nov 27, 2017
@JosJuice JosJuice changed the title Revert "[UI] Remove fullscreen resolution UI." [WIP] Re-add the monitor resolution mode setting in the Advanced tab Nov 27, 2017
@DaRkL3AD3R
Copy link

Thanks, that will go a long way in making things easier for those of us who regularly used that setting. Also greatly looking forward to the display selection option in the future. Much appreciated.

@JosJuice
Copy link
Member Author

Also greatly looking forward to the display selection option in the future.

I said if we want to add that. I don't have any plans on adding it personally, and I can't speak for what anyone else wants to do.

@Helios747
Copy link
Contributor

I'm fine with putting it in advanced.

@Helios747
Copy link
Contributor

I could see arguing for putting in misc, but I don't care too much

@JosJuice
Copy link
Member Author

I tried to put it in Misc at first, but putting the new checkbox to the right of Borderless Fullscreen made the window super wide, and putting it below Borderless Fullscreen would look awkward since there would be empty space to the right of both Borderless Fullscreen and the new checkbox.

@Helios747
Copy link
Contributor

Helios747 commented Nov 27, 2017

Fair enough. Then no objections here. It removes the UI option from the main landing tab which is my biggest issue. I'm still for removing useless options when possible because we have far too many of those but this one is pretty harmless.

The backlash on this was pretty minimal though tbf. One person Mad Online for an extended period of time isn't what I would care about with backlash. Nothing compared to nuking D3D12

@JosJuice
Copy link
Member Author

Yeah, D3D12 was significantly bigger. But that one also reduced the maintenance load a lot more :)

@cammelspit
Copy link

I am very glad you guys decided to reopen this. I for one will be one of those people who are very happy to see this put back and now, it won't be confused with IR or actual render resolution.

THANKS!

P.S. DX12 was a pain to use and never really worked properly anyway so I think of it just like DX9, too messed up to be worth using and anything to help the project overall. Just my opinion. :P

@JosJuice JosJuice added the WIP / do not merge Work in progress (do not merge) label Nov 27, 2017
@DaRkL3AD3R
Copy link

There a particular reason this one isn't merged yet? Not satisfied with the code? Or is it just disapproval from other developers?

@leoetlino
Copy link
Member

Closing as this is not ready and hasn't been updated in a while.

@leoetlino leoetlino closed this Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP / do not merge Work in progress (do not merge)
9 participants