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

DolphinQt/HacksWidget: Convert accuracy slider to ConfigSlider #13237

Merged
merged 2 commits into from
Dec 31, 2024

Conversation

AdmiralCurtiss
Copy link
Contributor

See #13234 (comment)

We have to extend the ConfigSlider class a little bit for this use case, but it's pretty straightforward.

Less code and makes it work with the per-game config system. Seems like the obvious way to go to me?

@TryTwo
Copy link
Contributor

TryTwo commented Dec 25, 2024

Thanks! This is all I needed, I just couldn't think of how to code it well.

@TryTwo
Copy link
Contributor

TryTwo commented Dec 25, 2024

I did a quick test and it works. Properly enables/disables. Works with both game and normal gfx windows. Correct values are written and all that.

Only note I see about the code is that I think there's a general preference for optional.value()[i] over (*optional)[i].

@AdmiralCurtiss
Copy link
Contributor Author

Only note I see about the code is that I think there's a general preference for optional.value()[i] over (*optional)[i].

Those two are different use cases. x.value() throws if there is no value, *x is UB if there is no value. Or in other words, *x is if you have already checked if there is a value, and x.value() is if you have not checked.

Anyway, I realized this doesn't even need to be optional, since we can just use an empty vector for the standard identity mapping case.

@TryTwo
Copy link
Contributor

TryTwo commented Dec 30, 2024

Tested again. It works. I didn't notice any issues in the code.

@AdmiralCurtiss AdmiralCurtiss merged commit b8921b1 into dolphin-emu:master Dec 31, 2024
13 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the slider-mappings branch December 31, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants