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

GSdx HW: Add Skipdraw Offset option for Skipdraw hack (continued) #2534

Merged
merged 1 commit into from Aug 20, 2018

Conversation

Projects
None yet
5 participants
@turtleli
Copy link
Member

commented Aug 2, 2018

Continuation of #2290.

I added Linux GUI code and simplified some of the Windows GUI code and misc other stuff.

@ivan89el

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2018

I have no problems, it works as well #2290
(I have windows 7 x64)

@turtleli turtleli force-pushed the turtleli:gsdx-skipdraw-offset-resumed branch to 8f3dedb Aug 4, 2018

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2018

Updated the Linux GUI so it says Skipdraw Range on Linux as well.

@ivan89el

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2018

I found a problem: i do not save the first value and always reset to 1

@turtleli turtleli force-pushed the turtleli:gsdx-skipdraw-offset-resumed branch from 8f3dedb to daa3547 Aug 5, 2018

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2018

Should be fixed now.

@ivan89el

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

Now everything is good.

case IDC_SKIPDRAWOFFSETEDIT:
return "Completely skips drawing surfaces from the surface in the left box up to the surface specified in the box on the right.\n\n"
"Use it, for example, to try and get rid of bad post processing effects.\n"
"Step 1: Increase the value in the left box and keep the value in the right box set to the same value as the left box to find and remove a bad effect. Try values between 1 and 100.\n"

This comment has been minimized.

Copy link
@lightningterror

lightningterror Aug 7, 2018

Member

The gui allows values up to 10000.

This comment has been minimized.

Copy link
@turtleli

turtleli Aug 7, 2018

Author Member

I read it as a recommendation. Alternatively it could be removed.

BTW, you approved #2290 but the wording here hasn't changed. ;)

This comment has been minimized.

Copy link
@lightningterror

lightningterror Aug 7, 2018

Member

I read it as a recommendation. Alternatively it could be removed.

Ye I kinda guessed that, I say we remove it.

BTW, you approved #2290 but the wording here hasn't changed. ;)

Thought there was no need for that since you're the one PRing it :)

This comment has been minimized.

Copy link
@turtleli

turtleli Aug 7, 2018

Author Member

Thought there was no need for that since you're the one PRing it :)

My point was that you've kinda approved something that you want changes to be made to, in which case it's ambiguous as to whether you've approved it or not.

This comment has been minimized.

Copy link
@lightningterror

lightningterror Aug 7, 2018

Member

Ah, well in this case it's just a miniature nitpick but I'm fine with it being merged either way.

@turtleli turtleli force-pushed the turtleli:gsdx-skipdraw-offset-resumed branch from daa3547 Aug 7, 2018

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

Updated to address review comment.

plugins/GSdx/GSdx.rc Outdated
RTEXT "Geometry Shader:",IDC_GEOMETRY_SHADER_TEXT,14,241,62,8
COMBOBOX IDC_GEOMETRY_SHADER_OVERRIDE,80,238,97,63,CBS_DROPDOWNLIST | WS_VSCROLL | WS_TABSTOP
RTEXT "Image Load Store:",IDC_IMAGE_LOAD_STORE_TEXT,14,256,62,8
COMBOBOX IDC_IMAGE_LOAD_STORE,80,253,97,63,CBS_DROPDOWNLIST | WS_VSCROLL | WS_TABSTOP

This comment has been minimized.

Copy link
@lightningterror

lightningterror Aug 9, 2018

Member

Since you're changing gl hacks maybe you can adjust IDC_GEOMETRY_SHADER_OVERRIDE, IDC_IMAGE_LOAD_STORE width to match HPO, Sprite ..etc

@turtleli turtleli force-pushed the turtleli:gsdx-skipdraw-offset-resumed branch Aug 9, 2018

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2018

Updated to address review comment.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

Step 2: If a bad effect found with Step 1 is not completely removed yet, then without changing the value in the left box, try increasing the value in the box to right until the effect is completely gone.

This is a bit of an issue when not using the up and down spin button or keyboard arrow keys to change the value. If you're going to type down a value, the value on the left also gets changed.

Edit: Perhaps that value correction can be done when the Ok button is clicked instead ?

Some issues remain.

plugins/GSdx/GSSetting.cpp Outdated
"Use it, for example, to try and get rid of bad post processing effects.\n"
"Step 1: Increase the value in the left box and keep the value in the right box set to the same value as the left box to find and remove a bad effect.\n"
"Step 2: If a bad effect found with Step 1 is not completely removed yet, then without changing the value in the left box, try increasing the value in the box to right until the effect is completely gone.\n";
"Note: Increase the value in the right box and keep the value in the left box set to \"1\" to reproduce the old skipdraw behaviour.";

This comment has been minimized.

Copy link
@lightningterror

lightningterror Aug 10, 2018

Member

I don't see the last line in the tooltip, maybe because of \"1\" ?

This comment has been minimized.

Copy link
@turtleli

turtleli Aug 10, 2018

Author Member

It's the ;

This comment has been minimized.

Copy link
@lightningterror

lightningterror Aug 10, 2018

Member

Ah right, didn't notice that.

@turtleli turtleli force-pushed the turtleli:gsdx-skipdraw-offset-resumed branch to 691d2aa Aug 10, 2018

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2018

Updated to improve value correction and fix the tooltip.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

Sweet, it works much better now. Is there anything else that needs tweaking ?
If not then this could be ready for a merge.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Aug 12, 2018

Once this gets merged I'm also thinking of adjusting the TC Offsets in to one line. Kinda like this new skipdraw feature.

TC Offset X,Y - X on the left and Y on the right.

@turtleli turtleli force-pushed the turtleli:gsdx-skipdraw-offset-resumed branch from 691d2aa to 67ce6ea Aug 13, 2018

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Updated to fix the comment (I mixed up skipdraw and skipdraw offset in one place). No functional changes.

@mirh

This comment has been minimized.

Copy link

commented Aug 13, 2018

Uh, I had always understood this PR to be merged *after* flatout one, not replace it

@turtleli turtleli force-pushed the turtleli:gsdx-skipdraw-offset-resumed branch from 67ce6ea to ee2d46b Aug 13, 2018

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Updated again. I changed the Linux behaviour back to what I had at the start of the PR - it doesn't have the same issue Windows does.

Uh, I had always understood this PR to be merged after flatout one, not replace it

It's easier to just replace it in this case.

@mirh

This comment has been minimized.

Copy link

commented Aug 13, 2018

Obviously, sure.
But I can remember somebody kind-of-complaining in the past for authorship vs commiting, or something.

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

I set the commit author to FlatOut.

git commit --author="namename<emailemail>"
@lightningterror

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

Updated again. I changed the Linux behaviour back to what I had at the start of the PR - it doesn't have the same issue Windows does.

Seems a bit odd that it doesn't.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Aug 18, 2018

@turtleli is this ready for a merge ?

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2018

Probably? I'll merge it after some (hopefully) final checks.

Seems a bit odd that it doesn't.

It's not exactly surprising if the OS and toolkit are both different.

@@ -2364,7 +2364,8 @@ bool GSState::IsBadFrame()
// General, often problematic post processing
if (GSLocalMemory::m_psm[fi.TPSM].depth || GSUtil::HasSharedBits(fi.FBP, fi.FPSM, fi.TBP0, fi.TPSM))
{
m_skip = m_userhacks_skipdraw;
m_skip_offset = std::max(m_userhacks_skipdraw_offset, 1);

This comment has been minimized.

Copy link
@turtleli

turtleli Aug 18, 2018

Author Member

It seems like the std::max is unnecessary.

@turtleli turtleli force-pushed the turtleli:gsdx-skipdraw-offset-resumed branch from ee2d46b to 41b09dd Aug 18, 2018

@lightningterror

This comment has been minimized.

Copy link
Member

commented Aug 19, 2018

It's not exactly surprising if the OS and toolkit are both different.

But isn't it better to keep that code functionality even if the issue is not present ?
Just making sure everything is proper.

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2018

Keep what functionality?

@lightningterror

This comment has been minimized.

Copy link
Member

commented Aug 19, 2018

Keep what functionality?

Updated again. I changed the Linux behaviour back to what I had at the start of the PR - it doesn't have the same issue Windows does.

Keeping the updated version and not how it was at the start of the PR.

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2018

Because the issue that affects Windows isn't present and the original Linux version has better behaviour than the updated version that works around an issue that doesn't exist?

@lightningterror

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

@turtleli I made some adjustments to the TC offsets hack and put them in one line like skipdraw offset. Do you wanna include these changes as well if they are alright ?

image

lightningterror@dc9ad1d

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

I'd prefer not to include it. It's unrelated to the PR and I'd have to delay merging this PR (which will probably be done tonight) so people have time to give feedback on those changes.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Sure thing, I'll look in to adding them in the future once this gets merged then.

gsdx:hw: Enable skipdraw hack to skip a range of draw calls
Enhance the skipdraw hack by allowing skipdraw to skip a range of draw
calls.

For example: When the broken effects are at frames 90-100, the default
skipdraw always skips 0-100, possibly skipping several functioning
effects as well. By enhancing the skipdraw feature, it is now possible
to skip just frames 90-100.

For the example given above set the first box to 90 and the second box
to 100 to skip frames 90-100.

coauthor:turtleli (Linux GUI + tidy/simplify Windows GUI code)

@turtleli turtleli force-pushed the turtleli:gsdx-skipdraw-offset-resumed branch from 41b09dd to d9560fd Aug 20, 2018

@turtleli

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

...Updated again because I accidentally undid the Linux changes on my previous push (was on Windows and didn't realise my local branch was out of sync). That's fixed now.

Will probably still merge it once the buildbots have finished running.

@turtleli turtleli merged commit 1498f53 into PCSX2:master Aug 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@turtleli turtleli deleted the turtleli:gsdx-skipdraw-offset-resumed branch Aug 20, 2018

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.