little changes on AF value behavior. #754

Merged
merged 2 commits into from Aug 21, 2015

Projects

None yet

4 participants

@ssakash
Member
ssakash commented Aug 9, 2015

No description provided.

@turtleli
Member

AF commit means you can't turn off AF in the GUI for Linux.

@ssakash
Member
ssakash commented Aug 11, 2015

@turtleli
AF doesn't have a checkbox for linux ? either way, since the majority of people vote for removing checkbox instead of additional drop down list option I'll be doing that.

@turtleli
Member

It doesn't.

@gregory38
Contributor

of course you need to replace all uses of the useless checkbox value. Here a quick list.

./GSTextureFX9.cpp:         ss->Anisotropic[0] = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_LINEAR;
./GSTextureFX9.cpp:         ss->Anisotropic[1] = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_POINT;
./GSDevice9.cpp:D3DTEXTUREFILTERTYPE LinearToAnisotropic = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_LINEAR;
./GSDevice9.cpp:D3DTEXTUREFILTERTYPE PointToAnisotropic = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_POINT;
./GSDeviceOGL.cpp:  if (GLLoader::found_GL_EXT_texture_filter_anisotropic && !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0)) {
./GSLinuxDialog.cpp:    theApp.SetConfig("AnisotropicFiltering", 1);
./GSTextureFX11.cpp:    sd.Filter = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3D11_FILTER_ANISOTROPIC : D3D11_FILTER_MIN_MAG_MIP_POINT;
./GSTextureFX11.cpp:            af.Filter = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3D11_FILTER_ANISOTROPIC : D3D11_FILTER_MIN_MAG_LINEAR_MIP_POINT;
./GSDevice11.cpp:   sd.Filter = sd.Filter = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3D11_FILTER_ANISOTROPIC : D3D11_FILTER_MIN_MAG_MIP_LINEAR;
./GSDevice11.cpp:   sd.Filter = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3D11_FILTER_ANISOTROPIC : D3D11_FILTER_MIN_MAG_MIP_POINT;
./GSSettingsDlg.cpp:    CheckDlgButton(m_hWnd, IDC_ANISOTROPIC, theApp.GetConfig("AnisotropicFiltering", 0));
./GSSettingsDlg.cpp:            theApp.SetConfig("AnisotropicFiltering", (int)IsDlgButtonChecked(m_hWnd, IDC_ANISOTROPIC));

Code need to depends on MaxAnisotropy instead. Jobs will be done if AnisotropicFiltering dissapears completely.

@ssakash
Member
ssakash commented Aug 13, 2015

done 👍 , please refer to the commit messages for information.

@turtleli turtleli and 2 others commented on an outdated diff Aug 13, 2015
plugins/GSdx/GSDevice11.cpp
@@ -307,7 +307,7 @@ bool GSDevice11::Create(GSWnd* wnd)
memset(&sd, 0, sizeof(sd));
- sd.Filter = sd.Filter = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3D11_FILTER_ANISOTROPIC : D3D11_FILTER_MIN_MAG_MIP_LINEAR;
+ sd.Filter = sd.Filter = theApp.GetConfig("MaxAnisotropy", 1)!=1 && !theApp.GetConfig("paltex", 0) ? D3D11_FILTER_ANISOTROPIC : D3D11_FILTER_MIN_MAG_MIP_LINEAR;
@turtleli
turtleli Aug 13, 2015 Member

You should remove the extra sd.Filter =.
I think theApp.GetConfig("MaxAnisotropy", 1) > 1 would be clearer.

@ssakash
ssakash Aug 13, 2015 Member

this would be better than changing the value of m_gs_max_anisotropy.push_back(GSSetting(1, "1", "off"));. to m_gs_max_anisotropy.push_back(GSSetting(0, "Off", "")); since 0 is not a valid range at this condition and won't be accepted even if users customize through the ini. any opinions ? @gregory38

@gregory38
gregory38 Aug 13, 2015 Contributor

I think it is more readable to change m_gs_max_anisotropy range value. It is more logical to have it off at 0 rather than 1.

@turtleli turtleli commented on an outdated diff Aug 13, 2015
plugins/GSdx/GSDeviceOGL.cpp
@@ -572,7 +572,7 @@ GLuint GSDeviceOGL::CreateSampler(bool bilinear, bool tau, bool tav)
gl_SamplerParameterf(sampler, GL_TEXTURE_MIN_LOD, 0);
gl_SamplerParameterf(sampler, GL_TEXTURE_MAX_LOD, 6);
- if (GLLoader::found_GL_EXT_texture_filter_anisotropic && !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0)) {
+ if (GLLoader::found_GL_EXT_texture_filter_anisotropic && theApp.GetConfig("MaxAnisotropy", 1)!=1 && !theApp.GetConfig("paltex", 0)) {
@turtleli
turtleli Aug 13, 2015 Member

Check is unnecessary here.

@turtleli turtleli commented on an outdated diff Aug 13, 2015
plugins/GSdx/GSSettingsDlg.cpp
@@ -213,10 +212,6 @@ bool GSSettingsDlg::OnCommand(HWND hWnd, UINT id, UINT code)
if (code == BN_CLICKED)
UpdateControls();
break;
- case IDC_ANISOTROPIC:
- if (code == BN_CLICKED)
- UpdateControls();
- break;
@turtleli
turtleli Aug 13, 2015 Member

Need to add a case IDC_PALTEX: here.

@turtleli
Member

Oh. Browser didn't refresh, didn't see @gregory38 replied. Sounds goods to me.

@ssakash
Member
ssakash commented Aug 13, 2015

Updated with m_gs_max_anisotropy.push_back(GSSetting(0, "Off", "")); as the option for disabling AF in the drop down list and all those other requested stuffs.

@turtleli
Member

You didn't compile/test your changes. And don't add trailing whitespace.

@ssakash
Member
ssakash commented Aug 13, 2015

nope, I haven't finished it yet. still need to add a commit for reusing the GSDX API functions instead of string duplication for crc hack level. I'll test the changes after finalizing stuffs.

@turtleli
Member

I've done the CRC in one of my branches. So don't bother doing that.

@ssakash
Member
ssakash commented Aug 13, 2015

@turtleli
Thanks, then my PR is complete. :)

PR tested and works as intended with the changes. 👍

@gregory38 gregory38 commented on an outdated diff Aug 17, 2015
plugins/GSdx/GSDeviceOGL.cpp
@@ -572,9 +572,10 @@ GLuint GSDeviceOGL::CreateSampler(bool bilinear, bool tau, bool tav)
gl_SamplerParameterf(sampler, GL_TEXTURE_MIN_LOD, 0);
gl_SamplerParameterf(sampler, GL_TEXTURE_MAX_LOD, 6);
- if (GLLoader::found_GL_EXT_texture_filter_anisotropic && !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0)) {
+ if (GLLoader::found_GL_EXT_texture_filter_anisotropic && (int)theApp.GetConfig("MaxAnisotropy", 0)>1 && !theApp.GetConfig("paltex", 0))
@gregory38
gregory38 Aug 17, 2015 Contributor

It would be better this way:

int anisotropy = theApp.GetConfig("MaxAnisotropy", 0);
if (GLLoader::found_GL_EXT_texture_filter_anisotropic && anisotropy && !theApp.GetConfig("paltex", 0))
Then correct the indentation of the code

@gregory38 gregory38 commented on an outdated diff Aug 17, 2015
plugins/GSdx/GSSettingsDlg.cpp
if (code == BN_CLICKED)
UpdateControls();
- break;
+ break;
@gregory38
gregory38 Aug 17, 2015 Contributor

white space issue

@gregory38
gregory38 Aug 17, 2015 Contributor

Actually we can merge IDC_PALTEX with above code. Do you agree with me?

@gregory38 gregory38 commented on the diff Aug 17, 2015
plugins/GSdx/GSLinuxDialog.cpp
@@ -560,8 +560,6 @@ bool RunLinuxDialog()
theApp.SetConfig("ModeWidth", mode_width);
theApp.SetConfig("msaa", 0);
theApp.SetConfig("windowed", 1);
- // Anisotropic is disabled when it is 1x, no need of an extra check box
@gregory38
gregory38 Aug 17, 2015 Contributor

Please update the default value of the combo box too (around line 298)

@ssakash
ssakash Aug 17, 2015 Member

done. 👍

@turtleli turtleli commented on an outdated diff Aug 17, 2015
plugins/GSdx/GSDevice9.cpp
@@ -84,8 +84,8 @@ static void FindAdapter(IDirect3D9 *d3d9, UINT &adapter, D3DDEVTYPE &devtype, st
// if supported and null != msaa_desc, msaa_desc will contain requested Count and Quality
-D3DTEXTUREFILTERTYPE LinearToAnisotropic = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_LINEAR;
-D3DTEXTUREFILTERTYPE PointToAnisotropic = !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_POINT;
+D3DTEXTUREFILTERTYPE LinearToAnisotropic = theApp.GetConfig("MaxAnisotropy", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_LINEAR;
+D3DTEXTUREFILTERTYPE PointToAnisotropic = theApp.GetConfig("MaxAnisotropy", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_POINT;
@turtleli
turtleli Aug 17, 2015 Member

There's an existing bug here. The scope is global so the config read only happens once. Move it inside GSDevice9::Create(), just before the variables are used.

@turtleli turtleli commented on an outdated diff Aug 19, 2015
plugins/GSdx/GSDevice9.cpp
@@ -188,6 +185,9 @@ void GSDevice9::ForceValidMsaaConfig()
bool GSDevice9::Create(GSWnd* wnd)
{
+ D3DTEXTUREFILTERTYPE LinearToAnisotropic = theApp.GetConfig("MaxAnisotropy", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_LINEAR;
+ D3DTEXTUREFILTERTYPE PointToAnisotropic = theApp.GetConfig("MaxAnisotropy", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_POINT;
+
@turtleli
turtleli Aug 19, 2015 Member

Don't add whitespace issues, you've used tabs and spaces inconsistently and have trailing whitespace. Move the lines to just before the variables are used, somewhere after line 300.

@turtleli turtleli commented on an outdated diff Aug 19, 2015
plugins/GSdx/GSDeviceOGL.cpp
- if (GLLoader::found_GL_EXT_texture_filter_anisotropic && !!theApp.GetConfig("AnisotropicFiltering", 0) && !theApp.GetConfig("paltex", 0)) {
- int anisotropy = theApp.GetConfig("MaxAnisotropy", 1);
- if (anisotropy > 1) // 1 is the default in opengl so don't do anything
+ if (GLLoader::found_GL_EXT_texture_filter_anisotropic && anisotropy && !theApp.GetConfig("paltex", 0))
@turtleli
turtleli Aug 19, 2015 Member

Whitespace error on this line. Also, the blank line should be before int anisotropy, not after. For style consistency with the rest of the file the '{' and '}' should be removed.

@turtleli turtleli commented on an outdated diff Aug 19, 2015
plugins/GSdx/GSSettingsDlg.cpp
case IDC_HACKS_ENABLED:
if (code == BN_CLICKED)
UpdateControls();
break;
- case IDC_SHADEBUTTON:
- if (code == BN_CLICKED)
- ShadeBoostDlg.DoModal();
- break;
- case IDC_HACKSBUTTON:
- if (code == BN_CLICKED)
- HacksDlg.DoModal();
- break;
+
@turtleli
turtleli Aug 19, 2015 Member

Trailing whitespace. It also wasn't necessary to move IDC_SHADEBUTTON and IDC_HACKSBUTTON.

Please check that you haven't added any whitespace issues after you've reworked your commits.

@ssakash
Member
ssakash commented Aug 19, 2015

Hopefully those trailing whitespaces should be gone for good. 😆

@turtleli
Member

Still issues:
GSDevice9.cpp: Random blank line, mix of spaces and tabs for indentation. Use a tab, since that's what the rest of the files uses.
GSSettingsDlg.cpp: Move the IDC_SHADEBUTTON and IDC_HACKSBUTTON back to its original location and remove the blank line you added so it looks like you never touched that area.
GSDeviceOGL.cpp: Indentation and line grouping issues. Too annoying to explain, final result should look like this: (but don't copy paste it otherwise you'll introduce even more whitespace issues)

    gl_SamplerParameterf(sampler, GL_TEXTURE_MAX_LOD, 6);

    int anisotropy = theApp.GetConfig("MaxAnisotropy", 0);
    if (GLLoader::found_GL_EXT_texture_filter_anisotropic && anisotropy && !theApp.GetConfig("paltex", 0))
        gl_SamplerParameterf(sampler, GL_TEXTURE_MAX_ANISOTROPY_EXT, (float)anisotropy);

    GL_POP();
    return sampler;
@turtleli turtleli was assigned by gregory38 Aug 20, 2015
@gregory38
Contributor

Note, first commit ought to be tested on linux.

@ssakash
Member
ssakash commented Aug 20, 2015

@turtleli
fixed those blank lines and I think I Indented it a little better, please take a look.

@turtleli
Member

GSDevice9.cpp and GSDeviceOGL.cpp have a mix of tabs and spaces for indentation. Use only tabs.
GSDeviceOGL.cpp has trailing whitespace on the blank lines.

@ssakash
Member
ssakash commented Aug 20, 2015

Fixed the indentation and trailing whitespaces on GSDevceOGL.cpp , I don't see any issues of indentation on GSDevice9.cpp. (if there are any issues, please highlight them on the code).

@turtleli turtleli and 1 other commented on an outdated diff Aug 20, 2015
plugins/GSdx/GSDevice9.cpp
@@ -304,6 +301,8 @@ bool GSDevice9::Create(GSWnd* wnd)
m_convert.bs.BlendEnable = false;
m_convert.bs.RenderTargetWriteMask = D3DCOLORWRITEENABLE_RGBA;
+ D3DTEXTUREFILTERTYPE LinearToAnisotropic = theApp.GetConfig("MaxAnisotropy", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_LINEAR;
+ D3DTEXTUREFILTERTYPE PointToAnisotropic = theApp.GetConfig("MaxAnisotropy", 0) && !theApp.GetConfig("paltex", 0) ? D3DTEXF_ANISOTROPIC : D3DTEXF_POINT;
@ssakash
ssakash Aug 20, 2015 Member

I'd need to hit the Tab often then :p , Fixed 👍

@ssakash
Member
ssakash commented Aug 20, 2015

I'd need to hit the Tab often then :p , Fixed 👍

@turtleli
Member

Ok. Will test soon.

Side note: You might want to delete some of your unused branches.

@turtleli
Member

First commit: The longer messages are clipped even when resized larger. Both Windows and Linux.
Other two commits seem fine.
clippedmessage

@ssakash ssakash changed the title from Resizable main window and little changes on GSdx plugin settings. to little changes on AF value behavior. Aug 21, 2015
@ssakash
Member
ssakash commented Aug 21, 2015

I'll remove the first commit for now, I'll handle it later. the Image does look a bit weird after all :p

@ssakash ssakash Remove anisotropic filtering checkbox value.
Removes the checkbox of Anisotropic filtering from the GSDX plugin settings, the checkbox was usually used to enable & disable the AF which is not necessary since there is an option in the drop down list for disabling AF.
the internal function value of "AnisotropicFiltering" has been replaced with "MaxAnisotropy" for detection.
the detection uses the function getconfig("MaxAnisotropy", value)  where value 0 means disabled and value is the default value when no value is set in the INI file.
3c9ca79
@ssakash
Member
ssakash commented Aug 21, 2015

The PR is complete, changed the title relevant to the commits. if there any issues needed to be addressed, please mention. :)

@turtleli
Member

Second commit message - change 'D3D11_FILTER_ANISOTROPIC' to 'anisotropic filtering'.

@ssakash
Member
ssakash commented Aug 21, 2015

No, that is correct. Please refer GSDevice11.cpp. Nevermind, makes sense. The changes are not only for D3D11

@ssakash
Member
ssakash commented Aug 21, 2015

Updated the commit message 👍

@ssakash ssakash AF checkbox status depends on paltex.
according to some of the internal functions in GSDevice files, Anisotropic Filtering is only done when paltex is disabled. do the same on the GUI for user awareness.
1116538
@turtleli turtleli merged commit 6e826d5 into PCSX2:master Aug 21, 2015
@ssakash ssakash deleted the ssakash:patch-45 branch Aug 21, 2015
@karasuhebi
Contributor

Why was it changed from 1x to Off? People should know 1x means no Anisotropic Filtering. :-\

@ssakash
Member
ssakash commented Sep 1, 2015

Off would make it much more clear for those who don't know 1X is off.

@karasuhebi
Contributor

Fair enough. I still don't understand why the checkbox was removed though. Removing it didn't really improve the layout/spacing of the GSdx Plugin Settings window at all. What it did do was remove a convenience that certain users (such as myself) used. Is there anywhere I can go to lobby to get it put back? :D

@ssakash ssakash restored the ssakash:patch-45 branch Sep 6, 2015
@ssakash ssakash deleted the ssakash:patch-45 branch Sep 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment