D3D: Move exclusive mode switching to UI thread.

[CrossVR]

This PR cleans up the implementation of exclusive fullscreen significantly by putting the UI thread in control of exclusive fullscreen switching. This eliminates all fullscreen switching code from SwapImpl() and moves it to DoFullscreen() and OnActive() where it can be implemented with significantly less code.

This should also take care of exclusive mode loops some people were encountering. And it allows you to enter/exit exclusive fullscreen even while paused.

To prevent concurrency errors with the backbuffer swap, this PR takes advantage of the deadlock fixes in PR #4439 by calling PauseAndLock() around exclusive fullscreen switching calls.

---

This change is

[lioncash]

Source/Core/DolphinWX/Frame.cpp, line 523 at r1 (raw file):

      if (SConfig::GetInstance().bRenderToMain)
        m_RenderParent->SetFocus();
      else if (RendererIsFullscreen() && g_ActiveConfig.ExclusiveFullscreenEnabled())

The if body should be braced if the else if body is.

You should probably extract this kind of thing to a helper function or something, this is pasted almost verbatim in two other places.

Something like:

// Name condition to something more applicable
void DoThing(bool condition, bool set_fullscreen)
{
  if (!condition)
    return;

  const bool was_unpaused = Core::PauseAndLock(true);
  g_renderer->SetFullscreen(set_fullscreen);
  Core::PauseAndLock(false, was_unpaused);
}

or whatever would keep it reasonably self-contained.

---

Comments from Reviewable

[lioncash]

Source/Core/VideoCommon/RenderBase.h, line 76 at r1 (raw file):

  virtual void SetViewport() {}
  virtual void SetFullscreen(bool enable_fullscreen) {}
  virtual bool IsFullscreen() { return false; }

IsFullscreen() should likely be a const function.

---

Comments from Reviewable

[CrossVR]

Review status: 0 of 19 files reviewed at latest revision, 2 unresolved discussions.

---

Source/Core/DolphinWX/Frame.cpp, line 523 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

The if body should be braced if the else if body is.

You should probably extract this kind of thing to a helper function or something, this is pasted almost verbatim in two other places.

Something like:

// Name condition to something more applicable
void DoThing(bool condition, bool set_fullscreen)
{
  if (!condition)
    return;

  const bool was_unpaused = Core::PauseAndLock(true);
  g_renderer->SetFullscreen(set_fullscreen);
  Core::PauseAndLock(false, was_unpaused);
}

or whatever would keep it reasonably self-contained.

I agree, will move it to a function.

---

Source/Core/VideoCommon/RenderBase.h, line 76 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

IsFullscreen() should likely be a const function.

That would imply the overrides of this function should also be const. That's not a qualifier I can give to the overrides in D3D which call a DXGI function to check the state.

---

Comments from Reviewable

[lioncash]

Source/Core/VideoCommon/RenderBase.h, line 76 at r1 (raw file):

Previously, Armada651 (Jules Blok) wrote…

That would imply the overrides of this function should also be const. That's not a qualifier I can give to the overrides in D3D which call a DXGI function to check the state.

Not sure I follow why that would even matter.`D3D::GetFullscreenState();` is part of a namespace. const would be OK in this instance since it's not modifying anything in the class itself.

---

Comments from Reviewable

[lioncash]

Review status: 0 of 20 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/DolphinWX/Frame.cpp, line 1183 at r2 (raw file):

void CFrame::DoExclusiveFullscreen(bool enable_fullscreen)
{
  if (g_renderer && g_renderer->IsFullscreen() != enable_fullscreen)

You can just do

if (!g_renderer || g_renderer->IsFullscreen() == enable_fullscreen)
    return;

which moves the main body back an indentation level.

---

Source/Core/DolphinWX/Frame.h, line 109 at r2 (raw file):

  bool RendererIsFullscreen();
  void DoFullscreen(bool bF);
  void DoExclusiveFullscreen(bool bF);

bool enable_fullscreen

---

Comments from Reviewable

[CrossVR]

Review status: 0 of 20 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/VideoCommon/RenderBase.h, line 76 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

Not sure I follow why that would even matter.D3D::GetFullscreenState(); is part of a namespace. const would be OK in this instance since it's not modifying anything in the class itself.

But I think that's a bit disingenuous, you'd expect a const function to be safe to call and not make any changes to any state whatsoever. That's not a guarantee that's given by GetFullscreenState(), in fact with a different parameter it'll actually increment a reference counter.

---

Comments from Reviewable

[lioncash]

Source/Core/VideoCommon/RenderBase.h, line 76 at r1 (raw file):

Previously, Armada651 (Jules Blok) wrote…

But I think that's a bit disingenuous, you'd expect a const function to be safe to call and not make any changes to any state whatsoever. That's not a guarantee that's given by GetFullscreenState(), in fact with a different parameter it'll actually increment a reference counter.

`const` on a class function applies specifically to the class it's implemented in (hence why they're called const _member_-functions), not global state. It indicates class state is not modified, which it isn't. const is correct here.

---

Comments from Reviewable

[CrossVR]

Review status: 0 of 20 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/DolphinWX/Frame.h, line 109 at r2 (raw file):

Previously, lioncash (Mat M.) wrote…

bool enable_fullscreen

Should I change the other one too?

---

Source/Core/VideoCommon/RenderBase.h, line 76 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

const on a class function applies specifically to the class it's implemented in (hence why they're called const member-functions), not global state. It indicates class state is not modified, which it isn't. const is correct here.

By that logic I could make SetFullscreen() const.

---

Comments from Reviewable

[lioncash]

Source/Core/VideoCommon/RenderBase.h, line 76 at r1 (raw file):

Previously, Armada651 (Jules Blok) wrote…

By that logic I could make SetFullscreen() const.

That doesn't really refute anything I said, but OK? Do that if you _really_ want to, but does that make sense in terms of the immediate class interface itself, especially if used in other backends? One queries for information, one has the literal written indication of modifying. A different implementation of a renderer may set a class variable in `SetFullscreen()`. It's why the other setters aren't const either.

---

Comments from Reviewable

[CrossVR]

Review status: 0 of 20 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/VideoCommon/RenderBase.h, line 76 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

That doesn't really refute anything I said, but OK? Do that if you really want to, but does that make sense in terms of the immediate class interface itself, especially if used in other backends? One queries for information, one has the literal written indication of modifying. A different implementation of a renderer may set a class variable in SetFullscreen(). It's why the other setters aren't const either.

I'll make the change, just wanted to clarify the reasoning.

---

Comments from Reviewable

[CrossVR]

Review status: 0 of 20 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/DolphinWX/Frame.cpp, line 1183 at r2 (raw file):

Previously, lioncash (Mat M.) wrote…

You can just do

if (!g_renderer || g_renderer->IsFullscreen() == enable_fullscreen)
    return;

which moves the main body back an indentation level.

Done.

---

Source/Core/DolphinWX/Frame.h, line 109 at r2 (raw file):

Previously, Armada651 (Jules Blok) wrote…

Should I change the other one too?

Done.

---

Source/Core/VideoCommon/RenderBase.h, line 76 at r1 (raw file):

Previously, Armada651 (Jules Blok) wrote…

I'll make the change, just wanted to clarify the reasoning.

Done.

---

Comments from Reviewable

[lioncash]

Review status: 0 of 20 files reviewed at latest revision, all discussions resolved.

---

Source/Core/DolphinWX/Frame.h, line 109 at r2 (raw file):

Previously, Armada651 (Jules Blok) wrote…

Done.

ah, sorry about not answering this question. I never got the notification for it.

---

Comments from Reviewable