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

Add imgui-based Netplay Chat #7903

Merged
merged 4 commits into from Mar 23, 2019

Conversation

7 participants
@spycrab
Copy link
Contributor

commented Mar 17, 2019

Screen Shot 2019-03-17 at 15 52 23

Known issues:

  • There's no hotkey for toggling the chat (Maybe later?)
  • Pressing Enter to send messages does not work yet
  • Not usable when controllers are mapped to the keyboard
  • The text cursor is not displaying properly

@spycrab spycrab added the WIP label Mar 17, 2019

Show resolved Hide resolved Source/Core/VideoCommon/NetplayUI.cpp Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetplayUI.cpp Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetplayUI.cpp Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetplayUI.cpp Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetplayUI.h Outdated
@Techjar

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

NetplayUI should probably be NetPlayChatUI instead for consistency. Note the capitalization.

@spycrab spycrab force-pushed the spycrab:imgui_np_chat branch 3 times, most recently from ca9a84f to 811b3a4 Mar 17, 2019

Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.h Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.h Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.h Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.h Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.h Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.h Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.cpp Outdated

@spycrab spycrab removed the WIP label Mar 17, 2019

@Techjar

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

Tested this today, looks great! Though I encountered a couple usability issues with it.

  1. I can't use the enter key to send chat messages, only the button works, which is rather inconvenient.
  2. No blinking cursor appears when the text box is focused, which is a bit confusing. From what I gathered imgui is supposed to be rendering one, so I'm not sure what's going on here.

@spycrab spycrab force-pushed the spycrab:imgui_np_chat branch from 811b3a4 to fe06a39 Mar 17, 2019

@spycrab spycrab force-pushed the spycrab:imgui_np_chat branch 4 times, most recently from abdd6ae to 829e9b4 Mar 18, 2019

@spycrab spycrab force-pushed the spycrab:imgui_np_chat branch 2 times, most recently from 37f0c06 to 9ed22b6 Mar 18, 2019

@spycrab

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@Techjar Should all be working now.

@spycrab spycrab force-pushed the spycrab:imgui_np_chat branch 2 times, most recently from 7e65d12 to ec71acb Mar 20, 2019

@spycrab spycrab force-pushed the spycrab:imgui_np_chat branch from ec71acb to 74de4e1 Mar 21, 2019

Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.h
Show resolved Hide resolved Source/Core/DolphinQt/NetPlay/NetPlayDialog.cpp Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.h Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.cpp Outdated
@stenzek
Copy link
Contributor

left a comment

ImGui portions seem okay otherwise, can't comment on the netplay interaction.

Show resolved Hide resolved Source/Core/VideoCommon/RenderBase.cpp Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.cpp
@@ -242,6 +243,8 @@ class Renderer
void BeginUIFrame();
void EndUIFrame();

std::unique_ptr<NetPlayChatUI>& GetNetPlayChatUI();

This comment has been minimized.

Copy link
@stenzek

stenzek Mar 23, 2019

Contributor

Rather than returning a reference to the unique_ptr (and adding an additional level of indirection), how about returning m_netplay_chat_ui.get(), that way callers can't modify the pointer.

Furthermore, why does instance need to be swapped during runtime anyway? Why not just create a single instance of it always, and change the callbacks as needed?

This comment has been minimized.

Copy link
@spycrab

spycrab Mar 23, 2019

Author Contributor

The idea was to actually abide by #7903 (comment). So the UI doesn't take up memory all of the time.

Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.cpp Outdated
Show resolved Hide resolved Source/Core/VideoCommon/NetPlayChatUI.cpp Outdated

@spycrab spycrab force-pushed the spycrab:imgui_np_chat branch from 74de4e1 to f3bdb6b Mar 23, 2019

@spycrab spycrab force-pushed the spycrab:imgui_np_chat branch from f3bdb6b to 7cfb626 Mar 23, 2019

@spycrab

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

This doesn't seem to be implementable in a proper way without a global variable, so I reintroduced it.

Reasons for that:

  • Greatly increases complexity
  • The availability of g_rendererat any given point varies greatly which makes it hard to interact with from the UI thread.

@spycrab spycrab merged commit c89139d into dolphin-emu:master Mar 23, 2019

9 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details

@spycrab spycrab deleted the spycrab:imgui_np_chat branch Mar 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.