osu-framework
osu-framework copied to clipboard
ppy
Add driver specific vsync/flush workarounds
I tried my best to put things at logical places, but to me it still feels like a bit of a hack, especially where the driver metadata is propagated through out to be reachable by GameHost.
I'm not good at naming things, so things like GraphicsBackendMetadata and PlatformWorkaroundMode might have to be renamed as well.
Tested on Windows, macOS, and Linux (Solus), and it seems to work as intended. Tested with a different GPU as well on Windows, and it's also fine there.
Also made it that way, so this setting can be altered in real time from UI, if we ever decide that this should be an user-facing config.
I think this direction is okay, apart from it being a setting. I'd propose this is removed. At most, it should be adjustable via envvar or something like that, for developer use only. Users should not be able to accidentally set this setting and have their game in an unknown state.
I think this direction is okay, apart from it being a setting. I'd propose this is removed. At most, it should be adjustable via envvar or something like that, for developer use only. Users should not be able to accidentally set this setting and have their game in an unknown state.
Should I move it to debug settings (reset every launch due to no config file backing), or should I completely axe any config, and have it exclusively as an Environment variable?
I'd say no config.
And probably not even environment variable, as those who would be interested in changing these knobs would have to be able to compile for themselves. I don't want people shooting themselves in the foot by giving suggestions to others to "create a shortcut with this env variable to lower input latency" or whatever.
Going back to more down-to-earth matters, code style here is going to need some nudging, CI doesn't like it.
I'd say no config.
And probably not even environment variable, as those who would be interested in changing these knobs would have to be able to compile for themselves. I don't want people shooting themselves in the foot by giving suggestions to others to "create a shortcut with this env variable to lower input latency" or whatever.
I'll implement the changes proposed in the review comments, axe the configurability, not implement environment variable configurability, and we can leave this PR open until we decide if we need any user-facing accessibility or not.
Although personally I would still leave a moderately easy way to make this user-accessible, so we can instruct users with GPU problems (like in [ppy/osu#9851]) to test a few settings to determine what causes it. In fact, in that exact case I linked, I think it's a problem unique to the user who reported it, so they would somehow need to apply that fix to themselves without applying the a fix unnecessarily to everyone else, degrading everyone else's performance just because it's isolatable to a single user. The UHD 630 bug affects everyone on Windows and macOS, so that's why it requires the fix to be applied to everyone on Windows and macOS.
Going back to more down-to-earth matters, code style here is going to need some nudging, CI doesn't like it.
I know, I just read some rules somewhere which say to not push unnecessary commits which unnecessarily overload the CI, and since I had review comments asking for changes, I decided not to push that code style fix (I was already done with before sleep) until I was done with the new changes.
I've applied fixes for the remaining inspections so we can keep moving this forward.
As per discussion at multiple places, I have removed Windows workarounds, as it's an issue which has to be solved by the user by upgrading/downgrading their OS and driver.
On macOS however that is not possible, so macOS workarounds have to be kept for now.
To confirm intention: half of the enum doesn't ever get triggered, and this will currently only affect macOS users. What's the plan with the remaining workaround you've proposed here? Is it no longer required?
To confirm intention: half of the enum doesn't ever get triggered, and this will currently only affect macOS users. What's the plan with the remaining workaround you've proposed here? Is it no longer required?
I actually use all four enum values in my local builds, as I made a build for each of my laptops which I use, as some of them experience great latency improvements and FPS stabilization instead of just minor FPS increase. Slightly inconvenient to have to rebuild each update, but I guess it is what it is, and I have to deal with it.
I can trim down the code to what is actually used right now, and if we end up ever needing the deleted code, then we could just fish it back out from the git history.
Please confirm if the code has to be trimmed, and then I'll push the changes.