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

Netplay: Simplify save data options. #11047

Merged

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Sep 11, 2022

Reduces the amount of different netplay settings for syncing save data from four independent boolean options to one tri-state option and one boolean option.

Old options:

  • Write Save Data On / Off
  • Load Wii Save On / Off
  • Sync Saves On / Off
  • Sync All Wii Saves On / Off

(don't ask me what some of these combinations even do)

New options:

  • No Save Data / Load Host's Save Data Only / Load and Write Host's Save Data
  • Use All Wii Saves On / Off

This should both make more sense to the user and make it easier to reason about the code.

@AdmiralCurtiss AdmiralCurtiss force-pushed the netplay-save-options-rework branch 2 times, most recently from 14955d2 to caad998 Compare September 12, 2022 21:27
@AdmiralCurtiss AdmiralCurtiss marked this pull request as ready for review September 12, 2022 21:28
@AdmiralCurtiss
Copy link
Contributor Author

Okay, I think this is ready now. Please review/test.

@lioncash
Copy link
Member

Code LGTM. Should be fine to merge once testing is done and whatnot

@Techjar
Copy link
Contributor

Techjar commented Sep 18, 2022

Pinging @MayImilae for opinions on the phrasing of the options.

@MayImilae
Copy link
Contributor

MayImilae commented Sep 18, 2022

Images would be nice~ (assuming this affects the GUI, I think so?)

@AdmiralCurtiss
Copy link
Contributor Author

Clipboard03

@MayImilae
Copy link
Contributor

MayImilae commented Sep 18, 2022

Oh yea, that's way better.

I'll look over this more thoroughly later for any possible fine tuning of the language in it, but immediately this is much improved.

@MayImilae
Copy link
Contributor

A few comments from thinking about this a bit.

  • Load and Write Host's Save Data - I'm unclear what this means exactly. Is this loading the host's save data and then saving to the host's save data? Or is it Loading the host's save data and saving it to the guests?
  • Use All Wii Save Data - I honestly don't know what this means. Given the context of the settings above it I am guessing this is like, the guest downloading and using all of the host's wii save data? Maybe? I'd like some explanation of what that does.
  • Strict Settings Sync - Naming is fine, but this could use a tooltip or something to explain what non-strict settings sync is.

@JMC47
Copy link
Contributor

JMC47 commented Sep 19, 2022

1: Load and Write to Host's savedata means it loads it for the netplay session and then writes back the changes to the host's NAND after the netplay session.
2: Use All Wii Save Data just means load all saves instead of just the active title. Useful for homebrew.

@AdmiralCurtiss
Copy link
Contributor Author

Yeah, I think JMC summarized it pretty well, but:

  • No Save Data -> No sava data is loaded from anywhere, and save data is discarded after the netplay session ends. (Same as previous 'Write Save Data' off, 'Sync Saves' off, 'Load Wii Save' off)
  • Load Host's Save Data Only -> The Host's save data is loaded, but all modifications are discarded after the netplay session ends. (Same as previous 'Write Save Data' off, 'Sync Saves' on, 'Load Wii Save' on)
  • Load and Write Host's Save Data -> The Host's save data is loaded, and modifications are written back to the Host's local save files after the netplay session ends. (Same as previous 'Write Save Data' on, 'Sync Saves' on, 'Load Wii Save' on)

The guest's saves are never involved in any way.

If 'Use All Wii Save Data' is checked (previously 'Sync All Wii Saves') then it will load/write all saves in the Host's NAND rather than just the one of the game being started.

I have written tooltips for these in case you missed it:

m_savedata_none_action = m_data_menu->addAction(tr("No Save Data"));
m_savedata_none_action->setToolTip(
tr("Netplay will start without any save data, and any created save data will be discarded at "
"the end of the Netplay session."));
m_savedata_none_action->setCheckable(true);
m_savedata_load_only_action = m_data_menu->addAction(tr("Load Host's Save Data Only"));
m_savedata_load_only_action->setToolTip(tr(
"Netplay will start using the Host's save data, but any save data created or modified during "
"the Netplay session will be discarded at the end of the session."));
m_savedata_load_only_action->setCheckable(true);
m_savedata_load_and_write_action = m_data_menu->addAction(tr("Load and Write Host's Save Data"));
m_savedata_load_and_write_action->setToolTip(
tr("Netplay will start usign the Host's save data, and any save data created or modified "
"during the Netplay session will remain in the Host's local saves."));
m_savedata_load_and_write_action->setCheckable(true);
m_savedata_style_group = new QActionGroup(this);
m_savedata_style_group->setExclusive(true);
m_savedata_style_group->addAction(m_savedata_none_action);
m_savedata_style_group->addAction(m_savedata_load_only_action);
m_savedata_style_group->addAction(m_savedata_load_and_write_action);
m_data_menu->addSeparator();
m_savedata_all_wii_saves_action = m_data_menu->addAction(tr("Use All Wii Save Data"));
m_savedata_all_wii_saves_action->setToolTip(tr(
"If checked, all Wii saves will be used instead of only the save of the game being started. "
"Useful when switching games mid-session. Has no effect if No Save Data is selected."));
m_savedata_all_wii_saves_action->setCheckable(true);

'Strict Settings Sync' is not a new option, that was there before, exactly named as-is -- it enforces some graphical settings that are usually fine to be different like internal resolution across all players. I see no reason to touch it in this PR, but if you think it's worth clarifying something here I can make a separate PR for it.

@Techjar
Copy link
Contributor

Techjar commented Sep 19, 2022

  • Strict Settings Sync - Naming is fine, but this could use a tooltip or something to explain what non-strict settings sync is.

It has one. As AdmiralCurtiss said, that's been there for a while. Here's that bit of code specifically so you can see the tooltip text.

m_strict_settings_sync_action = m_data_menu->addAction(tr("Strict Settings Sync"));
m_strict_settings_sync_action->setToolTip(
tr("This will sync additional graphics settings, and force everyone to the same internal "
"resolution.\nMay prevent desync in some games that use EFB reads. Please ensure everyone "
"uses the same video backend."));

To be clear, we currently cannot use the fancy styled tooltips on this widget, so it relies on generic platform-controlled tooltips. That might be addressable, but is definitely a task for a different PR.

@AdmiralCurtiss
Copy link
Contributor Author

Anyone wanna give more comments here? I'd like to see this merged sometime.

@JMC47
Copy link
Contributor

JMC47 commented Sep 23, 2022

this is a huge improvement over what we have.

@MayImilae
Copy link
Contributor

You know what, this is a definite improvement and the lack of clarity in a few options is covered by the tooltips. So I say LGTM!


But since I've already written this post anyway, I'll just say what bothered me.

  • Load and Write Host's Save Data - As someone who doesn't use Netplay, my immediate thought was that this would save the host's save to the guest system. I'm not sure how to make it better and the tooltip covers this so whatever I guess.

  • Use All Wii Save Data - IMO this should just say "Use All Host's Wii Save Data". But grammatically that's terrible and trying to fix it leads to "Use all of the Host's Wii Save Data" which is hella long. I get why it is how it is after thinking about it. And again, tooltips to the rescue.

@AdmiralCurtiss AdmiralCurtiss merged commit 571e300 into dolphin-emu:master Sep 24, 2022
11 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the netplay-save-options-rework branch September 24, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants