-
Notifications
You must be signed in to change notification settings - Fork 52
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
FF7: allow filtering of internal textures that were originally filtered. #764
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for coming up with PR. I'm not really sure what are we fixing here, if this is just your preference, or we assume that what the Steam release did is the right one. In the meantime we unpack this, please look at the comments I left which we need to work nevertheless on top.
Overall I'm not really a fan of adding flags, or better only if needed, so I'm trying to understand what are we really trying to bring up here and which would be the best way to bring it to players.
Nevertheless, thanks for working on this, looking forward to it :)
both the original pc release, and the steam release under default settings look like this. It was hard to find footage of OG pc release, but here it is, on an emulated voodoo graphics card it was designed for. https://youtu.be/LSkuTPRanuk?t=472 You can see the filtered buster sword texture, and the filtered battle textures. In the steam release, you can disable linear filtering, which makes things look like FFNX currently does it. Because both the steam release and the 98 cd release do this out of the box, i decided to make it a flag that defaults to true, with people who just extracted it into their steam version given the option to change it. I trust i explained my reasoning? If the original 98 release hadn't actually done this I wouldn't have bothered. Also, at some point FF8 may end up being able to filter textures too, and similar work will need to be done there. i was unable to get ff8 to filter any internal textures in my testing, or I would have written similar code for that game as well. I added the option because in my opinion, "the PSX version didn't do this" isn't a good enough reason to remove a feature. |
Starting from the fact that linear filtering is a visual preference, we don't have to do what the original driver does if we think it is not needed, especially because we allow those textures to be modded in the first place ( unlike the original game and driver which is intended to work only with the original assets ). Because of this, I was asking if this was on PSX because that's the version we use as a basis to be the "most polished one" as we all know the history behind the PC port and how crap it was, so a lot of things were left over. I understand you might want this feature in, but this is not an heavy argument for me to forcefully accept the PR. I am still open however to include it, but never I will accept statements like this:
Whatever we do here serves the community, not me, not you. So here you have "my PSX argument". Again, I would like to know what are we really trying to solve here, before I merge this code. I hope you do understand. On top of all of this, I also left some comments as there are still minor things to polish. Thanks! |
The psx didn't do it because the hardware couldn't do it. You can enable texture smoothing on a ps2 to turn this feature on for many games. Many emulators for the console can and will do it. But i wasn't even considering that. I don't see this as just a personal preference. In my opinion, Square has spoken. The fact that it's forced on the next gen remasters, with no way to shut it off is another argument in favor of "Square says they are supposed to be filtered". Also, not everyone has the disk space or ssd to high def upscale everything. Maybe they just want a retranslation. Maybe they just want voice acting. Maybe they just want their controller to actually work, and the game to become big picture compatible. Thanks for clarifying that this was a design decision, and not just something to stop glitches. I could not tell this from the code, as was under the impression that it was a technical limitation, which is why i felt the urge to try and fix it (and it fought me enough to reinforce that hypothesis) I'm sorry if I am coming across as hostile. This really is not my intention. I just see no reason the option shouldn't be there. Were it just for me, i'd just keep it to myself and my personal unmodded installation, and wouldn't bother with the pull request. Which is what i'll do if this isn't accepted. I think this is an improvement, because it gives people more control, and lets people who actually first played it on pc play the game they are used to, and gives an option for people who don't like the look of the AI upscales on low end hardware. |
src/renderer.cpp
Outdated
|
||
if (!internalState.bDoTextureFiltering || !internalState.bIsExternalTexture) flags |= BGFX_SAMPLER_MIN_POINT | BGFX_SAMPLER_MAG_POINT | BGFX_SAMPLER_MIP_POINT; | ||
if (!internalState.bDoTextureFiltering) flags |= BGFX_SAMPLER_MIN_POINT | BGFX_SAMPLER_MAG_POINT | BGFX_SAMPLER_MIP_POINT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this line to what it was before, this PR shouldn't touch the renderer at all. It took us quite an effort to get it working for both FF7 and FF8. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the renderer itself is what's hard blocking filtering of internal textures.
The pull request depends on me stopping this hard override. If i don't do this, nothing else in my pull request has any effect at all.
There's is literally no way to override this check. It was the entire reason turning bilinear filtering on did nothing before unless you were modding.
I understand your argumentation and I'm fine adding a new option on top. Let's find a way though to make it work without disrupting core parts as it's just a behavioral change. About movies, please keep them as they are, it was like this since the days of Aali's driver and users are accustomed to it. One more thing: I noticed all your indentation is using spaces. Please look careful on this PR change tab and check what is aligned with what was before ( for eg. if the file is using tabs, keep using tabs ). We'll eventually uniform all files to use spaces in a separate commit. Thank you. |
as i said. the core itself is forcefully shutting off filtering of literally everything that's not modded. I probably found all the same glitches you did, and instead decided to override on the offending textures only, while letting the others through. Would this be acceptable?
which would leave ff8 unaffected. I admit a better approach would be to whitelist the ones that are okay to filter (anything attached to a battel scene, or the world map, and the buster sword), but my current approach works in ff7 according to my testing, and sure looks like the original pc. |
Tell me how to place an override in for the internal movies, then. i can't figure it out. :) !(VREF(texture_set, ogl.external)) shows intellisense error when it's inside of gl_draw_movie_quad_common Okay think i figured that out. i now have a hard override for all internal movies, which is exactly what you asked for. // don't filter internal movies Except that the movies are still filtered(!). unless it's done in renderer. what the.... okay, seems you can't actually reference current_state.texture_set in gl_draw_movie_quad_common, because it's not actually set at the time. My attempt to check it was dead code, tested by commenting stuff out. Without that, you can't look at ogl.external. You can't check bIsExternalTexture outside of renderer.cpp, you can only set it. So i can't see a way to check if the movie is external or not at that point in the code. |
As stated above, please leave the |
There is exactly ONE part in the code that actually sets the bIsExternalTexture flag outside of renderer.cpp itself, and it also does other stuff. i don't know how to even reference newRenderer where I would need to to get access to that flag and change it. i can't do it in gl_special_case, which is obviously where the code should go, because it is a special case. I've hit my limit here. all commits cancelled on my repo, and resyncing my local source. |
so seems this is dead code, or nearly so. Line 132 in 7f3739e
as ogl.external is only ever set true by load_external_texture, which doesn't appear to be called for movies as near as I can tell. do tell me if i'm wrong here. Which had me wondering if i had revealed a bug after all, since the source was saying that movies were getting filtered, and yet they clearly were not, with this very difficult to verify for anything but movies that are low enough resolution that you can actually tell. |
Summary
This restores the missing filtering from all the 3d models when using the internal textures.
Motivation
This restores a missing feature that ffnx removes. FFNX was refusing to filter any internal textures. Now it only refuses to filter the ones that aren't safe to filter, as far as I know.
I'm aware this code is ugly and needs gardening, but i've tested it extensively (checked all the minigames), and it now appears to be at the point that it manages to replicate the look of the unmodded PC version when using the original assets.
ACKs
Closes #754 , in my opinion.