Improve netplay UX #3588

ShimmerGlass · 2016-02-02T15:58:39Z

Improve netplay setup dialog UX

Focus "Hash Code" / "IP address" text box by default in "Connect"
Focus game list in "Host" tab
RETURN keypress now host/join depending on selected tab
Remember last hosted game

Join scenario:

Copy netplay code
Open netplay
Paste code
Press enter

Host scenario:

Open netplay
Go to host tab
Press enter

Improve NetPlay UX

Log ping to OSD under FPS
Remove PanicAlertT:
- Simply log message to netplay window
- Remove them when they are useless
Show some netplay message in OSD
- Chat messages
- Pad buffer changes
- Desync alerts
Stop the game consistently when another player disconnects / crashes
Prettify chat textbox

OSD:

Chat box:

lioncash · 2016-02-02T17:12:55Z

Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 177 [r1] (raw file):
last_hosted_game

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 178 [r1] (raw file):
Our coding style uses braces on the next line. Alternatively, you can just get rid of the braces.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 475 [r1] (raw file):
current_tab

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 477 [r1] (raw file):

switch (current_tab)
{
case CONNECT_TAB:
    DoJoin();
    break;
case HOST_TAB:
    DoHost();
    break;
}

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 485 [r1] (raw file):
current_tab

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 487 [r1] (raw file):

switch (current_tab)
{
case CONNECT_TAB:
    m_connect_ip_text->SetFocus();
    break;

case HOST_TAB:
    m_game_lbox->SetFocus();
    break;
}

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.h, line 28 [r1] (raw file):
These can be static constexpr instead of static const

Comments from the review on Reviewable.io

JosJuice · 2016-02-02T17:44:35Z

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 46 [r1] (raw file):
You should be able to use File::GetUserPath(F_DOLPHINCONFIG_IDX) instead of having this variable.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 260 [r1] (raw file):
This will overwrite any changes that other parts of Dolphin have been made to Dolphin.ini since the window was opened. You should only save an INI that you recently opened, like the old version of this function did.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 360 [r1] (raw file):
Same here.

Comments from the review on Reviewable.io

ShimmerGlass · 2016-02-02T20:04:27Z

@lioncash fixed the coding style issues
@JosJuice reverted the ini changes to be more atomic, but used F_DOLPHINCONFIG_IDX as you suggested.

ShimmerGlass · 2016-02-02T20:28:17Z

Ive added a commit here. Meant to do that earlier but was busy correcting PR issues and eating.

ShimmerGlass · 2016-02-02T20:45:12Z

Review status: 0 of 2 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 177 [r1] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 178 [r1] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 360 [r1] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 475 [r1] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 477 [r1] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 485 [r1] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 487 [r1] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.h, line 28 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

lioncash · 2016-02-02T20:53:47Z

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 112 [r4] (raw file):
Our coding style uses single-line comments. e.g.

// Will be overrided by OnDirectTraversalChoice, but is nescessary
// so that both inputs do not take up space

also overrided should be overridden

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 489 [r4] (raw file):
These Layout() calls can just be simplified to:

m_connect_ip_text->GetParent()->Layout();

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.h, line 30 [r4] (raw file):
Typo. This should be TRAVERSAL

Comments from the review on Reviewable.io

ShimmerGlass · 2016-02-02T20:58:27Z

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 112 [r4] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 489 [r4] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.h, line 30 [r4] (raw file):
Done.

Comments from the review on Reviewable.io

ShimmerGlass · 2016-02-04T07:49:52Z

Left to do :

Add option to show/hide netplay OSD messages
Try to remove some TraversalClient PanicAlertT's

lioncash · 2016-02-04T13:51:14Z

Review status: 0 of 9 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

Source/Core/Core/NetPlayClient.cpp, line 589 [r6] (raw file):
max_ping.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 322 [r6] (raw file):
ini_file

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 493 [r6] (raw file):
// Let the event propagate

Source/Core/DolphinWX/NetPlay/NetWindow.cpp, line 632 [r6] (raw file):
wxColour colour = *wxBLACK;

then just change the other color assignments to be stack variable assignments. (e.g. colour = wxColour(0, 150, 150);)

The code below would have produced a memory leak from explicit new.

Source/Core/DolphinWX/NetPlay/NetWindow.cpp, line 633 [r6] (raw file):
printed_msg

Source/Core/DolphinWX/NetPlay/NetWindow.h, line 40 [r6] (raw file):

enum class ChatMessageType
{
    Error,
    Info,
    In,
    Out
}

also a comment should be left explaining what In and Out actually represent (or use better names)

Source/Core/DolphinWX/NetPlay/NetWindow.h, line 70 [r6] (raw file):
player should be a const reference.

Source/Core/VideoCommon/OnScreenDisplay.cpp, line 22 [r6] (raw file):
s_msg_list

Source/Core/VideoCommon/OnScreenDisplay.cpp, line 23 [r6] (raw file):
s_named_msg_list

Source/Core/VideoCommon/OnScreenDisplay.cpp, line 32 [r6] (raw file):
This will cause a memory leak. The handle to the pointer is lost after this function ends. Also there's no need to use explicit new when unique_ptr exists.

Source/Core/VideoCommon/OnScreenDisplay.cpp, line 68 [r6] (raw file):
This is now literally duplicated code. Please refactor with a design that doesn't do that.

Source/Core/VideoCommon/OnScreenDisplay.h, line 27 [r6] (raw file):
enum class

Source/Core/VideoCommon/OnScreenDisplay.h, line 29 [r6] (raw file):
NetplayPing

Source/Core/VideoCommon/OnScreenDisplay.h, line 32 [r6] (raw file):
These should be constants above the Message struct in their own namespace to enforce scoping.

namespace Color
{
constexpr u32 CYAN   = ...;
constexpr u32 GREEN  = ...;
constexpr u32 RED    = ...;
constexpr u32 YELLOW = ...;
}

Source/Core/VideoCommon/OnScreenDisplay.h, line 40 [r6] (raw file):
Similarly here.

namespace Duration
{
constexpr u32 SHORT     = 2000;
constexpr u32 LONG      = 5000;
constexpr u32 VERY_LONG = 10000;
}

Comments from the review on Reviewable.io

delroth · 2016-02-04T14:11:42Z

@lioncash @JMC47 should this go in 5.0? As in, is it important enough compared to the risks?

ShimmerGlass · 2016-02-04T20:09:52Z

Sorry for camelCase @lioncash. Again. Old habits die hard i guess.

mathieui · 2016-02-04T20:15:42Z

Source/Core/VideoCommon/OnScreenDisplay.h, line 29 [r6] (raw file):
NetPlayPing (to be consistent)

Comments from the review on Reviewable.io

ShimmerGlass · 2016-02-04T20:34:52Z

Review status: 0 of 9 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

Source/Core/Core/NetPlayClient.cpp, line 589 [r6] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetWindow.cpp, line 632 [r6] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetWindow.cpp, line 633 [r6] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetWindow.h, line 40 [r6] (raw file):
It should more clear now.

Source/Core/DolphinWX/NetPlay/NetWindow.h, line 70 [r6] (raw file):
Done.

Comments from the review on Reviewable.io

lioncash · 2016-02-04T21:12:23Z

@Aestek No worries :)

@delroth As long as it gets tested and verified that behaviour is always consistent (e.g. via a few dry-runs with people on different OSes running Smash Bros or something over netplay), I don't see why not. Otherwise, I'd leave it out.

ShimmerGlass · 2016-02-04T21:33:36Z

@lioncash @delroth Testing this PR is pretty fast forward and ill start doing real world tests as soon as i can get my hand on a windows build of it. However NetPlay already makes Dolphin crash sometimes so it can be hard to tell if this PR is causing the crash (but im pretty confident it does not, I've ran a lot of local tests).

ShimmerGlass · 2016-02-05T00:28:04Z

Review status: 0 of 9 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 322 [r6] (raw file):
Done.

Source/Core/DolphinWX/NetPlay/NetPlaySetupFrame.cpp, line 493 [r6] (raw file):
Done.

Comments from the review on Reviewable.io

ShimmerGlass · 2016-02-05T00:29:12Z

Review status: 0 of 9 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

Source/Core/VideoCommon/OnScreenDisplay.cpp, line 22 [r6] (raw file):
Done.

Source/Core/VideoCommon/OnScreenDisplay.cpp, line 23 [r6] (raw file):
Now removed.

Source/Core/VideoCommon/OnScreenDisplay.cpp, line 32 [r6] (raw file):
Done.

Source/Core/VideoCommon/OnScreenDisplay.cpp, line 68 [r6] (raw file):
Done.

Source/Core/VideoCommon/OnScreenDisplay.h, line 27 [r6] (raw file):
Now removed.

Source/Core/VideoCommon/OnScreenDisplay.h, line 32 [r6] (raw file):
Done.

Source/Core/VideoCommon/OnScreenDisplay.h, line 40 [r6] (raw file):
Done.

Comments from the review on Reviewable.io

lioncash · 2016-02-05T00:32:34Z

Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Source/Core/DolphinWX/NetPlay/NetWindow.cpp, line 651 [r8] (raw file):
indentation here is off by one tab

Comments from the review on Reviewable.io

ShimmerGlass · 2016-02-05T00:34:37Z

Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Source/Core/DolphinWX/NetPlay/NetWindow.cpp, line 651 [r8] (raw file):
Done.

Comments from the review on Reviewable.io

Helios747 · 2016-02-05T02:42:39Z

@dolphin-emu-bot rebuild

lioncash · 2016-07-16T19:00:05Z

Source/Core/Core/NetPlayClient.cpp, line 31 [r26] (raw file):

// called from ---GUI--- thread
NetPlayClient::~NetPlayClient() {

Btw, I just noticed this, but why is everything formatted with the newline on the same line? We use brace on the next line.

Comments from Reviewable

lioncash · 2016-07-16T19:01:13Z

Source/Core/Core/NetPlayClient.cpp, line 31 [r26] (raw file):

Previously, lioncash (Mat M.) wrote…

Btw, I just noticed this, but why is everything formatted with the newline on the same line? We use brace on the next line.

err `formatted with the newline` -> `formatted with the brace`

Comments from Reviewable

ShimmerGlass · 2016-07-17T08:04:41Z

Review status: 0 of 13 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

Source/Core/Core/NetPlayClient.cpp, line 31 [r26] (raw file):

Previously, lioncash (Mat M.) wrote…

err formatted with the newline -> formatted with the brace

done.

Comments from Reviewable

ShimmerGlass · 2016-07-17T18:53:56Z

For the record this PR fixes a few crashes where a player makes the others crash by stopping the game or crashing himself.

jjdelvalle · 2016-07-18T15:34:24Z

Apparently this needs another rebase ;(

JMC47 · 2016-07-18T18:14:38Z

Netplay seems to be permanently disabling a lot of options until I close the emulator altogether.

jjdelvalle · 2016-07-18T18:23:33Z

@JMC47 isn't that outside the scope of this PR? The master branch build does that too. Unless you mean closing Dolphin altogether and not just the game.

JMC47 · 2016-07-18T18:32:44Z

Oh. Then I guess it is. When did that happen?

E2xD · 2016-07-18T18:32:52Z

I will confirm that after launching Netplay (even on master branch) Controller Settings and CPU Clock Override Settings are disabled. Permanently is a stretch as restarting dolphin fixes the situation. But as @clinchergt says, this should be addressed by another PR and is outside of the ramifications of this one. I say lgtm.

JMC47 · 2016-07-18T19:01:39Z

Did a more thorough test, even made sure Wiimote Netplay was still working.

Jofzar · 2016-07-19T02:10:02Z

@JMC47 Some of those were disabled in a PR, not sure which one it was, that must be a side effect of some of those. I had a quick look but couldn't find which PR it was, but im certain they exist.

JosJuice · 2016-07-19T09:50:31Z

Review status: 0 of 13 files reviewed at latest revision, 6 unresolved discussions.

Source/Core/DolphinWX/VideoConfigDiag.cpp, line 101 [r28] (raw file):

                "backend, so for the best emulation experience it's recommended to try "
                "both and "
                "choose the one that's less problematic.\n\nIf unsure, select OpenGL.");

The text reformatting changes in this file are not appropriate.

Comments from Reviewable

ShimmerGlass · 2016-07-23T18:29:23Z

@JosJuice cleaned all that

JMC47 · 2016-07-23T18:30:22Z

@Aestek LGTM behavior wise. Tested it throughly on GameCube and Wiimote Netplay.

Helios747 · 2016-07-23T18:41:54Z

Reviewed 1 of 13 files at r22, 2 of 10 files at r24, 2 of 5 files at r28, 6 of 6 files at r29.
Review status: 11 of 13 files reviewed at latest revision, 6 unresolved discussions.

Comments from Reviewable

* Focus "Hash Code" / "IP address" text box by default in "Connect" * Focus game list in "Host" tab * RETURN keypress now host/join depending on selected tab * Remember last hosted game * Remove PanicAlertT: * Simply log message to netplay window * Remove them when they are useless * Show some netplay message in OSD * Chat messages * Pad buffer changes * Desync alerts * Stop the game consistently when another player disconnects / crashes * Prettify chat textbox * Log netplay ping to OSD Join scenario: * Copy netplay code * Open netplay * Paste code * Press enter Host scenario: * Open netplay * Go to host tab * Press enter

Once a tab is selected the focus can be set directly from the page changed event. However once the tab was shown, the first tab order control element was given the focus by default. This was fixed by delaying the action and setting the focus after the default focus had been assigned.

E2xD · 2016-07-23T19:20:40Z

Tested and works as expected. LGTM :3

Jofzar · 2016-07-26T04:26:54Z

Not sure how I somehow missed it after all this testing, but the md5 check box is actually off centre with the game selection checkbox in the host window

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch 3 times, most recently from 3dcb09c to 6fb4729 Compare February 2, 2016 20:02

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch from 2b83938 to a524426 Compare February 2, 2016 20:32

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch from a524426 to 95dd451 Compare February 2, 2016 20:57

ShimmerGlass changed the title ~~Improve netplay setup dialog UX:~~ Improve netplay UX Feb 4, 2016

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch from 7f890f7 to 107555a Compare February 4, 2016 07:50

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch from 107555a to 325438c Compare February 4, 2016 20:31

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch from 325438c to 13aa736 Compare February 5, 2016 00:27

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch from 13aa736 to 3b31a6f Compare February 5, 2016 00:34

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch from 807ca4c to a9a7df5 Compare July 16, 2016 19:04

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch 3 times, most recently from 6b97cdf to 4a70891 Compare July 18, 2016 17:36

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch 2 times, most recently from 2570e3d to 4ae5d32 Compare July 23, 2016 18:26

Aestek and others added 3 commits July 23, 2016 20:58

Standardizes constant use

2ba4b22

ShimmerGlass force-pushed the feature/improve-netplay-dialog branch from 4ae5d32 to 67eb814 Compare July 23, 2016 18:58

ShimmerGlass mentioned this pull request Jul 24, 2016

NetPlay: One click host #4057

Closed

delroth merged commit d7de39e into dolphin-emu:master Jul 26, 2016

ShimmerGlass deleted the feature/improve-netplay-dialog branch July 26, 2016 08:20

Improve netplay UX #3588

Improve netplay UX #3588

Conversation

ShimmerGlass commented Feb 2, 2016

Improve netplay setup dialog UX

Join scenario:

Host scenario:

Improve NetPlay UX

lioncash commented Feb 2, 2016

JosJuice commented Feb 2, 2016

ShimmerGlass commented Feb 2, 2016

ShimmerGlass commented Feb 2, 2016

ShimmerGlass commented Feb 2, 2016

lioncash commented Feb 2, 2016

ShimmerGlass commented Feb 2, 2016

ShimmerGlass commented Feb 4, 2016

lioncash commented Feb 4, 2016

delroth commented Feb 4, 2016

ShimmerGlass commented Feb 4, 2016

mathieui commented Feb 4, 2016

ShimmerGlass commented Feb 4, 2016

lioncash commented Feb 4, 2016

ShimmerGlass commented Feb 4, 2016

ShimmerGlass commented Feb 5, 2016

ShimmerGlass commented Feb 5, 2016

lioncash commented Feb 5, 2016

ShimmerGlass commented Feb 5, 2016

Helios747 commented Feb 5, 2016

lioncash commented Jul 16, 2016

lioncash commented Jul 16, 2016

ShimmerGlass commented Jul 17, 2016

ShimmerGlass commented Jul 17, 2016

jjdelvalle commented Jul 18, 2016

JMC47 commented Jul 18, 2016

jjdelvalle commented Jul 18, 2016 • edited

JMC47 commented Jul 18, 2016

E2xD commented Jul 18, 2016 • edited

JMC47 commented Jul 18, 2016

Jofzar commented Jul 19, 2016

JosJuice commented Jul 19, 2016

ShimmerGlass commented Jul 23, 2016

JMC47 commented Jul 23, 2016

Helios747 commented Jul 23, 2016

E2xD commented Jul 23, 2016

Jofzar commented Jul 26, 2016

jjdelvalle commented Jul 18, 2016 •

edited

E2xD commented Jul 18, 2016 •

edited