Do depth copy in through mode as well

[dbz400]

As always , not too sure if it is totally correct and may need more testing .Tested on my NV system and better performance loss than previously .(less than 10 FPS in unthrottling mode)

Tested with the following games that may require depth copy to work propertly

1. Saint Seiya Omega
2. Ys 7
3. Gundam AGE Universe
4. Jeanne D Arc

[dbz400]

Also fixes #5068 .May help #5028 and #5070

[unknownbrackets]

Why did you remove the gstate.isModeClear() check? What is the rationale/reasoning for not allowing this in throughmode?

I'm pretty sure that throughmode is just as capable of doing depth testing.

-[Unknown]

[dbz400]

Indeed , I only test it based on real game to see if it works propertly and how's the compability in general case .

Of course it may not cover all cases and only best effort .

[unknownbrackets]

I still don't understand the clearmode check removal.

At the least it should have a comment saying that why throughmode is excluded, and that it is a hack to attempt to improve performance.

-[Unknown]

[dbz400]

I have added comment to specific mention it is a hack and why throughmode is excluded.

[unknownbrackets]

I still don't understand why you removed the isModeClear() check, and if I read this correctly, you're ONLY doing it during throughmode, not excluding throughmode.

-[Unknown]

[dbz400]

Done .I put the !gstate.isModeClear() back :)

[psennermann]

And what about #5064 ? Wouldn't it be fixed as well?

[daniel229]

It does not fix #5064

[solarmystic]

@raven02

The newly changed commit in this pull request causes the performance to be bad again.

To clarify, for line 882 in GPU/GLES/Framebuffer.cpp, when if (!gstate.isModeClear() || !gstate.isClearModeDepthMask()) { (current master line) is changed to

if (!gstate.isClearModeDepthMask() && gstate.isModeThrough()) { which you originally suggested in your #5070 (comment) helps restore performance close to what it used to be while still fixing the Jeanne D'arc issue with missing characters..

However, putting back !gstate.isModeClear() into that line,

if ((!gstate.isClearModeDepthMask() && gstate.isModeThrough()) || !gstate.isModeClear()) { which is what the current pull request has now been changed to does nothing to improve the performance in GBE.

Just an observation, since if this pull request is merged as it is right now, #5070 will not be fixed since the commit has been altered. It seems like adding back the !gstate.isModeClear() line makes the performance awful again for some reason.

[unknownbrackets]

Well, as I said, it's necessary to check both, but the code didn't make sense in the beginning and still doesn't make sense.

If gstate.isClearModeDepthMask() is true, AND gstate.isModeClear() is true, then we definitely do not want to copy the framebuffer. It'd be pointless, since it's about to clear it with 95%+ certainty.

if (!(gstate.isModeClear() && gstate.isClearModeDepthMask()))

It happens that this is logically identical to:

if (!gstate.isModeClear() || !gstate.isClearModeDepthMask())

That's what the code currently does. If a hack is added to skip this except in throughmode, (which I still expect there may be better ways, and sitll doesn't really make sense to me), then it would be one of:

if (gstate.isModeThrough() && !(gstate.isModeClear() && gstate.isClearModeDepthMask()))

if (gstate.isModeThrough() && (!gstate.isModeClear() || !gstate.isClearModeDepthMask()))

The current code does not make sense. It says to copy the framebuffer always if clear mode is off, OR if (clear depth is off AND it's not throughmode.)

The point is that gstate.isClearModeDepthMask() is an invalid value when gstate.isModeClear() is false. A check of only gstate.isClearModeDepthMask() (without some related check of gstate.isModeClear()) is ALWAYS A BUG, imho, because its value is meaningless without knowing if it's in clear mode. So you must also check gstate.isModeClear().

-[Unknown]

[dbz400]

Anyway , I gonna close this and hopefully we can have a better option here :)