-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
BPM: Add Network and HDD settings page #13294
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
Conversation
@TheLastRar all of these have been addressed if you'd like to give it another look |
TheLastRar
left a comment
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.
You've made the BackgroundProgressDialog a popup, (and thus blocks input), which is probably better for HDD creation.
The BackgroundProgressDialogs is used when achievement data is still downloading when loading a savestate
On inspection, this is might be okay also being a modal as the calling code blocks anyway
(I also couldn't force it to show up when I tried)
If we are making this a modal dialog, then the function would need renamed as its no longer a background progress bar.
Yeah, my intention was to use that for creating hard drives too now if sparse wasn't able to be used I will rename it though |
4905a32 to
e756a2b
Compare
|
ofc thats failing now hold on 😭 |
|
Are you able to resolve the new warnings here https://github.com/PCSX2/pcsx2/actions/runs/18546991379/job/52866873714#step:13:1015 |
|
alright that's fixed |
TheLastRar
left a comment
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.
Changes look okay, lightly tested on windows
JordanTheToaster
left a comment
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.
Settings and functions appear to work correctly from what testing I can do.
kamfretoz
left a comment
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.
Each option works correctly.
pcsx2/ImGui/FullscreenUI.cpp
Outdated
| if (file_size > 0) | ||
| { | ||
| const int size_gb = static_cast<int>(file_size / (1024LL * 1024LL * 1024LL)); | ||
| const bool uses_lba48 = (file_size > static_cast<s64>(120) * 1024 * 1024 * 1024); |
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.
The new code uses 1024 * 1024 * 1024 three times (a fourth line uses 1024LL * 1024LL * 1024LL). I'm sure the compiler already reduces this to a single number and that anyone reading this would know what that's doing, but maybe a constexpr might make this a bit more readable (and then, for that one line, just static_cast<long long>(ONE_GIGABYTE) or create a separate ONE_GIGABYTE_LL)? Just a suggestion since I don't know if people would find the triple-1024 more clear.
TheTechnician27
left a comment
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.
Maybe I'm mistaken about what the TRANSLATE_NOOPs do at the bottom of Fullscreen.cpp, but it doesn't look like any were added here despite quite a few user-facing messages being added. If I'm wrong, this PR is good to go; otherwise, that's the only thing holding back an approval right now.
Besides that, everything else looks good. The mutices (including in the new ImGuiFullscreen code) don't look deadlockable, the HDD creation process looks good and safe, and the network settings look like they reflect the good behavior of the UI.
At some point, we'll probably want to centralize some of the Qt and FSUI logic so that both can pull from it instead of rebuilding the wheel where possible.
TheTechnician27
left a comment
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.
Turns out I was mistaken before. Everything looks good to go.
F0bes
left a comment
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.
Tested a bit. Seems to work. Just a couple code things.
pcsx2/ImGui/FullscreenUI.h
Outdated
|
|
||
| namespace FullscreenUI | ||
| { | ||
| class HddCreateInProgress; |
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.
Forward declaration isn't necessary. HddCreateInProgress is only used in FullscreenUI.cpp, where it is defined.
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.
Yeah, no it's not it's been removed
pcsx2/ImGui/FullscreenUI.cpp
Outdated
| #include "USB/USB.h" | ||
| #include "VMManager.h" | ||
| #include "ps2/BiosTools.h" | ||
| #include "DEV9/AdapterUtils.h" |
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.
Doesn't look like this include is necessary?
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.
yup nope it's not I mistakenly added it (it's gone now as of latest push)
Signed-off-by: SternXD <[email protected]>
Signed-off-by: SternXD <[email protected]>
F0bes
left a comment
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.
Code looks good now, tested on macOS and it seemed to work fine.



(I apologize I accidentally messed up rebasing on the other PR so just decided to redo it.)
Description of Changes
This PR adds a complete Network and HDD settings page to FullscreenUI, bringing feature parity with the Qt interface for DEV9. This includes:
Rationale behind Changes
Previously, users had to exit FullscreenUI and use the Qt interface to configure network and HDD settings for DEV9. This created an inconsistent user experience and made it a horrible experience for anyone couch gaming.
The changes solve this by:
Suggested Testing Steps
Network Settings:
HDD Settings:
Cross-Platform:
Did you use AI to help find, test, or implement this issue or feature?
I did not use it for my code but it did help me with grammar and better helping understand why this should be added.