-
Notifications
You must be signed in to change notification settings - Fork 35
Refactoring #366
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
Refactoring #366
Conversation
Haven't been able to look through it much yet, from what I've read std::string should work fine with UTF-8 already though, other than doing doing things like string splits/regexes which might need extra handling, but don't think we really use those on file paths anyway. Our std::string/c_str()/char* uses probably do end up calling some Win32 xxxA funcs though, on Windows those recently had UTF-8 support added in ~Win10 (but might require a manifest flag to activate), don't know if WINE/Pluton supports that at all yet, maybe those still only support ASCII there, if so then changing to wstring/wchar_t/xxxW is probably the better choice. E: hm, using std::string to store UTF-8 does seem a little sketchy though, eg. using size() returns the number of bytes instead of actual character count, could imagine things breaking with that. Probably is better for us to move over to wstring/wchar instead. E2: eh seems Silent's patches also use wstring/wchar too, so guess you can ignore what I said :P Also it looks like he also fixed some game funcs by treating their strings as UTF-8 and converting them over to wchar_t with MultiByteToWideChar, for them to be used with xxxW: https://github.com/CookiePLMonster/SilentPatchMGR/blob/master/SilentPatchMGR/SilentPatchMGR.cpp#L322 I did try looking into one of the INI reading funcs in UHD before, seems they make use of std::ifstream to handle reading the file though, was hoping to patch it to std::wstring somehow but didn't seem like EXE had any std::ifstream ctor that would accept wstring, seemed like a pretty big task to add it too... maybe something similar to the UTF-8 method Silent used could work there though. |
True, yeah. I decided to move try and move to wstring/wchar because of the weirdness you mentioned. Seems UTF8 with regular strings should work fine as long as whatever you're passing it to knows/expects that the string is going to be UTF8-encoded. The ini reader, for example, doesn't do that, apparently ._. Edit: Latest commit makes use of namespaces for some things. Not sure about it, but I think it works/looks better this way? Let me know what you think. (I'll rebase whatever we choose to keep from here after we merge the trainer branch, so no worries with that :p) |
b780cbd
to
4959593
Compare
Just rebased this onto the trainer branch to make it easier to test/make changes. |
4959593
to
d8678c4
Compare
@emoose Just pushed the DXVK stuff. It would be nice to also have a way to test if the user's system supports Vulkan, and fallback to d3d9 if it isn't available. Haven't looked into that yet. Anyway, seems it does improve performance: |
You can select multiple files and open properties window to change settings on them all, can maybe just add the warnings to "Disable Specific Warnings" for them. Think that only needs to be applied to the .c/.cpp files of it, and maybe any of our .cpp that include dxvk headers too (unless we edit whatever headers they include) Nice performance diff there, could try enabling ( E: eh the results are a little weird actually, DX9 gets ~800FPS during the little "picking up radio" animation, but then after the radio call and I get chara control it drops to ~200FPS. E: ah okay, r103 farm gets ~257-270FPS with vulkan, and ~220-250FPS with dx9, so there is still an increase, but not as large as I thought it was :P |
Had a quick look through the code, the new namespacing seems pretty nice, There's also a few things I've been meaning to fix up, just some ideas to make things tidier, will probably try fixing them after we merge this refactor (but if anyone wants to look into them feel free):
|
Ah cool. Had no idea we could do it per file like that. Seems it does get rid of the warnings. Thanks!
Hmm, do you think there would be any issue when using #255 with vulkan?
Nice! I've read about Btw, I think you should be able to push to this branch, right? If not, I can just merge this right now to net get in your way. Been thinking about just going ahead and merging #255, even though not many people tested it so far. Do you think we should be safe-ish to do that? Edit: Looks like VS's profiler picks up the fakepdb if we use ChkMatch as described here: |
Yeah we'd probably be fine merging #255, I never had any issues with it at least, what do you think of making MultithreadFix enabled by default too? Maybe a better chance of finding out if there are any problems with it with certain configurations. Still need to add some cfgMenu options for that too, I'll add that now. (E: updated it, also moved to framerate section & made it default enabled, let me know if you think it should be disabled instead) About dxvk, it seemed to work fine with the 1.10.3 release at least, haven't tried it with our async enabled ver though. |
Sounds good to me. I was about to suggest leaving it enabled by default. |
d03aba2
to
f7c9302
Compare
Just merged the MT fix here and did some testing in the cave area after the village (same position, camera angle, etc): VK + MT fix enabled: 470 fps Quite a nice improvement, even if it can change depending on the area you are in or your camera angle. Edit: Also been playing a bit of mercenaries with |
Game only makes use of `__cdecl` & `__thiscall`, IDA defaulted to `__stdcall` initially though, so some early func defs used that. `__fastcall` gets used to emulate `__thiscall` since VS doesn't allow using `__thiscall` directly (it matches up to thiscall pretty closely, besides second param being tied to `edx`, which we just define as unused) Only usages of `__stdcall` should be Win32 related funcs.
Ah, by the way, just noticed Edit: The d3d9 fallback was surprisingly simple to implement. Seems to be working fine too. Leaving |
Ah I was thinking of moving that to frame rate too, would probably make more sense there. Is that BTW just wondering, have you started on any changelog for the stuff after 1.7.7.6? I'm not even sure where to start with that lol, I guess we can just look through the commits since that release: 1.7.7.6...master |
Ah, yes, it does help with that. It is a "new" fix, but I've had it in a local branch more or less ever since I posted that comment. It makes the water much more similar to the GC version, but I have an impression that the workaround isn't a 1-1 perfect match with GC. Still much better than the jelly water, though :p
Ouch, 200 commits since then? lol |
@emoose, here's what I got so far for the changelog:
Should probably go more in-depth about all the trainer features, though. Besides that, I think I haven't missed anything? There also PR #369. I'm not sure if that's ready to merge yet. |
Yeah, it's just a relatively minor issue that I got stuck on. Everything else tested okay. |
@nipkownix looks good! Seems pretty complete too, nothing missing really came to mind going through it at least, guess trainer stuff might need it's own separate changelog though lol. If you want to do a release soon there are a couple things I might try getting added, the item glow fix at #331 (comment) seems to work fine, should only affect Saddler there (the fix there is more a workaround then a real fix tho, need to check GC ver and see if it acts any different there, if we do find a better fix we can always add it in a later update though), will try PRing it soon. Haven't been able to test them much yet but think @pas-de-2's NTSC difficulty fixes should be fine to merge, mostly seems to be enabling existing code that changes the difficulty, so don't think there should be any huge issues with it, if there are problems users can always just disable the patch I guess :p Did find a way to improve the "disable enemy targetting" patch as well to let it work on more enemies, that needs a bunch more testing though, maybe something for a later release. |
Made a quick WIP changelog for the trainer, feel free to edit if wanted (or disregard if you already started on one :p), not sure if I might have forgot something here though...
E: ran the list through ChatGPT, had to fix up a couple small issues but seemed to improve it a little (I like the new intro it made :p) |
Just pushed some changes to settings, mainly moving some stuff to the new Overrides tab. Should probably make sure everything is still saving/loading fine and that I've missed nothing. Thanks for the trainer changelog btw. I had no idea where to begin with that :p |
Huh, sounds like a good idea to me, actually. I remember being against adding new tabs in the past (before the UI redesign, I think it had something to do with that?) for some reason, but even the Trainer tab wasn't a thing back then. @pas-de-2, would you like to open a PR for that? If not, I could just do it myself. |
Sure, up |
Updated changelog:
Just gotta make sure everything is saving/loading fine, and that I haven't missed anything from the changelog. |
@nipkownix looks like a couple PRs might be missing there, I think it's just these in the pic + the NTSC difficulty setting, besides those it looks good, can't think of much else that's missing. |
Oh yeah, we're missing the Saddler fix and NTSC mode. Nice catch! |
Good job with the release, great to see we made it in time for Christmas 🥳 I added a tiny section in the release text about updating from HD Project 1.1, think the instructions there should be correct, but feel free to change it if not. (mostly only added that section because some other sites like PCGW used to mention deleting all re4_tweaks files before updating, which I don't think we want :P) Anyway have a good Christmas! |
Thanks!
Ohh, nice thinking. I'll probably copy that over to the Nexus pages too. Thank you, and have a good Christmas as well! |
@emoose Could you give me your opinion on this PR draft? This makes some changes I've been meaning to make for some time. Biggest change being the wrappers, but I'm also trying to improve support for running from paths that contain UTF-8 chars.
While messing with the game on Linux, I noticed that the game crashes on start if the path it is running from contains special characters such as Japanese words. Before I could even dig into that issue, I realized something else: re4_tweaks itself was also having some trouble with special chars in paths.
I've changed some of our stuff to use
wstring
instead, but some other issues remain: I couldn't get our .ini reader to read files inside a path with special chars. There's also thelooseFilePath
from our sideload feature, which I haven't tested with special chars yet, but I imagine it wouldn't work as is.Of course, none of this special/UTF-8 chars stuff is really important right now unless we can make the game itself run from paths with those. (Do people even do that normally? Regardless, I imagine the game might also fail to load its own config.ini from the Documents path if the user's account name contains special chars..)
Do you think any of these things would be best left as is? The wrappers seem to be working nicely (from my tests). The
wstring
changes also aren't hurting, but they're pretty much useless right now, so no worries if we end up discarding this PR.Ah, this should also fix the LNK4098 warning related to freetype. I noticed you pushed a workaround in 9bccd1f earlier. Turns out the problem was my fault for not including a debug version of the .lib as well. Sorry about that :p