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-TC: Fix load size calculation in target update #2122

Merged
merged 1 commit into from Mar 3, 2018

Conversation

@ssakash
Member

ssakash commented Nov 8, 2017

Summary of changes:

  • Fix the load size calculation in target update.
  • Add an optional macro (ENABLE_ACCURATE_BUFFER_EMULATION) for enabling more accurate emulation of the buffer size. Haven't discovered any cases where it's necessary yet, but according to my old logs, it might be necessary on some superman game which uses 2048x2048 on scissoring.

Might help with #1972, also the code impacts every resolutions not just custom. So potentially there might also be some improvements apart from custom resolutions. Also please look out for the performance impact of this change.

Fixes #1972, Fixes #2110 (Custom resolution crashing regression)
Fixes #2138 (FMV flickering Beyond good and evil regression)

Anyone wants to help me test this? @atomic83GitHub @MrCK1 @lightningterror

@atomic83GitHub

This comment has been minimized.

Contributor

atomic83GitHub commented Nov 8, 2017

I can help for this, can you post a custom PCSX2 version with the changes?

@MrCK1

This comment has been minimized.

Member

MrCK1 commented Nov 8, 2017

I'll test it, but for the record I never had crashing on custom res.

@atomic83GitHub

This comment has been minimized.

Contributor

atomic83GitHub commented Nov 8, 2017

So, I have test Sillent Hill 3, Top spin and Kung fu panda, and none of them crash with custom resolution!

I have also noticed that the ingame texture seems more precise with any resolution wich improve the overal render, good!

Also: it seems that Sillent hill 3 did not display the up and left part of the inventory screen before this PR, it is now perfect.
capture

And for finish, I don't have any performance impact on my games.

@ssakash

This comment has been minimized.

Member

ssakash commented Nov 8, 2017

Also: it seems that Sillent hill 3 did not display the up and left part of the inventory screen before this PR

Was this issue exclusive to custom resolution before (or) was it also present in native scaling resolutions (2x, 3x, 4x native ...)?

Also thanks for testing, really appreciated. :)

@atomic83GitHub

This comment has been minimized.

Contributor

atomic83GitHub commented Nov 8, 2017

#ssakash thanks!
It was present in any Hardware resolution (even with native).

@lightningterror

This comment has been minimized.

Member

lightningterror commented Nov 8, 2017

Can you mark it using paint so I can reproduce it ? 😝

@atomic83GitHub

This comment has been minimized.

Contributor

atomic83GitHub commented Nov 8, 2017

Before, in some places if you goes into inventory, you have this corruption, now it is gone for me.
capture
Or enable Preload frame data to avoid it.

@MrCK1

This comment has been minimized.

Member

MrCK1 commented Nov 13, 2017

self-reminder for myself to check Timesplitters later. :)

@MrCK1

This comment has been minimized.

Member

MrCK1 commented Nov 18, 2017

Timesplitters is still busted unfortunately.

@lightningterror

This comment has been minimized.

Member

lightningterror commented Nov 19, 2017

Looks like it also solves MGS 2 and 3 crashing.
https://forums.pcsx2.net/Thread-MGS2-3-randomly-crash-after-1-3-minutes

t_size.y = lround(static_cast<float>(t_size.y) / t_scale.y);
// Ensure buffer width is at least of the minimum required value.
// Probably not necessary but doesn't hurt to be on the safe side.
int buffer_width = static_cast<int>(m_TEX0.TBW << 6);

This comment has been minimized.

@gregory38

gregory38 Nov 23, 2017

Contributor

Do you really need the cast here ?

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Dec 11, 2017

So if I understand it right, before we limit the upload based on GS size (t_size/t_scale). Now we limit it to the maximum size. However following code will stretch the data based on the scale factor.

	m_renderer->m_dev->StretchRect(t, m_texture, GSVector4(r) * GSVector4(m_texture->GetScale()).xyxy());

Strangely we avoid the 0 value in scale. But in this case, it means that uploading code is useless. So maybe it can be completely skipped. I'm not sure to understand where is the code fix.

Then it would be better to limit the stretch to the texture size.

upscaled_size = GSVector4(r) * GSVector4(m_texture->GetScale()).xyxy()
min(upscaled_size, m_texture->GetSize());

Finally, perf wise, the impact is likely small. t_size is just a way to clamp the value of r. So it might only impact the preload hack (which is quite dumb)

@lightningterror

This comment has been minimized.

Member

lightningterror commented Feb 2, 2018

What's the status on this ?

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Feb 2, 2018

Has @ssakash been around lately? It seems this PR is waiting for feedback.

@lightningterror

This comment has been minimized.

Member

lightningterror commented Feb 2, 2018

He was on discord 2 days ago.

@hellbringer616

This comment has been minimized.

hellbringer616 commented Feb 3, 2018

No regressions on the games i've played (can provide a list if needed) solves a crashing issue on Rogue Galaxy when performing a power attack by Steve when using custom resolution.

GSdx-TC: Fix load size calculation in target update
Previously, the calculation for the size of data to be loaded was done
based on the rendering target buffer size and scaling multiplier, which
was totally wrong. This led to different resolutions having different
load sizes while the size of the real GS memory is common regardless of
the scaling variancies.

Hence use the default rendering target buffer size for the load size
independent of the scaling values. I've also removed a buffer height saturation
code which seemed unreliable.

Note: The accurate version of the code can be enabled using the macro
provided in config.h (which is more intensive on resources), the current
code goes along with the approach of maintaining a decent performance
level along with a formidable accuracy.
@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Feb 6, 2018

The PR now fixes Beyond Good and Evil FMV flickering as long as the Memory Wrapping hack is enabled.

@ssakash

This comment has been minimized.

Member

ssakash commented Feb 6, 2018

Then it would be better to limit the stretch to the texture size.

That would change the behaviour compared to before and I'm not sure why we'd need to do it, the limit might only work on higher resolutions though (3x and above I guess), though is there any specific reason for suggesting to change the older stretch logic?

Strangely we avoid the 0 value in scale. But in this case, it means that uploading code is useless. So maybe it can be completely skipped. I'm not sure to understand where is the code fix.

I'm not sure how a 0 value scenario for scale would be possible, the rescaling logic created chaos on custom resolutions where the buffer width and height values are unusual, removing that rendering target size dependency for t_size calculation was the fix for such cases.

The PR now fixes Beyond Good and Evil FMV's as long as the Memory Wrapping hack is enabled.

Hmm, you mean the issue which was caused by one of gregory's older commits? (265ea82)

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Feb 6, 2018

Hmm, you mean the issue which was caused by one of gregory's older commits? (265ea82)

Yes, the issue from #2138 was fixed by your PR. The game requires memory wrapping because of a later change.

@lightningterror

This comment has been minimized.

Member

lightningterror commented Feb 6, 2018

So that's 2 regressions fixed. Pretty neat.

Gave Silent Hill2/3 a quick test again just to be sure it doesn't crash again with custom resolution and it's all ok.

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Feb 6, 2018

Gave Silent Hill2/3 a quick test again just to be sure it doesn't crash again with custom resolution and it's all ok.

Did you check the top left corner issue too?

@lightningterror

This comment has been minimized.

Member

lightningterror commented Feb 6, 2018

Well there is a way to corrupt the mini window, when you switch between hardware and software renders you can cause the mini window to be completely broken, either corrupted garbage graphics or just black window instead, as a matter of fact when you switch between hardware and software you can trigger an entire screen garbage but it lasts less than 1s.

Through normal gameplay I wasn't able to reproduce it, tho I have the NTSC version, or it's hard to reproduce I dunno.

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Feb 6, 2018

Switching to the SW renderer and back didn't corrupt the mini window when I tried it. But I can confirm the less than 1s garbage when switching renderers.

I found a reliable way of reproducing the issue. Go to the menu, examine the pendant, then go to the options menu and straight back to the inventory. The mini window will now be corrupted. This PR nor the Preload Frame Data hack seems to affect the issue.

@lightningterror

This comment has been minimized.

Member

lightningterror commented Feb 6, 2018

That explains why I only reproduced it by chance.

Well this is the result I expected honestly.

@atomic83GitHub

This comment has been minimized.

Contributor

atomic83GitHub commented Feb 7, 2018

I can confirm this PR solve the Beyond Good and Evil FMV regression when the memory wrapping option is enabled.

@MrCK1

This comment has been minimized.

Member

MrCK1 commented Feb 22, 2018

No change in Syphon Filter unfortunately.

@refractionpcsx2

This comment has been minimized.

Member

refractionpcsx2 commented Feb 27, 2018

Is this ready to merge?

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Feb 27, 2018

Is this ready to merge?

We're waiting on a test of Rogue Galaxy.

@refractionpcsx2

This comment has been minimized.

Member

refractionpcsx2 commented Feb 27, 2018

Then I'll mark it as needs testing.

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 2, 2018

This can be merged.

#2110 (comment)

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Mar 2, 2018

Not yet. @hellbringer616 mentioned no crashes so far, which suggest it's still being tested. ;)

@hellbringer616

This comment has been minimized.

hellbringer616 commented Mar 3, 2018

After 3 hours of game play i can't get it to crash; I'd call that good

@refractionpcsx2

This comment has been minimized.

Member

refractionpcsx2 commented Mar 3, 2018

Before I merge this, do we want any "Fixes" comments in the commit message?

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 3, 2018

I edited the PR message, you can add them in the commit messages if it's needed.

Fixes #1972, Fixes #2110 (Custom resolution crashing regression)
Fixes #2138 (FMV flickering Beyond good and evil regression)

@refractionpcsx2 refractionpcsx2 merged commit 084530e into master Mar 3, 2018

4 checks passed

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

This comment has been minimized.

Member

lightningterror commented Mar 3, 2018

Ah I've been waiting so long for this to get merged.

@refractionpcsx2

This comment has been minimized.

Member

refractionpcsx2 commented Mar 3, 2018

I bet ssakash has too :p

My only question, is this going to have any effect on the large frame buffer option? I feel like it's rendered it obsolete (although now it always uses a large frame buffer which is bad for perf)

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 3, 2018

I think it will still help in games with fmv flickering just like before.

@ssakash ssakash deleted the target_load branch Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment