PCSX2 / pcsx2

PCSX2 - The Playstation 2 Emulator
https://pcsx2.net
GNU General Public License v3.0
11.89k stars 1.63k forks source link

[GSdx SW] Xenosaga Episode I [NTSC-U]: glitchy event slot visuals #1769

Closed trostboot closed 7 years ago

trostboot commented 7 years ago

PCSX2 version: 1.5.0-dev-1789, 1.4.0

PCSX2 options: defaults, no speedhacks

Plugins used: GSdx SW, SW settings have no effect on it

Description of the issue: Some pixels in the event slot graphic (bottom right of the screen) during battles are shifted to the right in SW rendering modes. It looks correct in HW modes, regardless of API.

SW mode (glitched): gsdx_20170113105621

HW mode (correct): gsdx_20170113105619

GSdump: gsdx_20170113105443.zip

How to reproduce the issue: Enter any battle and look at the event slot. It's most noticeable in the empty slot.

Last known version to work: Unknown, it's present in current git builds as well as 1.4.0. I can look at older builds later today if need be.

PC specifications: i5-4690k, R9 270X, 16GB RAM, Win10 Pro x64 14393

gregory38 commented 7 years ago

I'm trying to debug the dump. The bad draw call is the number 936

The game renders the area the inner rectangle in 3 parts (top/middle/bottom). Each part is composed of 2 triangles. Delta(position X) == Delta(texture X) == 288.

However when I dump the dscan variable (delta). Note, position is scaled with a factor 1.0f and texture is scaled with a factor 65536.0f

Prim dscan p:1.000000 t:65536.007812
Prim dscan p:1.000000 t:65536.007812

Prim dscan p:1.000000 t:65535.996094
Prim dscan p:1.000000 t:65535.996094

Prim dscan p:1.000000 t:65536.007812
Prim dscan p:1.000000 t:65536.007812

As you can see the top/bottom part got t slighly above 1. But the middle is smaller than 1 which could "enlarge" the texture. I don't have any proof that this small rounding can actually provoke a wrong sampling.

Note the rounding error can be computed as (t - 65536)/65536 ~~ 10 ^ -7. Wikipedia says that float can give around 7.2 decimal digits. So we are at the limit of float. double will be better but much slower (need to redo everything anyway)

gregory38 commented 7 years ago

I can confirm the rounding issue. If I add an epsilon (0.01f) to t.x, the issue is gone.

gregory38 commented 7 years ago

@sudonim1 I need your help on the math. The function GSRasterizer::DrawTriangle computes the scanline information. In particular the delta of texture coordinate. In this issue we have flat triangles (2 triangles create a sprite).

So delta(texture.x)/delta(x) is 1. In other word, texture X coordinate is incremented by 1 when the X position is incremented by 1. However the math gave us either

Due to integer/trunc, the minus epsilon is rather annoying. Any idea how we can improve it ?

Note to self: a Jak game gets also a bad texture coordinate (which give wrong palette color). Maybe it is a similar issue.
sudonim1 commented 7 years ago

Oh god more GSVector code, this stuff is impossible to read at the best of times. Before thinking about double, the whole system should be moved to fixed point btw because that's what the GS actually uses.

Essentially drawing a triangle is done by calculating deltas in barycentric coordinates for the deltas in x/y from scanning (the barycentric coordinates are used directly to determine whether to draw a pixel and used as weights for the texture coordinates, depth, etc., usually multiplying in the setup). If we're rounding different ways for dx in right angled triangles of the same width, the algorithm we're using for this may be strictly mathematically correct but isn't appropriate for real computer graphics.

sudonim1 commented 7 years ago

I don't immediately have an answer for the right algorithm to use.

sudonim1 commented 7 years ago

I'm aware that's fairly useless, I'm trying to find a really good article on this I read once with a very practical approach to optimisation compared to most sample rasterisers.

sudonim1 commented 7 years ago

I'm going to drop a few links here for now and note that half-space functions are at right angles to barycentric coordinate transforms.

http://www.cs.unc.edu/~olano/papers/2dh-tri/ http://web.archive.org/web/20110710170542/http://www.devmaster.net/codespotlight/show.php?id=17 http://www.drdobbs.com/parallel/rasterization-on-larrabee/217200602 http://www.uni-obuda.hu/journal/Mileff_Nehez_Dudra_63.pdf

sudonim1 commented 7 years ago

Actually the half-space functions should be equivalent to each of the barycentric coordinates? Geh, I'm rusty.

gregory38 commented 7 years ago

Are you sure the GS rasterizer is fixed-point. S/T/Q are float. Anyway I think the math is correct. I need to check my code, but I did something like that (when processing triangle)

dscan.t.x *= (1 + epsilon); // I don't remember the number of 0 in 1.0000001f

It does the trick for this issue. But it doesn't solve my Jak testcase. I would need to find the bad draw call to dump the state properly. As far as I remember, the game does a palette blit with ST ranging from 0f to 1f but the blit shift the palette by 1 texel. It could just be the game uses another code path (sprite instead of triangle for example).

sudonim1 commented 7 years ago

As I recall there was something in the GS manual which outright stated that the DDA is fixed point at least for everything but STQ. There's no reason it has to only be an integer unit or only be a floating point unit.

sudonim1 commented 7 years ago

At the very least it would be good to document the maths that gsdx is using now, it's not at all obvious from the vector code.

sudonim1 commented 7 years ago

Although perhaps what I'm saying is too idealistic for a quick fix, trying to hack out the rounding errors seems misguided.

gregory38 commented 7 years ago
 There's no reason it has to only be an integer unit or only be a floating point unit.

They likely have 3 units to compute color/fog/texture in parallel. However if there is a floating point unit, it is likely the same as EE/VU.

On the math stuff, an overview of my understanding, I think GSdx

In the rendering, it does

for (c = left.c, t = left.t, l = 0 ; l < lenght; ) {
      // do rendering here

      // then compute next scanline data
      c += delta.c;
      t += delta.t;
      l++;
}

The advantage of the above code is that you need few register/computing in the fragment shader.

gregory38 commented 7 years ago
Although perhaps what I'm saying is too idealistic for a quick fix, trying to hack out the rounding errors seems misguided.

Yes. The hypothesis is that an error of +1/+2 epsilons is better than -1/+1 epsilon.

gregory38 commented 7 years ago

Hum, I'm looking at the similar Jak issue. The blit is better with my correction, however there is a "similar" issue on the Y axis. Note that X/Y axes aren't fully symetrical so I"m not sure is the same issue.

gregory38 commented 7 years ago

@trostboot @FlatOutPS2 we need people to test the PR. It seems to work on my CPU (Haswell) but we need more CPU coverage.

sudonim1 commented 7 years ago

The CPU thing was mostly because I thought the change wasn't working for me, it can probably be ignored. I thought the issue was the gap in the red and blue chevrons. Please add a big red arrow to screenshots showing subtle graphical problems :P

FlatOutPS2 commented 7 years ago

The dump now flickers on and off with the new PR with one frame still showing the issue, but that's probably just a static remnant frame.

trostboot commented 7 years ago

Looks good to me ;)

gsdx_20170227204715

gregory38 commented 7 years ago

Fixed by https://github.com/PCSX2/pcsx2/commit/438fbf31cf2a846bf680434357ef3054f44ac000

Thanks you for the report.Big thanks to pseudo for the help.