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

Round viewport coordinates when vertex rounding is enabled #10398

Merged
merged 3 commits into from Apr 9, 2022

Conversation

Pokechu22
Copy link
Contributor

This should fix https://bugs.dolphin-emu.org/issues/9105 (an issue where a blue line appears in Wii Sports Resort's archery at 3x IR or higher).

The issue was caused by the game trying to reset the depth on a part of the screen by drawing a rectangle over it (object 264 in the fifolog), followed by it clearing the color of that region using an EFB copy, but at higher IRs these two things stop matching exactly.

Specifics

Object 264 has this:

BP register BPMEM_SCISSORTL
Scissor Top: 402
Scissor Left: 788

BP register BPMEM_SCISSORBR
Scissor Bottom: 489
Scissor Right: 852

BP register BPMEM_SCISSOROFFSET
Scissor X offset: 171
Scissor Y offset: 171

i.e. a scissor with a top coordinate of 402-(171*2)=60, a left coordinate of 446, a bottom coordinate of 147, and a right coordinate of 510, so a 65 by 88 scissor rectangle with the top-left at (446, 60).

It also has this:

XF register Write 6 XF regs at 101a
XFMEM_SETVIEWPORT + 0
Viewport width: 32.5
XFMEM_SETVIEWPORT + 1
Viewport height: -44
XFMEM_SETVIEWPORT + 2
Viewport z range: 16777216
XFMEM_SETVIEWPORT + 3
Viewport x origin: 820.766
XFMEM_SETVIEWPORT + 4
Viewport y origin: 446
XFMEM_SETVIEWPORT + 5
Viewport far z: 16777216

This is centered at y=446-(171*2)=104 and x=478.766, and has a radius of 32.5 horizontally/44 vertically, meaning the top-left is at (446.266, 60) and the size is 65 by 88. The problem here is the discrepancy between 446 and 446.266, which is too small to see on real hardware at 1x IR, and still too small at 2x IR (where it becomes 892 and 892.532; I think these both round to 892 due to pixel centering, but I'm not 100% sure), but is a problem at 3x IR (1338 vs 1338.798) and definitely a problem at 4x IR (1784 vs 1785.064). Note that there's no reason why the game couldn't have used precisely 446.

The EFB copy afterwards (EFB copy 27) uses this:

BP register BPMEM_EFB_TL
EFB Left: 446
EFB Top: 60

BP register BPMEM_EFB_WH
EFB Width: 64
EFB Height: 88

The mismatch between a width of 65 and of 64 is odd, and I'm not sure what the deal with it is, but it doesn't seem to actually cause problems (at least here), and existed before this PR.


Here's a quick summary of the affected coordinates before and after this PR:

Coord Old color Old depth New color New depth
Left 446 446 446 446
Top 60 60 60 60
Right 509 510 509 510
Bottom 147 147 147 147

And at 3x IR (with the 1x IR equivalent in parentheses):

Coord Old color Old depth New color New depth
Left 1338 (446) 1339 (446⅓) 1338 (446) 1338 (446)
Top 180 (60) 180 (60) 180 (60) 180 (60)
Right 1529 (509⅔) 1532 (510⅔) 1529 (509⅔) 1532 (510⅔)
Bottom 443 (147⅔) 443 (147⅔) 443 (147⅔) 443 (147⅔)

The only difference is the change from old depth left to new depth left, where it went from 446⅓ to 446. (Note that we don't want fractions on the top or left, but we want fractions on the bottom or right, since we want all pixels that correspond to a 1x IR pixel to be affected (e.g. if at 1x IR a pixel with x=147 is affected, we want x=147, x=147⅓, and x=147⅔ to be affected at 3x IR.)


This could be tied behind an option (either vertex rounding or a new one), but I don't think should make any difference at 1x IR and will only ever result in games rendering better (unlike vertex rounding, which slightly reduces detail).

@Pokechu22 Pokechu22 marked this pull request as draft January 26, 2022 21:21
@Pokechu22
Copy link
Contributor Author

Pokechu22 commented Jan 26, 2022

With regards to the fifoci differences when this was always enabled, I did some testing with Star Fox Assault.

Images

Here's what it looks like on real hardware:

3_y0_x0
image
image

Here's what it looks like on master with vulkan for me:

1_y0_x0
image
image

And here's this PR:

1_y0_x0
image
image

Here's the viewport configuration:

XF register Write 6 XF regs at 101a
XFMEM_SETVIEWPORT + 0
Viewport width: 320
XFMEM_SETVIEWPORT + 1
Viewport height: -224
XFMEM_SETVIEWPORT + 2
Viewport z range: 16777215
XFMEM_SETVIEWPORT + 3
Viewport x origin: 662.5
XFMEM_SETVIEWPORT + 4
Viewport y origin: 566.5
XFMEM_SETVIEWPORT + 5
Viewport far z: 16777215

The viewport is centered at x=662.5-342=320.5, y=224.5, and the radii are 320 and 224, so it covers (.5, .5) to (640.5, 448.5). This PR ends up rounding that to (1, 1) to (641, 449), and because modern graphics APIs don't draw anything outside of the viewport, that means the row with y=0 and the column with x=0 render black. (Adjusting clipping to allow drawing outside of the viewport, needed by Mario Party 8, would get rid of the black row and column, but there still is an offset). Real hardware seems to work differently from rounding, so I'll put this behavior behind a checkbox (and enable it by default for Wii Sports Resort via gameini).

@Pokechu22
Copy link
Contributor Author

I've made this configurable. Also, here are some images of how it affects Wii Sports Resort:

3x IR

00000000_2022-01-26_19-54-12
00000000_2022-01-26_19-54-24

1x IR

00000000_2022-01-26_19-54-44
00000000_2022-01-26_19-54-48

@Pokechu22 Pokechu22 marked this pull request as ready for review January 27, 2022 04:28
@Pokechu22
Copy link
Contributor Author

After looking a bit further, I found a place where regular vertex rounding is needed in Wii Sports Resort:

Images

00000000_2022-01-26_21-30-17
00000000_2022-01-26_21-30-23

Look at the gradient between the "golf" and "1 player" text; there's a small section where it's missing (that shows the sky behind). This is because for some reason the gradient is actually two separate objects, and they don't line up perfectly. Vertex rounding fixes this perfectly, and as such maybe we should just combine viewport rounding with vertex rounding. There probably aren't any games that need viewport rounding but not vertex rounding. (As far as I can tell, this issue applies to all player count select menus in Wii Sports Resort, not just golf, but I might be wrong.)

@JMC47
Copy link
Contributor

JMC47 commented Jan 27, 2022

That sounds reasonable to me.

@Pokechu22 Pokechu22 force-pushed the viewport-rounding branch 3 times, most recently from b37b769 to 4308134 Compare January 27, 2022 06:10
@Pokechu22 Pokechu22 changed the title Round viewport coordinates to the nearest integer Round viewport coordinates when vertex rounding is enabled Jan 27, 2022
@iwubcode
Copy link
Contributor

It seems reasonable to me to add this to vertex rounding for now. If we ever run into a case where a user needs the old behavior to NOT occur but the new viewport rounding to occur, we can create a new setting.

@Pokechu22 - can you update the vertex rounding description in the meantime?

@Pokechu22
Copy link
Contributor Author

This is the existing description:

static const char TR_VERTEX_ROUNDING_DESCRIPTION[] = QT_TR_NOOP(
"Rounds 2D vertices to whole pixels.<br><br>Fixes graphical problems in some games at "
"higher internal resolutions. This setting has no effect when native internal "
"resolution is used.<br><br><dolphin_emphasis>If unsure, leave this "
"unchecked.</dolphin_emphasis>");

I'm not sure what I could add to it that would be useful to end-users; sure, I could also mention that it also rounds viewport coordinates, but I don't know if that'd be useful. I could add something about "sometimes reduces detail or moves things slightly" (the latter applies to both viewport and vertex rounding, while the former only to vertex rounding), I guess.

@iwubcode
Copy link
Contributor

iwubcode commented Jan 27, 2022

Would be curious to hear from others. I'd probably opt for "rounds viewport" but I can appreciate most users aren't going to care about that distinction.

I was more thinking it could possibly help for support. Ex: a user says: "I'm seeing this issue in this build" and we didn't remember that vertex rounding changed, we might see the description and go "OH YEAH it does more than round the vertices now". Actually maybe change the checkbox title too in that case. "Vertex/Viewport Rounding".

EDIT: If you decide to this, I'd keep it simple, something like:

Rounds 2D vertices to whole pixels and rounds the viewport to a whole number.

Fixes graphical problems in some games at higher internal resolutions. This setting has no effect when native internal resolution is used.

@philou-felin
Copy link

I am that kind of end user and, while English is not my native language, I still need to say that I'm not sure I understand the "viewport" part of the setting.

I think I remember "viewport" being the "windows" which displays the video and the graphics, like "viewport" is a thing in Web pages.

It think you should keep the mention about coordinates, because it seems these are the one being rounded and it makes more sense to me... ... Honestly, I think @iwubcode has a point about tech support / troubleshooting. Maybe what you really need here is a good enough reminder for people who'll help end-users like me on the forum or anywhere else...

Hopefully I'm not being detrimental here by participating.

@Pokechu22
Copy link
Contributor Author

Pokechu22 commented Jan 27, 2022

I think I remember "viewport" being the "windows" which displays the video and the graphics, like "viewport" is a thing in Web pages.

"Viewport" is one of those terms that has a ton of different meanings; computer graphics has a lot of terms like that unfortunately. The applicable meaning is in this English Wikipedia article (I think this is the applicable part of the Spanish article, and I can't say with certainty where it is in other languages).

In 3D computer graphics it refers to the 2D rectangle used to project the 3D scene to the position of a virtual camera. A viewport is a region of the screen used to display a portion of the total image to be shown.


I also need to correct myself about that earlier "loses detail" comment: vertex rounding doesn't actually lose detail. I thought it applied to both 3D and 2D vertices and adjusted positions in 2D space (while still leaving the depth in tact), but no, the description is correct: it only moves 2D vertices (presumably only vertices using an orthographic projection instead of a perspective projection, but I haven't completely checked that). This means only user interface stuff is moved for the most part, but 3D models that are rendered in the distance still have full detail.

@JMC47
Copy link
Contributor

JMC47 commented Jan 29, 2022

I think changing the name would possibly confuse people, and most people don't know terminology like that. And to be honest, I wouldn't recognize that as a Viewport rounding issue at a glance (The Wii Sports Blue Line) so I'm okay with keeping the name as is. In fact, I'd prefer it. The stuff being in the description is fine enough. I'm just worried about regressions in other Vertex rounding games, so I'll do some testing later.

@Pokechu22
Copy link
Contributor Author

Yeah, I think to an end-user, the only way to tell the difference would be to try to enable vertex rounding, see that it didn't help, and wonder why. And I guess if you're willing to use terminology loosely, the corners of the viewport are vertices (though that's not how the viewport works internally), and we're rounding those like we'd round any other 2D vertices. So it's probably fine to leave the name as-is.

@Pokechu22 Pokechu22 force-pushed the viewport-rounding branch 2 times, most recently from 289d7a0 to 2fb7fac Compare February 11, 2022 18:39
@Pokechu22 Pokechu22 force-pushed the viewport-rounding branch 2 times, most recently from a4de530 to 2635a32 Compare February 23, 2022 01:24
@Pokechu22 Pokechu22 force-pushed the viewport-rounding branch 2 times, most recently from b0dc6e0 to 5a99b19 Compare April 7, 2022 19:08
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine. I'm not sure what side-effects this has, but since it's only on when a setting that we already know is hacky is on, it's probably fine.

@Pokechu22 Pokechu22 merged commit d7709d4 into dolphin-emu:master Apr 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants