p2sr / SourceAutoRecord

Speedrun plugin for Source engine games.
MIT License
89 stars 27 forks source link

`toggle` doesn't work for SAR cvars #188

Closed p2srluma[bot] closed 2 months ago

p2srluma[bot] commented 1 year ago

https://discord.com/channels/146404426746167296/811780246608281650/1130889362247254118


This action was performed automatically, triggered by a report by discord user amj#0 at 18-07-2023 15:50:34 UTC.

ThisAMJ commented 1 year ago

Likely related to #159

ThisAMJ commented 8 months ago

Investigation reveals toggle doesn't work for SAR cvars which have the FCVAR_NEVER_AS_STRING flag, because ConVar::ChangeStringValue doesn't get ran/asserts that the flag isn't set.

My question is why we use this flag at all, since it seems very scarcely used in source (6 convars in total). This behavior was introduced in the initial commit! 41958be199945a796d44c417b89ed7f93e1d2429 ConVar.h

@NeKzor

NeKzor commented 4 months ago

The FCVAR_NEVER_AS_STRING flag is useful to distinct numeric and string ConVars. I don't see why we want to remove this flag now because it works as intended. The 6 other non-SAR commands indicate that this is a problem with toggle and not with SAR.

Also I noticed that sar_hud_* commands are not affected by this because some inconsistency was introduced in 45fa47b14f048658d7766b304fb2465a001c3de4.

Likely related to #159

Interesting that this gets mentioned. Here we can format numeric SAR ConVar values into a string the way we want (%g) instead of relying on the engine (%f):

https://github.com/p2sr/SourceAutoRecord/blob/ab87d1bca4bb25feac6e8a0d5309157224cb640e/src/Features/ConfigPlus.cpp#L69-L69

ThisAMJ commented 4 months ago

Why do we make that distinction though, when the game doesn't e.g. volume. FCVAR_NEVER_AS_STRING implies we NEVER EVER want to print its value as the SDK says, which SAR explicitly disobeys in the line you linked.

// never try to print that cvar

if ( m_nFlags & FCVAR_NEVER_AS_STRING ) return "FCVAR_NEVER_AS_STRING";

The base game cvars with it set are very obscure debug things -- we could patch toggle I guess but removing the flag is way easier and less invasive. Can get a proper list of the base cvars in ~8hr

ThisAMJ commented 4 months ago
cl_overdraw_test
mat_texture_list_all
mat_texture_list_view
mat_show_texture_memory_usage
mat_texture_limit
ThisAMJ commented 4 months ago

To me it's not a matter of it "working as intended", because our stuff is not correct.