Scaled EFB copy sliders for exclusions (extended from Pr/7533) #10337
Conversation
gordonfreeman01
changed the title
|
Updated @TryTwo's #7533 (which has the full discussion on this change) to work with latest Dolphin master branch. Also updated some coding conventions to fit more recent versions. I'm new to this so there may be some further changes needed, feedback naturally welcome. The change here adds the "Scaled EFB Copy Exclusions" setting box (with the "Don't scale EFB copy if" checkbox and a slider to tune the value) to the Options->Graphics Settings->Advanced tab (near the bottom below, I've already set a value here but by default is at the far right of the slider): This option is useful as discussed here to fix ghosting happening due to the way bloom is implemented in many Wii games when raising the Internal Resolution (IR) for modern screens (the higher the IR, the worse the problem). Here are some samples of the issue in Tron Evolution: Battle Grids: Before ("Don't scale EFB copy if" disabled): After ("Don't scale EFB copy if" enabled, slider set to 161): |
|
Please rerun the checks, the linter should clear the code now. |
|
Just going to mention it here as I've mentioned elsewhere, I'd like to see someone look into this in general more (in particular the downscaling that @phire mentions). I'm still surprised these effects are "broken" at higher IR. It just doesn't make sense to me. But I do know other emulators have also taken a similar approach (RPCS3 I believe has something similar) so maybe it isn't as buggy as I imagine. Still, I'd like a proper investigation before merging this. The 'solution' I've come up with is something similar to how Monster Hunter Tri was handled. Removing the game's native bloom (see #9779 ) and applying a high resolution bloom instead. However, it is more work than what is presented here (which while giving incorrect results, is close enough). |
|
Note that the large amount of fifoci differences are because it's looking at differences compared to #7533, instead of compared to master. There have been a lot of rendering changes since then, and you're seeing them all applied at once. Generally, it's better if you rebase instead of merge ( |
|
Thanks, will work on that and submit the changes (will likely take until the end of day as currently taken up). |
|
I found the current GUI design weird and not consistent with similar settings from the "Advanced" page. Considering that setting can be set to any value between 0 and 630 I also don't think an slider is a suitable approach here. Based on that, here's a mockup of how I think this should be presented on the GUI:
I went with "threshold" instead of "size" but both should fit. The current tooltip could also be split in two, one explaining what the setting does (for the check box) and other explaining the valid ranges (for the spin box)... |
|
Honestly I kind of prefer the slider in the previous UI. In this case there is a known maximum and minimum that relate to a ratio of EFB Copies being scaled, and a slider very intuitively shows that. Plus the way the previous UI use of "less than" is extremely useful in conveying what it is doing. The only downside is that it doesn't really fit visually with the rest there, but, this is the advanced menu, function over form works pretty well here. Of course the best of both worlds scenario would be a slider AND a value editing box. Can QT do that? |
gordonfreeman01
force-pushed
the
pr/7533
branch
from
5261641
to
3471f17
Compare
|
I've cleaned up the branch via a rebase, thanks @Pokechu22. I think I've used the co-author tag correctly - TryTwo is the original author with the majority of the actual work and commit messages seem to be correct although for this branch I set myself as author and TryTwo as co-author in the message thinking that GitHub might throw errors if I change that order (please let me know if this needs to be changed as I'm just maintaining TryTwo's change here). Regarding the slider, this setting behaves like a PhotoShop slider, you need to experiment with values for each game within a constrained range and visually observe the result (possible in real-time with the beautiful instant graphics setting changes in Dolphin). I therefore think it is the best solution for this although agree with @MayImilae that having a text box along with it would be ideal for further fine-tuning. For the help text, I checked some other slider options and there doesn't seem to be another setting where the explanation of the values and its need are as tightly coupled as this one. A user could go in thinking "hey, this value set so high must be good" (the reason why this is under Advanced and not Hacks on the original PR) if the explanation is not included on the checkbox and then get confused and angry when the game turns into a blurry mess because they need to tweak it down. The full explanation being both on the slider and the checkbox therefore makes sense to me. |
gordonfreeman01
force-pushed
the
pr/7533
branch
from
3471f17
to
975d5d6
Compare
|
Sorry for the accidental regressions, they're all fixed now although the build using code upto the PR just before this one seems to have introduced a bug in the DX12 renderer (any game I tried crashes, tested without this change to confirm). |
|
Yes. See PR #10341 |
gordonfreeman01
force-pushed
the
pr/7533
branch
from
975d5d6
to
aa0a4b0
Compare
|
Cool, tested and works on my end too, integrated into the branch for this PR. |
gordonfreeman01
force-pushed
the
pr/7533
branch
from
aa0a4b0
to
c9fd722
Compare
|
Missed one lint error which showed up in the checks, thought I'd set up the auto lint check correctly, need to fix that on my system, meanwhile the lint error found has been fixed. |
There was a problem hiding this comment.
While I want the bloom issues resolved as much as anyone, as my previous comment suggests, I'm skeptical that this hack is required. Much like how we didn't merge 3807 or 9473. I think we should do a more thorough investigation before merging this to make sure this is the only viable solution.
Merging this not only means we need game specific ini changes, it also means anyone interested in fixing this properly has less incentive to do so.
Looking at Xenoblade EFBs in Renderdoc, I'm reminded how difficult a problem this is to resolve generically. It pains me but maybe this is as good a solution as we will get.
|
Based on my writeup of bloom in Rygar, maybe something to try would be checking if the game has "Half scale" (previously labeled as "mipmap filter") enabled? If they're reducing the resolution during the EFB copy, it seems likely that it's going to be used for blurring, though I don't know if that'll apply to all games. |
|
@Pokechu22 I think this can be checked in Melee; does Fountain of Dreams use the half scale? The reflection in the pool is lower resolution, but I wonder if it's using the half scale thing to hide that the details are missing on some characters. |
|
Would it help if there was one issue regrouping Bloom problems in general? I found a few apparently disparate open items such as https://bugs.dolphin-emu.org/issues/12660 and https://bugs.dolphin-emu.org/issues/11443 but doesn't seem like there is a master list linking all of them (where each linked item could have the fifologs per game for example). |
|
Fountain of Dreams does not use half-scale; it just configures the viewport and scissor and then renders into an 80 by 60 pixel region as the first thing it does, before rendering the scene again at full resolution. Since I'm guessing they're using a lower resolution for performance reasons, rendering at a higher resolution and then downscaling would go against their purposes. At least, I think that's what's going on. I think a master issue covering the topic would be useful (using redmine's subtasks system). |
| // A &QSlider signal won't fire when game ini's trigger a change, due to a signalblock in | ||
| // GraphicsSlider | ||
| connect(m_scaled_efb_exclude_slider, &GraphicsSlider::valueChanged, [=] { | ||
| m_scaled_efb_exclude_label->setText(tr("Size < ") + |
There was a problem hiding this comment.
Instead of 'Size', 'Width' would be more accurate?
There was a problem hiding this comment.
Also this can be
m_scaled_efb_exclude_label->setText(tr("Size < %1").arg(m_scaled_efb_exclude_slider->value())
There was a problem hiding this comment.
Agree, thanks, implemented this here and in the LoadSettings function a few lines down. Based on the comment here looks like it was duplicated on purpose due to "A &QSlider signal won't fire when game ini's trigger a change, due to a signalblock in GraphicsSlider".
| @@ -317,6 +365,10 @@ void AdvancedWidget::AddDescriptions() | |||
| m_dump_xfb_target->SetDescription(tr(TR_DUMP_XFB_DESCRIPTION)); | |||
| m_disable_vram_copies->SetDescription(tr(TR_DISABLE_VRAM_COPIES_DESCRIPTION)); | |||
| m_use_fullres_framedumps->SetDescription(tr(TR_INTERNAL_RESOLUTION_FRAME_DUMPING_DESCRIPTION)); | |||
| m_scaled_efb_exclude_slider->SetDescription(tr(TR_SCALED_EFB_EXCLUDE_DESCRIPTION)); | |||
| m_scaled_efb_exclude_enable->SetDescription(tr(TR_SCALED_EFB_EXCLUDE_DESCRIPTION)); | |||
| // TODO: Create GraphicsLabel or do something fitting current Dolphin standards | |||
There was a problem hiding this comment.
No need for this TODO, we don't put tooltips on labels
| // EFB exclusions | ||
| GraphicsBool* m_scaled_efb_exclude_enable; | ||
| GraphicsSlider* m_scaled_efb_exclude_slider; | ||
| // TODO: Check if this type has a proper Dolphin class (e.g. GraphicsLabel) - doesn't seem so |
|
I'm going to trust iwubcode on whether this solution is reasonable or not. Otherwise, I think having something like this is probably fine. |
gordonfreeman01
force-pushed
the
pr/7533
branch
from
c9fd722
to
34b0ae2
Compare
|
I implemented the suggestions on the latest PR. Also, GitHub only has comment resolved, not resolve, then a separate close step to which I'm used to, so I accidentally set one comment to resolved then unresolved it. Please take a look. For completeness, from my perspective, I agree this is a stopgap solution but it at least gives users a pretty good mitigation to help resolve (albeit imperfectly) an issue making some games unplayable due to excessive ghosting. There are occasionally artifacts such as shimmering with this approach (as mentioned by TryTwo in the original thread) but it's much better than nothing. Until a permanent and fully correct solution is found (an event I sincerely hope for), I think including this option would help the more intrepid users (who else would dare use the Advanced tab?) have some relief. |
|
Just discovered something: PPSSPP seems to experience similar problems when upscaling the IR and has a setting called "Lower resolution for effects" which has some presets (safe, balanced and aggressive) and they fix the problems fairly well. I'll investigate and detail more later but this thread looked like the right place to mention this as this proposed setting seems to be a similar technique (at least on the surface). |
gordonfreeman01
force-pushed
the
pr/7533
branch
from
37b5c66
to
f0e3595
Compare
|
@JMC47 - I agree the UI is complex but it's less about the UI and more about the settings. Sonic Colors will likely be reported by users and it's possible that other games that also exhibit similar problems. Creating an improved heuristic to cover these sort of cases may end up requiring us to break users' current configuration because of how it was laid out. If we're ok with possibly breaking user configs in the future, I think this is nice for users to have over nothing at all. I'd still like to have a heuristic that targets the specific EFBs but I imagine most users won't care or possibly notice a difference. |
|
So, even if all the options aren't present in the UI, you're suggesting they exist (in INI/settings?) so that they can be set if necessary? |
|
@dolphin-emu-bot rebuild |
|
So I did some testing, and while this seems to work in the Basic Bloom case, there are some problems with some of the more complicated bloom setups. Metroid Prime Trilogy does the copy area on screen, meaning that when we do this EFB Exclusion stuff, the upper left corner of the screen gets turned "1x IR", but the exact same values are also what makes the bloom scale. There's no way to choose one or the other without some kind of more complicated setup, which I honestly don't know is possible. The old heuristics also didn't work for this game that were under development, so it may just not be fixable. Mario Kart Wii: Seemed to work fine for bloom. It uses the typical setup, so I don't think that's surprising. The GUI is pretty easy to use, but it makes our Graphics Tab super tall. I wonder if maybe we should make a new tab for some of these options? |
|
It's really useful in monster hunter tri. The bloom is still flickering a bit but tons better than before. |
|
@Narugakuruga - for Monster Hunter Tri look at this blog post. It details a possible way to handle bloom (remove it with a patch and then apply a post process effect). We could possibly apply some effects but this slider approach is very simplistic. |
|
So far it's the best solution for monster hunter tri . |
|
Due to the game specific nature of this option, I think it would be better to place this option in the game specific options only. This seems too custom and too powerful for the majority of the user. Besides, adjusting this in the main settings for a specific game means that for the next game there is a likelyhood that the values need to be changed. |
| @@ -2078,6 +2078,31 @@ void TextureCacheBase::CopyRenderTargetToTexture( | |||
| u32 tex_h = height; | |||
| u32 scaled_tex_w = g_renderer->EFBToScaledX(width); | |||
| u32 scaled_tex_h = g_renderer->EFBToScaledY(height); | |||
| bool EFBScaleExcludeTexture = false; | |||
|
|
|||
| if (g_ActiveConfig.iEFBExcludeEnabled && !is_xfb_copy) | |||
There was a problem hiding this comment.
On line 2115, there is another check if the copy should be upscaled or not which can be incorporated here.
There was a problem hiding this comment.
Thanks for the feedback, given that moving the check down to 2115 would allow scaleByHalf to override the base texture width and height, I just wanted to understand what that parameter actually means before pushing the change. Is scaleByHalf a part of standard Wii behaviour or a setting that EFBScaleExcludeTexture should force to ignore if true (unclear from a check for references)?
There was a problem hiding this comment.
It's part of standard Wii behavior, applied by games when they see the need to do so. If the texture is not scaled (due to EFBScaleExcludeTexture), scaleByHalf should still be applied (in this case, it would go from 1x IR to .5IR). If the texture is scaled, then scaleByHalf still applies (reducing from e.g. 8x IR to 4x IR), but perhaps it shouldn't.
There was a problem hiding this comment.
Thanks, update pushed :) EDIT: I accidentally missed the commit with the latest comment resolution, just repushed with the correct code, my apologies.
|
I have my own solution that I'm working on. It's less easy to use (in some ways) but imo the better way to go for reasons I stated in this PR (and some other reasons that will be obvious when it's posted). No ETA on when it'll be done but thought I'd mention it since this PR is getting more views again. |
…ing (bloom mitigation) added to Graphics->Advanced Author: Gordonfreeman01 <5954931+gordonfreeman01@users.noreply.github.com> Co-authored-by: TryTwo <taolas@gmail.com> Fix build issues for GUI of EFB Scale exclude options Fixed correct type for graphics options Restored TextureCacheBase.cpp and re-added iEFBExcludeEnabled function Added emphasis to default option in EFB scaling exclusion description text Removed unnecessary include of QSlider from AdvancedWidget.cpp (GraphicsSlider already included) Removed extra GFX_HACK_COPY_EFB_SCALED in GraphicsSettings.h, added EFBExclude defaults to VideoConfig.h Fix linter errors Fix linter error Restored accidentally reverted code Fix more regressions Fixed lint error PR suggestions implemented: Removed TODOs, refactored code to display m_scaled_efb_exclude_slider value Changed label for Scale EFB slider to Width in LoadSettings as well
|
Updated with the latest review comments. Of note, several games from different developers can have the same exclusion value, for example: Star Wars The Clone Wars Lightsaber Duels by LucasArts and Code Lyoko: Quest for Infinity by The Game Factory both need a width value of 81 to be excluded. Maybe there is some sort of standard library being used across titles which could potentially be targeted to fix this. EDIT: I accidentally missed the commit with the latest comment resolution, just repushed with the correct code, my apologies. |
…height and/or width added Enhanced UI layout, corrected descriptions and exclusion defaults set to 0 Use EFB_WIDTH and EFB_HEIGHT instead of more variables to define EFB scale exclusion slider limits Moved no scaling condition to after half-scale application
|
Hi thanks for keeping up with this PR. There was a forum post about optionally checking if the DST pointer is the same as the previous EFB copy before downscaling. It said it fixes Metroid and Sonic Colors. This would be more reliable than the height slider for Sonic. If you can't find anything else that needs a height slider, and isn't fixed by the dst check, I would remove it. I don't think we need to worry about accidentally downscaling non-bloom copies that have no affect on the final image quality. What's the difference between only downscaling bloom copies VS only upscaling EFB copies that improve the final image? I worry about trying to be overly accurate with tons of options, because games will change how they use the EFB copies depending on the level/area you are in, or in Sonic's case the angle of the camera. Also, while the style of blooming a scene is similar across games, the coding for it is wildly different between various games. |
The issue I have with this statement is that it's entirely dependent on the game. If you have three bloom copies you want to ignore, with this solution you pick the largest one. It's possible that a game can provide a copy that is big enough that it impacts the quality of other operations (as seen in Sonic Colors). I'd be ok with that if it seemed necessary but it seems like you can just target the individual copies and get the same effect. The dst pointer solution is very interesting but the forum topic seems to indicate it doesn't work on all games (I haven't tested with it personally). But talk is cheap. I wanted to mention my implementation which is #10518 . Touching up games isn't nearly as easy as a slider but the solution handles many more scenarios and I think dumping the efbs for the texture name to copy into a file isn't too terrible for content creators to do. |
|
Your work definitely adds a lot of cool stuff you can do. I haven't looked into tagging bloom copies enough to know how reliable it is for that though. I'll try to play around with it later. Both solutions rely on there being a finite number of games, each setup and tested for compatibility with the solution. Everything is game dependent. It's uncertain if the ID tag method will work correctly with all games, and I don't know if specific areas of games could have special effects that overlap, but if the overlap is always less than what the slider would produce, that's an improvement. So long as it doesn't miss. Does the MKWii heat haze effect get caught with the bloom or break detection? If tagging works and someone wants to go through and ID bloom in all the games, I'm all for it. Right now, the slider solution is easy and widely tested, and works for most games. I think the outliers have a fix with the dst pointer being used as an additional, optional filter. |
Yeah, definitely don't want to introduce a solution that only works part of the time or misses some values. I do think anything you can do with the slider approach, you can do with the tag approach. The slider has some advantages since it is a specialized solution but having a standardized system that works for a variety of applications is really powerful.
Yes it does. I compared against your reference photo in the original forum topic. Seems to be fairly similar.
It can, it is entirely depends on the game. Mario Kart for instance has this weird shadowy effect at the edge of the scene and while the Last Story generally works, it does seem to impact cutscenes in a strange manner. For other games I haven't noticed any issues.
It is the count of the number of copies. The current Dolphin outputs that in the filename. This keeps files unique for users wanting to analyze the efbs but isn't used by the efb targeting system.
Right now I've tested a number of the games people have complained about on the forums:
I still have yet to test:
I do not own:
For metroid, I have a fifolog that I will try testing later on. |
|
I haven't got graphics mod working yet, but I can manually build dolphin with the exclusions.. Metroid is 320, 224, RGB565 and does correctly skip the top-left corner EFB, which is RGBA8. Monster hunter seems to only do bloom with efb copies, and needs testing to see what produces the best result. It's probably better to remove bloom though, so I'd need to get the GraphisMod working. |
Yes, it does.
Yeah for games that don't behave as well, we can possibly introduce a separate action.
I just tried Monster Hunter Tri (Demo). It's interesting. Enabling the bloom removal or the bloom at native at runtime removes the face texture. Doing it before the game starts works fine. Disabling the check at runtime doesn't return the face texture, so this one could a quirk of the demo (or possibly a bug on our end?). Will have to look at it more. EDIT: Ah, I bet this is less about the efbs being miss-detected and more about the invalidation of the texture cache removing a texture that the game expects to be available.. |
|
Wow!, I finally managed to inject a blur shader into the EFB pipeline and, when applied to only upscaled bloom copies, it fixes things amazingly well. No shimmering issue in mario kart wii or metroid. The upscaled efb copies look properly bloom-y, because they have enough blur to mix the pixels together. Unfortunately I majorly hacked it together and don't know the proper way to make a blur pass on an EFB copy. So if anyone knows how to setup a custom shader pass in texturecachebase for incremental efb copies, that'd be really helpful. |
Yes, you use my post processing feature as an action to the graphics mod feature :P. But yeah, all that time I said I was working on something semi-related, I was working on the post processing system with graphics triggers. My hope was that a blur or downsampling shader could be applied and that would fix the issue. Great to hear you confirmed that! |
|
https://imgur.com/a/OLBl78X I made an image pack to compare the blur injection. @iwubcode I assume "post processing" is after the entire image has been finished, right? Or did I misunderstand. |
|
@TryTwo - when you think of a post processing system (ex: Reshade), that is what you'd think of typically (post processing the final image with some effects). My post processing PR allows the user to post process the current color frame buffer at any given point. This allows you to do effects before certain images are drawn (ex: the HUD) |
|
Oh that's way more amazing than I fully realized. I'll ask you questions on that PR. |
|
Decided to update my old Bloom PR with proof of concept Blurring. |
|
Given graphics mods are now merged, I think this can be closed. Any users wanting to use this sort of interface can view TryTwo''s PR for which a build is provided. |
Merged #7533 to dolphin master and resolved merged conflicts, updated some code conventions to fit more recent versions