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

Fix FrameStatisticsViaTouch to work as described in the wiki #6040

Merged
merged 4 commits into from Nov 10, 2023

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Nov 5, 2023

https://github.com/ppy/osu-framework/wiki/Environment-variables#osu_frame_statistics_via_touch

I think it makes more sense to have it as described in the wiki. A hybrid solution would be to default to enabled on debug, and disabled on release (the envvar would always override the default in the case).

Currently there's no way to disable touch frame statistics in debug builds (one can only enable them in release builds).

@bdach
Copy link
Collaborator

bdach commented Nov 5, 2023

Why would we want only one of four boolean envvars to work like this?

@Susko3
Copy link
Member Author

Susko3 commented Nov 5, 2023

If you're commenting on the != "0" vs == "1", it's just flipping the default.
Do you think all boolean envvars should default to false?

Would having a helper function make the default more clear?

bool ParseBooleanEnvironmentVariable(string name, bool defaultValue)

@bdach
Copy link
Collaborator

bdach commented Nov 5, 2023

I guess I partially misunderstood what this was going for, but this implementation doesn't seem to match the wiki either, since this 100% should not be default-enabled in release builds, which it would be right now, because:

if (FrameworkEnvironment.FrameStatisticsViaTouch || DebugUtils.IsDebugBuild)
{
base.AddInternal(new FrameStatisticsTouchReceptor(this)
{
Depth = float.MaxValue,
Anchor = Anchor.BottomRight,
Origin = Anchor.BottomRight,
RelativeSizeAxes = Axes.Both,
Size = new Vector2(0.5f),
});
}

while the wiki says:

Note that this only works in debug builds. Disable for debug builds by setting to 0.

If you're angling android for this, I'd rather just finally fix android debug builds not properly being marked as debug builds.

@Susko3
Copy link
Member Author

Susko3 commented Nov 5, 2023

It seems you're looking at some other branch, I've changed the conditional to && so it works as written on the wiki.
https://github.com/ppy/osu-framework/pull/6040/files#diff-c311d03d5213e6632087e2ddc2b3a2c8f2f2db806d33330312a387d719780445L269-R269

If you're angling android for this, I'd rather just finally fix android debug builds not properly being marked as debug builds.

Completely forgot that IsDebugBuild still doesn't work on android 🙃
But I was actually annoyed by this on windows. I don't need the double tap for statistics as I can just press Ctrl+F11.

@@ -23,7 +23,7 @@ static FrameworkEnvironment()
StartupExecutionMode = Enum.TryParse<ExecutionMode>(Environment.GetEnvironmentVariable("OSU_EXECUTION_MODE"), true, out var mode) ? mode : null;
NoTestTimeout = Environment.GetEnvironmentVariable("OSU_TESTS_NO_TIMEOUT") == "1";
ForceTestGC = Environment.GetEnvironmentVariable("OSU_TESTS_FORCED_GC") == "1";
FrameStatisticsViaTouch = Environment.GetEnvironmentVariable("OSU_FRAME_STATISTICS_VIA_TOUCH") == "1";
FrameStatisticsViaTouch = Environment.GetEnvironmentVariable("OSU_FRAME_STATISTICS_VIA_TOUCH") != "0";
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I get that this is flipping the default, but it reads really awkwardly. I'd prefer leaving the comparison as =="1" but adding a pre-check for empty string / null. Or something else that isn't this.

@pull-request-size pull-request-size bot added size/S and removed size/XS labels Nov 6, 2023
peppy
peppy previously approved these changes Nov 7, 2023
Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Probably okay

@peppy peppy requested a review from bdach November 7, 2023 01:45
@bdach bdach enabled auto-merge November 10, 2023 05:04
@peppy peppy disabled auto-merge November 10, 2023 07:25
@peppy peppy merged commit 5c5c6c5 into ppy:master Nov 10, 2023
15 of 21 checks passed
@Susko3 Susko3 deleted the fix-touch-framestatistics branch November 10, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants