Fix stretched BGAs in 9th and 10th style #249

Merged
JeffPaine4890 merged 22 commits from fix-stretched-bgs-for-iidx-9-and-10 into master 2023-06-25 14:02:03 +03:00
JeffPaine4890 commented 2023-04-11 01:30:57 +03:00 (Migrated from github.com)

Resolves #238

Only a couple things I'm not sure about:

  1. This greatly expands the range of where the vertices can be for the function to identify it as a BGA that needs to be fixed. I doubt this will be an issue because I can't think of any situation where any of the games play a video even remotely in those positions outside of a song, but you'd probably know better than me if there would be any issues on that front

  2. I did notice you were checking a lot of the vertex positions to a specific value +/- 0.1, in my implementation I simply used the values as the edge of a >= or <= case. Is that extra 0.1 margin of error necessary, or was that just implemented as a "just in case?" If you think it's necessary we could probably just bump the edges another 0.1 (i.e. instead of checking for between 164 and 168 we check for between 163.9 and 168.1)

If you need it for anything, here are the vertex positions for the 9th/10th videos:

single (9th and 10th)
165,0 476,0 476,415 165,415

double UL
 (9th) 11,24 137,24 137,192 11,192
(10th) 11,26 137,26 137,214 11,214

double DL
 (9th) 11,200 137,200, 137,368 11,368
(10th) 11,206 137,206, 137,394 11,394

double UR
 (9th) 500,24 626,24 626,192 500,192
(10th) 500,26 626,26 626,214 500,214

double DR
 (9th) 500,200 626,200 626,368 500,368
(10th) 500,206 626,206 626,394 500,394
Resolves #238 Only a couple things I'm not sure about: 1. This greatly expands the range of where the vertices can be for the function to identify it as a BGA that needs to be fixed. I doubt this will be an issue because I can't think of any situation where any of the games play a video even remotely in those positions outside of a song, but you'd probably know better than me if there would be any issues on that front 2. I did notice you were checking a lot of the vertex positions to a specific value +/- 0.1, in my implementation I simply used the values as the edge of a >= or <= case. Is that extra 0.1 margin of error necessary, or was that just implemented as a "just in case?" If you think it's necessary we could probably just bump the edges another 0.1 (i.e. instead of checking for between 164 and 168 we check for between 163.9 and 168.1) If you need it for anything, here are the vertex positions for the 9th/10th videos: ``` single (9th and 10th) 165,0 476,0 476,415 165,415 double UL (9th) 11,24 137,24 137,192 11,192 (10th) 11,26 137,26 137,214 11,214 double DL (9th) 11,200 137,200, 137,368 11,368 (10th) 11,206 137,206, 137,394 11,394 double UR (9th) 500,24 626,24 626,192 500,192 (10th) 500,26 626,26 626,214 500,214 double DR (9th) 500,200 626,200 626,368 500,368 (10th) 500,206 626,206 626,394 500,394
icex2 (Migrated from github.com) reviewed 2023-04-11 01:39:08 +03:00
@@ -12,3 +12,3 @@
# Fix stretched BG videos on newer GPUs. Might appear on Red and newer
# Fix stretched BG videos on newer GPUs. Might appear on SIRIUS and older. On 9th and 10th style this issue may only affect older BGAs (from 1st-3rd style)
gfx.bgvideo_uv_fix=false
icex2 (Migrated from github.com) commented 2023-04-11 01:39:08 +03:00
# Fix stretched BG videos on newer GPUs. Might appear on 9 to 17

I think we don't have to be that specific in the config file here. Maybe worth mentioning that last part about some BGAs only affected on 9 to 10 in a comment in the code.

```suggestion # Fix stretched BG videos on newer GPUs. Might appear on 9 to 17 ``` I think we don't have to be that specific in the config file here. Maybe worth mentioning that last part about some BGAs only affected on 9 to 10 in a comment in the code.
icex2 (Migrated from github.com) reviewed 2023-04-11 01:39:42 +03:00
icex2 (Migrated from github.com) commented 2023-04-11 01:39:42 +03:00

This looks weird. Did you create copies or move the config files for 18 to 26 by accident?

This looks weird. Did you create copies or move the config files for 18 to 26 by accident?
JeffPaine4890 (Migrated from github.com) reviewed 2023-04-11 01:41:24 +03:00
JeffPaine4890 (Migrated from github.com) commented 2023-04-11 01:41:24 +03:00

Yeah I don't know why it's showing up like that. The only change in those files is deleting the "fix BGs" option which is never checked in the iidxhooks for those styles

Yeah I don't know why it's showing up like that. The only change in those files is deleting the "fix BGs" option which is never checked in the iidxhooks for those styles
icex2 (Migrated from github.com) reviewed 2023-04-11 01:41:36 +03:00
icex2 (Migrated from github.com) left a comment

I did notice you were checking a lot of the vertex positions to a specific value +/- 0.1, in my implementation I simply used the values as the edge of a >= or <= case. Is that extra 0.1 margin of error necessary, or was that just implemented as a "just in case?" If you think it's necessary we could probably just bump the edges another 0.1 (i.e. instead of checking for between 164 and 168 we check for between 163.9 and 168.1)

If you compare doubles/floats, you cannot use equals as there is always some error marging that they won't match exactly. That's what the 0.1 is for.

> I did notice you were checking a lot of the vertex positions to a specific value +/- 0.1, in my implementation I simply used the values as the edge of a >= or <= case. Is that extra 0.1 margin of error necessary, or was that just implemented as a "just in case?" If you think it's necessary we could probably just bump the edges another 0.1 (i.e. instead of checking for between 164 and 168 we check for between 163.9 and 168.1) If you compare doubles/floats, you cannot use equals as there is always some error marging that they won't match exactly. That's what the 0.1 is for.
JeffPaine4890 (Migrated from github.com) reviewed 2023-04-11 01:43:41 +03:00
@@ -12,3 +12,3 @@
# Fix stretched BG videos on newer GPUs. Might appear on Red and newer
# Fix stretched BG videos on newer GPUs. Might appear on SIRIUS and older. On 9th and 10th style this issue may only affect older BGAs (from 1st-3rd style)
gfx.bgvideo_uv_fix=false
JeffPaine4890 (Migrated from github.com) commented 2023-04-11 01:43:41 +03:00

I figured some users might not realize their videos are stretched since it doesn't affect all of them, this would spur them to check an affected song to see if they need to enable the option. (As opposed to later styles where all videos are affected) But that can be removed if you think it's unnecessary

I figured some users might not realize their videos are stretched since it doesn't affect all of them, this would spur them to check an affected song to see if they need to enable the option. (As opposed to later styles where all videos are affected) But that can be removed if you think it's unnecessary
icex2 (Migrated from github.com) reviewed 2023-04-11 01:44:16 +03:00
@@ -638,2 +635,2 @@
iidxhook_util_d3d9_float_equal(vertices[1].y, 0.0f, 0.1f)) &&
(vertices[2].x >= 472.0f && vertices[2].x <= 476.0f &&
(vertices[0].x >= 164.0f && vertices[0].x <= 168.0f &&
iidxhook_util_d3d9_float_equal(vertices[0].y, 0.0f, 0.1f) &&
icex2 (Migrated from github.com) commented 2023-04-11 01:44:16 +03:00

Seeing in your PR description that you have the exact numbers, I would use these instead of ranges. That makes the heuristic more specific which is preferred if you have exact numbers. The larger the rage, the higher the "risk" that this might be applied to another texture as well.

Seeing in your PR description that you have the exact numbers, I would use these instead of ranges. That makes the heuristic more specific which is preferred if you have exact numbers. The larger the rage, the higher the "risk" that this might be applied to another texture as well.
icex2 (Migrated from github.com) reviewed 2023-04-11 01:46:37 +03:00
@@ -12,3 +12,3 @@
# Fix stretched BG videos on newer GPUs. Might appear on Red and newer
# Fix stretched BG videos on newer GPUs. Might appear on SIRIUS and older. On 9th and 10th style this issue may only affect older BGAs (from 1st-3rd style)
gfx.bgvideo_uv_fix=false
icex2 (Migrated from github.com) commented 2023-04-11 01:46:36 +03:00

Ok, fair point that it might not be obvious to the user. Let's keep your version then.

Can you also adjust that description in src/main/iidxhook-util/config-gfx.c?

Ok, fair point that it might not be obvious to the user. Let's keep your version then. Can you also adjust that description in `src/main/iidxhook-util/config-gfx.c`?
icex2 (Migrated from github.com) reviewed 2023-04-11 01:48:31 +03:00
icex2 (Migrated from github.com) commented 2023-04-11 01:48:30 +03:00
image

Look closely at the path. Your changed files are outside of dist/iidx for some reason. If you move these files inside dist/iidx that might fix it already and show correctly in the change set.

<img width="354" alt="image" src="https://user-images.githubusercontent.com/1855103/231013356-967f43f6-1a9d-4568-8d06-c08824118293.png"> Look closely at the path. Your changed files are outside of `dist/iidx` for some reason. If you move these files inside `dist/iidx` that might fix it already and show correctly in the change set.
JeffPaine4890 (Migrated from github.com) reviewed 2023-04-11 01:50:55 +03:00
JeffPaine4890 (Migrated from github.com) commented 2023-04-11 01:50:55 +03:00

Whoops, I must have been in the wrong directory, I'll fix that >_>

Whoops, I must have been in the wrong directory, I'll fix that >_>
JeffPaine4890 (Migrated from github.com) reviewed 2023-04-11 01:51:26 +03:00
@@ -12,3 +12,3 @@
# Fix stretched BG videos on newer GPUs. Might appear on Red and newer
# Fix stretched BG videos on newer GPUs. Might appear on SIRIUS and older. On 9th and 10th style this issue may only affect older BGAs (from 1st-3rd style)
gfx.bgvideo_uv_fix=false
JeffPaine4890 (Migrated from github.com) commented 2023-04-11 01:51:26 +03:00

No problem

No problem
JeffPaine4890 (Migrated from github.com) reviewed 2023-04-11 03:36:31 +03:00
@@ -638,2 +635,2 @@
iidxhook_util_d3d9_float_equal(vertices[1].y, 0.0f, 0.1f)) &&
(vertices[2].x >= 472.0f && vertices[2].x <= 476.0f &&
(vertices[0].x >= 164.0f && vertices[0].x <= 168.0f &&
iidxhook_util_d3d9_float_equal(vertices[0].y, 0.0f, 0.1f) &&
JeffPaine4890 (Migrated from github.com) commented 2023-04-11 03:36:30 +03:00

Alright, that if statement is breaking my brain but I tested it on 9th, 10th, and newer styles and everything seems to be working fine now.

Alright, that if statement is breaking my brain but I tested it on 9th, 10th, and newer styles and everything seems to be working fine now.
icex2 commented 2023-04-11 10:42:37 +03:00 (Migrated from github.com)

Thanks for the effort. I will squash merge that one later as the amont of changes is very small and the commit history is too detailed in this case. I will also try to improve readibility on that if-block because it's in pretty bad shape now.

Some learning/reading resources for future contributions as we see great value in a linear and well broken down git history for future reference:

There is a lot more out there, but these should serve as entry points to the different topics and knowing what you need to google for.

Take some time and have a read, I hope these are valuable and help.

Thanks for the effort. I will squash merge that one later as the amont of changes is very small and the commit history is too detailed in this case. I will also try to improve readibility on that if-block because it's in pretty bad shape now. Some learning/reading resources for future contributions as we see great value in a linear and well broken down git history for future reference: * [Short overview article on squash and rabase](https://medium.com/swlh/squash-and-rebase-git-basics-5cb1be1e0dac) * [Slightly more in-depth article about rebase](https://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html) * [Classic "how to write good git commit messages" article](https://cbea.ms/git-commit/) * [Extended reading: Semantic/conventional commit messages](https://www.conventionalcommits.org/en/v1.0.0/): I only started doing this recently and found this a very helpful framework to break down changes more thoughtful. There is a lot more out there, but these should serve as entry points to the different topics and knowing what you need to google for. Take some time and have a read, I hope these are valuable and help.
JeffPaine4890 commented 2023-04-11 19:54:30 +03:00 (Migrated from github.com)

Thanks for the links

As for the if block, when I was working on it I found it a great help just to put an empty line between the checks for vertices[0].x, vertices[0].y, vertices[1].x, etc. I wasn't sure if that would conflict with the existing code style so I removed all the blank lines before I committed. But maybe just putting some comments in there to separate things would help.

Thanks for the links As for the if block, when I was working on it I found it a great help just to put an empty line between the checks for vertices[0].x, vertices[0].y, vertices[1].x, etc. I wasn't sure if that would conflict with the existing code style so I removed all the blank lines before I committed. But maybe just putting some comments in there to separate things would help.
icex2 commented 2023-04-15 16:57:41 +03:00 (Migrated from github.com)

Sorry for the late reply, life kept me busy during the week. I will move forward and merge your branch once I wrapped up some work of my own. I found a nice way to de-clutter the module and also improve the UV patching code to make it more readable.

If you are interested, you can take a peek at these helpers on my PR already. I want to copy-paste and tweak them for the UV patching.

Sorry for the late reply, life kept me busy during the week. I will move forward and merge your branch once I wrapped up some work of my own. I found a nice way to de-clutter the module and also improve the UV patching code to make it more readable. If you are interested, you can take a peek at [these helpers on my PR](https://github.com/djhackersdev/bemanitools/pull/250/files#diff-8446ec8512a1ff006e52488ed7c4101583ced7436816880ead8dc202c84ad2e7R26-R75) already. I want to copy-paste and tweak them for the UV patching.
icex2 (Migrated from github.com) approved these changes 2023-06-25 13:58:56 +03:00
icex2 (Migrated from github.com) left a comment

Managed to have some spare time, energy and motiviation to have another look at this. Realized it wasn't smart to block this because of the cleanup that I will likely not get around to that soon. Sorry that I let this somewhat rot for the last 2-3 months.

The contribution looks definintely fine and is well documented. The latter should help with any follow-up fixes and cleanup down the road, if even required.

Therfore, lgtm and merging this. Will be included in the next release. Will try to do better next time regarding expectation management.

Managed to have some spare time, energy and motiviation to have another look at this. Realized it wasn't smart to block this because of the cleanup that I will likely not get around to that soon. Sorry that I let this somewhat rot for the last 2-3 months. The contribution looks definintely fine and is well documented. The latter should help with any follow-up fixes and cleanup down the road, if even required. Therfore, lgtm and merging this. Will be included in the next release. Will try to do better next time regarding expectation management.
Sign in to join this conversation.