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

VertexShaderManager: Only look for freelook config changes if we're using freelook #8884

Merged
merged 1 commit into from Jul 4, 2020

Conversation

iwubcode
Copy link
Contributor

@iwubcode iwubcode commented Jun 17, 2020

Another attempt to fix 12157. I made the assumption that pulling from our onion config system was relatively fast but that doesn't seem to be the case from testing that fifolog (4fps difference)

@iwubcode
Copy link
Contributor Author

I'd love to hear from other devs on how they handle config changes that can occur at any time?

@delroth
Copy link
Member

delroth commented Jun 17, 2020

Snapshot at the beginning of the frame? It doesn't make sense to change this setting mid-rendering anyway.

@delroth
Copy link
Member

delroth commented Jun 17, 2020

Also: Config::AddConfigChangedCallback means you don't even have to snapshot, you can update the local version when it gets set.

@JosJuice
Copy link
Member

JosJuice commented Jun 17, 2020

I'd love to hear from other devs on how they handle config changes that can occur at any time?

This function is called on config change and writes the current config values to g_Config, which is then copied to g_ActiveConfig when a new frame starts:

void VideoConfig::Refresh()

@iwubcode
Copy link
Contributor Author

Thank you both. This is useful.

I'm going to be moving these settings out of the graphics/video config in #8867 so I was curious how that was working. Despite having looked at VideoConfig.cpp so many times, I didn't even notice that AddConfigChangedCallback!

For now though, I will add this setting to VideoConfig.cpp so I don't need to query it every time when freelook is enabled.

@iwubcode iwubcode marked this pull request as draft June 17, 2020 20:33
@iwubcode iwubcode marked this pull request as ready for review June 17, 2020 20:58
@iwubcode
Copy link
Contributor Author

@delroth / @JosJuice - updated, let me know what you think

@lioncash lioncash changed the title VertexShaderManager: Only look for freelook config changes if we're u… VertexShaderManager: Only look for freelook config changes if we're using freelook Jun 17, 2020
Copy link
Contributor

@stenzek stenzek left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. I think the CheckForConfigChanges() method makes more sense though.

Source/Core/VideoCommon/VertexShaderManager.cpp Outdated Show resolved Hide resolved
…ion each call which caused a performance hit, move check to RenderBase where it is checked when config changes
@iwubcode
Copy link
Contributor Author

iwubcode commented Jul 2, 2020

Thanks @stenzek , updated per your suggestion.

@@ -406,6 +407,11 @@ void Renderer::CheckForConfigChanges()

UpdateActiveConfig();

if (g_ActiveConfig.bFreeLook)
{
g_freelook_camera.SetControlType(g_ActiveConfig.iFreelookControlType);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this function have to run every frame? Could you save the old freelook control type/freelook enable and only run it when that changes? e.g.

const bool old_freelook = g_ActiveConfig.bFreeLook;
const int old_freelook_control_type = g_ActiveConfig.iFreelookControlType;

if (old_freelook != g_ActiveConfig.bFreeLook || old_freelook_control_type != g_ActiveConfig.iFreelookControlType)
{
  g_freelook_camera.SetControlType(g_ActiveConfig.iFreelookControlType);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't have to but what's the point of all this checking? It doesn't really matter that function is called so often because the actual function already bails if there are no changes. The whole point is to just get the new value from configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't see the check here: https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/VideoCommon/FreeLookCamera.cpp#L179

I misremembered it unconditionally reallocating it. In that case, it's fine as-is.

@stenzek stenzek merged commit 9c12a84 into dolphin-emu:master Jul 4, 2020
10 checks passed
@iwubcode iwubcode deleted the freelook_performance_fix branch August 8, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants