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

Make Free Look a proper controller, move to separate UI #8867

Merged
merged 15 commits into from Dec 24, 2020

Conversation

iwubcode
Copy link
Contributor

@iwubcode iwubcode commented Jun 13, 2020

This switches Free Look to be a proper controller in both the UI and internally. I was always planning on doing this but assumed it would receive push back due to many users feeling comfortable with the current approach.

The biggest advantage to doing this is that HotkeyManager was only meant for Hotkeys. So adding Gyro controls like I did in #8747 or other controls which I have planned, is really gross code-wise.

But there are a couple other reasons I'm doing this:

The freelook controllers don't really fit into any current category ("Inputs", "Graphics", etc). So I've introduced another menu option "Free Look Settings".

image

The Free Look controller screen loses the description information it had previously. I don't think this information should exist in Dolphin, we don't have any other input specific descriptions in other places. Therefore I'm hoping to add a new wiki page for Free Look. I've added a link to this page to describe the different modes and various options supported. This will be expanded as more of these PRs get merged. @mbc07 hoping we can work together on this if possible or someone else who can help me with the wiki.

image

I took the liberty to make the change everyone was asking for in #8747. The "General" tab now spans horizontally. Additionally, I removed the "Zoom" since it isn't really a zoom and moved those controls to the "Move" section.

Code specific details

  • I moved speed under the camera. This is needed if/when we make multiple Free Look cameras. And could allow for game-specific speed settings in the future (maybe reusing the concept of game world units in meters like needed for WIP - OpenXR #8380 )
  • I am still reusing the HotkeyManager to call into the FreelookManager. This is to avoid adding another thread. Longer-term, maybe we need a thread to handle various pieces of input? This was a quick fix

@mbc07
Copy link
Contributor

mbc07 commented Jun 13, 2020

Will take a look at this very soon. Which wiki URL you're currently pointing to?

@iwubcode

This comment has been minimized.

Source/Core/DolphinQt/Config/Mapping/MappingWindow.h Outdated Show resolved Hide resolved
Source/Core/VideoCommon/FreeLookCamera.cpp Show resolved Hide resolved
Source/Core/Core/BootManager.cpp Outdated Show resolved Hide resolved
@mbc07
Copy link
Contributor

mbc07 commented Jun 13, 2020

About "Freelook" vs "Free Look", I don't have a strong opinion on neither, but I think the terminology should be consistent (e.g. if we end with "Free Look", it should be worded like that everywhere, on the UI and on the wiki URL as well). I would like to hear @MayImilae thoughts about this.

About the PR, it seems broken currently?

  • Nothing happens when you click on Controllers button in the toolbar
  • There's no device dropdown list on the Freelook Controller settings (Options => Other Settings => Configure), just the refresh button is there

@iwubcode

This comment has been minimized.

@iwubcode
Copy link
Contributor Author

@mbc07 - updated to fix your concerns

@iwubcode iwubcode marked this pull request as draft June 14, 2020 05:47
@mbc07
Copy link
Contributor

mbc07 commented Jun 15, 2020

The UI issues indeed are fixed on the current version of this PR. Just don't forget to change the wiki URL to be consistent with the UI (currently referring as "freelook").

I'm a bit busy at the moment but ping me when this PR is about to be merged so I can work on the wiki guide in case someone else doesn't do that before. Having that description of the camera modes as a starting point would be great but I couldn't find where exactly that was submitted (I'm pretty sure it was on GitHub, but where?)...

@iwubcode
Copy link
Contributor Author

iwubcode commented Jun 16, 2020

I'm still playing around with this but I have some ideas for the future.

Here's the rundown:

  • Dropped the previous enable / disable checkbox. Each controller/camera will have a "None" option just like the Gamecube controllers have. This allows you to turn on/off individual cameras. Important when dealing with multi-screen
  • Changed "Configure" to "Configure Controller". Admittedly a bit more verbose but I want the freedom to add an "Options" button later, for non-controller specific options. And that's also why I...
  • Removed the freelook settings from the graphics ini and instead made a specific freelook ini for non-controller settings. Right now, one measly setting! However, this will be more important in the future. Here's what the setting looks like:
[Freelook.Camera1]
ControlType = 0

(that sets the control type to "None")

Decided to make this change now, instead of waiting until later when more users are potentially using all these new features.

--

One thing I'm still trying to figure out is how to handle the "toggle". Toggle makes less sense when the configuration it deals with is a dropdown! Still, I have something that works for now. I'm tempted to remove it and instead add keys for previous/next control type. Does anyone have any thoughts on this?

@iwubcode
Copy link
Contributor Author

@mbc07 - sure, I'll ping you again when things are ready to go!

FYI I started on the wiki page - https://wiki.dolphin-emu.org/index.php?title=Freelook . I wasn't sure if there's a way to hide it from the public and/or mark it as in development..

@mbc07
Copy link
Contributor

mbc07 commented Jun 16, 2020

I wasn't sure if there's a way to hide it from the public and/or mark it as in development..

Apart from some very specific administration-related special pages, everything on the wiki is public, so don't worry for now. I'll keep checking it from time to time and point you anything that might need attention in the meantime...

@iwubcode
Copy link
Contributor Author

Took a hint from #8884 and improved performance by not pulling from the onion config each frame. Instead, we copy a Freelook::Config object at the start of the frame to an "active" config struct and use that for access.

Cleaned up the code and so I think this is ready for review.

@mbc07 - please see what you think of the wiki, I'm not sure what else to add at the moment. Will have more once the other PRs are merged

@iwubcode iwubcode marked this pull request as ready for review June 19, 2020 16:24
@iwubcode iwubcode force-pushed the freelook_controller branch 4 times, most recently from 9f115cc to caad62b Compare June 27, 2020 18:03
@iwubcode
Copy link
Contributor Author

iwubcode commented Jun 27, 2020

With #8820 merged, I updated this review to include FoV (also updated wiki). Let me know what you think!

Source/Core/Core/ConfigLoaders/BaseConfigLoader.cpp Outdated Show resolved Hide resolved
Source/Core/Core/FreelookConfig.h Outdated Show resolved Hide resolved
{
if (!s_has_registered_callback)
{
::Config::AddConfigChangedCallback(
Copy link
Member

@BhaaLseN BhaaLseN Jun 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somehow feel this isn't the best location for this; but I don't think I can suggest a better one either.

In some way, it makes sense to only update g_Config (as your actual settings source for Freelook code) but not g_ActiveConfig (as a UI copy that doesn't necessarily need regular refreshing); but on the other hand it feels weird to drag the static bool around just because you only need it for one instance (vs. just adding the callback in the ctor, using this).

Actually: Is there any particular reason to keep g_ActiveConfig around (and UpdateConfig for that matter)? If the UI changes a setting and saves to config, the callback should be called anyways, which updates cameraConfig. Or is there some non-obvious dependency that I'm missing?

Copy link
Contributor Author

@iwubcode iwubcode Jun 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This structure was copied from VideoConfig, I went back and forth on keeping the comment (see https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/VideoCommon/VideoConfig.cpp#L64 ) or if it'd just be confusing. Since UpdateActiveConfig is still called via the video thread, I imagine the race condition could still occur but wasn't 100% sure. Thought it'd be better to be safe than sorry.

As for why this isn't just registered in the constructor? I'm not sure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can guess why VideoConfig does it (that is: keeping two copies; one for the UI update and another that is used by the video code; and swapping between them) - there is more than one member that might be updated during Refresh, and if it did that while a game was running (and unpaused) it might pick up one setting in the new state but another setting in the old state.

I suppose we can leave it at that, as long as we remember there are other places doing the same thing; for a later point if/when we change this.

Source/Core/Core/FreelookManager.cpp Outdated Show resolved Hide resolved
@iwubcode iwubcode force-pushed the freelook_controller branch 2 times, most recently from 38a7b57 to 48efc48 Compare June 28, 2020 05:04
Copy link
Contributor

@JMC47 JMC47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't been paying too much attention to this, but I only had to use it for like 2 minutes to realize that this is a huuuuuuuuuuuuge improvement.

Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, untested.

…figuration window. Launched from a new menu option - "Free Look Settings". The HotKeyScheduler still calls the Free Look functionality to reduce the total number of threads
…ed, ensuring that the configuration updates appropriately with any changes
…his ensures Free Look settings are copied at the start of the frame. Also update the camera's controller type at this time
@lioncash lioncash merged commit 26c097e into dolphin-emu:master Dec 24, 2020
10 checks passed
@iwubcode iwubcode deleted the freelook_controller branch December 26, 2020 04:37
}

void FreeLookController::Update()
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you get rid of the const auto lock = GetStateLock(); here? Was it already locked?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! This was not intentional. I don't know much about all the inner-workings of inputs, so I didn't notice. I'll open a PR to add it after doing the appropriate testing


if (IsHotkey(HK_FREELOOK_RESET, true))
g_freelook_camera.Reset();
FreeLook::UpdateInput();
Copy link
Contributor

@Filoppi Filoppi Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the InputGate is always open for the FreeLook controller, meaning it would run if the dolphin window was in the background, is that what you want? I think it should use the same background settings as the game input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, not sure you've seen that FreeLook won't run if Hotkeys are disabled, there is a
if (!HotkeyManagerEmu::IsEnabled()) return condition above, I'm guessing it should run independently of that setting.

Copy link
Contributor Author

@iwubcode iwubcode Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did miss that hotkeys can be disabled. But as I mention in this PR, this was a simple stop-gap to get things merged more quickly. Long term I'd expect there to be a separate input thread that calls both hotkey and FreeLook systems and that FreeLook would not be impacted at all by hotkeys.

I'd personally like to add a new "background input" setting specifically for FreeLook because while I'd assume users would want it to operate it similarly to the game, they might not. I'd also add an "Input Sources" button like in the main controller window. It's confusing to me if users have to go somewhere else to modify those settings for FreeLook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants