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

Cache IsFullScreenMode and invalidate the value only when SizeChanged #3569

Merged
merged 1 commit into from
Nov 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 37 additions & 23 deletions dev/Lights/MaterialHelper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -894,9 +894,12 @@ void MaterialHelper::OnVisibilityChanged(const winrt::CoreWindow&, const winrt::
// experience severe rendering lag in resize scenarios (Bug 13289165).
void MaterialHelper::OnSizeChanged(const winrt::IInspectable& /*sender*/, const winrt::IInspectable& /*args*/)
{
const bool isFullScreenOrTabletMode = IsFullScreenOrTabletMode();

const auto strongThis = get_strong();

// When IsFullScreen changes we get a SizeChanged event.
strongThis->m_isFullScreenModeValid = false;

const bool isFullScreenOrTabletMode = strongThis->IsFullScreenOrTabletModeImpl();
m_sizeChangedListeners(strongThis, isFullScreenOrTabletMode);
Comment on lines +902 to 903
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Technically we could just pass strongThis->IsFullScreenOrTabletModeImpl() to the event and skip saving it in a variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but it was using a local before and I think putting it inline reduces readability slightly. Unfortunately I can't find a CppCoreGuideline rule to support either style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good point. Since there doesn't seem to be a CppCoreGuideline rule governing this (I haven't found one too) and the CI already passed, I think we should leave it as is and merge the PR. Allocating a bool isn't too bad after all and readability profits from this design.

}

Expand Down Expand Up @@ -1007,34 +1010,45 @@ bool MaterialHelper::RS2IsSafeToCreateNoise()
/* static */
bool MaterialHelper::IsFullScreenOrTabletMode()
{
try
{
auto instance = LifetimeHandler::GetMaterialHelperInstance();
auto instance = LifetimeHandler::GetMaterialHelperInstance();
return instance->IsFullScreenOrTabletModeImpl();
}

// ApplicationView::GetForCurrentView() is an expensive call - make sure to cache the ApplicationView
if (!instance->m_applicationView)
bool MaterialHelper::IsFullScreenOrTabletModeImpl()
{
if (!m_isFullScreenModeValid)
{
try
{
instance->m_applicationView = winrt::ViewManagement::ApplicationView::GetForCurrentView();
}
// ApplicationView::GetForCurrentView() is an expensive call - make sure to cache the ApplicationView
if (!m_applicationView)
{
m_applicationView = winrt::ViewManagement::ApplicationView::GetForCurrentView();
}

// UIViewSettings::GetForCurrentView() is an expensive call - make sure to cache the UIViewSettings
if (!m_uiViewSettings)
{
m_uiViewSettings = winrt::ViewManagement::UIViewSettings::GetForCurrentView();
}

const bool isFullScreenMode = m_applicationView.IsFullScreenMode();
const bool isTabletMode = m_uiViewSettings.UserInteractionMode() == winrt::ViewManagement::UserInteractionMode::Touch;

// UIViewSettings::GetForCurrentView() is an expensive call - make sure to cache the UIViewSettings
if (!instance->m_uiViewSettings)
m_isFullScreenMode = isFullScreenMode || isTabletMode;
}
catch (winrt::hresult_error)
{
instance->m_uiViewSettings = winrt::ViewManagement::UIViewSettings::GetForCurrentView();
// Calling GetForCurrentView on threads without a CoreWindow throws an error. This can happen in XamlIsland scenarios.
// In those cases assume that we are not in full screen or tablet mode for now.
// Task 19285526: In Islands, IsFullScreenOrTabletMode() can use ApplicationView or UIViewSettings.
m_isFullScreenMode = false;
}

const bool isFullScreenMode = instance->m_applicationView.IsFullScreenMode();
const bool isTabletMode = instance->m_uiViewSettings.UserInteractionMode() == winrt::ViewManagement::UserInteractionMode::Touch;

return isFullScreenMode || isTabletMode;
}
catch (winrt::hresult_error)
{
// Calling GetForCurrentView on threads without a CoreWindow throws an error. This can happen in XamlIsland scenarios.
// In those cases assume that we are not in full screen or tablet mode for now.
// Task 19285526: In Islands, IsFullScreenOrTabletMode() can use ApplicationView or UIViewSettings.
return false;
m_isFullScreenModeValid = true;
}

return m_isFullScreenMode;
}

/* static */
Expand Down
3 changes: 3 additions & 0 deletions dev/Lights/MaterialHelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ class MaterialHelper : public winrt::implements<MaterialHelper, MaterialHelperBa
static void ResetFailedToAttachLightsCount();

private:
bool IsFullScreenOrTabletModeImpl();
void EnsureCompositionCapabilities();
void EnsureSizeChangedHandler();

Expand Down Expand Up @@ -253,6 +254,8 @@ class MaterialHelper : public winrt::implements<MaterialHelper, MaterialHelperBa
bool m_wasWindowHidden{}; // True if we've received CoreWindow.VisiblityChanged w/ Visibility == false
bool m_waitingForRenderingAfterBecomingVisible{}; // True if we got a VisibilityChanged(True) and are waiting for a CT.Rendering to complete the RS2 workaround for Bug 11159685
bool m_energySaverStatusChangedRevokerValid{};
bool m_isFullScreenModeValid{};
bool m_isFullScreenMode{};

event<std::function<void(com_ptr<MaterialHelperBase>, bool)>> m_policyChangedListeners;
event<std::function<void(com_ptr<MaterialHelperBase>, bool)>> m_sizeChangedListeners;
Expand Down