fix(menu): stop reporting guessed defaults in Bug reports #103
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!103
Loading…
Reference in a new issue
No description provided.
Delete branch "mooki/Longshot:fix/bug-report-dynamic-defaults"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Why
Bug reports should not invent a default when a setting depends on the display, upscaler or Auto preset. The old report could say
Aspect ratio: Monitor's shape (default: 32:9)even when the player left Monitor's shape selected.This changes report text only, not the player's settings. HumanBean rebased the three original commits onto main
fbd420241aand added3078962ee62c268530a8e342c591768ea687c2d3. That is his commit, tested unchanged here. The four older game runs below had shared load, so they do not show a speed or cost difference.What changed
gpu/vk/vk_menu.cpp: keep current PC-dependent values without inventing a default.report_default()restores defaults the menu can name. Writing a report does not change any option.gpu/vk/vk_bug_report.h/.cpp: keep all settings in the form's Changes from the defaults field. A Depends on this PC line comes after known-default changes and before switches. There is no separate section to lose when filing the issue.3078962eadds the link fallbacks and known-default handling. The existing report test now checks the field, its ordering and the shorter links. The defaults test covers 16 authored cases, including a changed aspect ratio, Custom before and after Auto, Frame generation by, and saved game language. No existing check was removed.Current report layout
The README sentence this follows is:
This example was printed by the real formatter and report writer at
3078962e, using an authored 3440x1440 monitor, a 16:9 output, Custom before Auto, NVIDIA DLSS, Frame generation by set to Built-in FG, and a 110% field of view. It is a game-free example, not a new game run. The monitor default is named by the code, not guessed:Rows that still say default depends on this PC:
RX_VK_DLSS_PRESETappliesapply_autoon options the renderer is using.Rows with a known default now state it when changed:
language.txt, otherwise the first language. Leaving that menu row normally saves the choice, so this difference rarely remains in a report.Shorter links
The link keeps the same field. It first includes known-default changes, PC rows and switch names, never switch values. If that will not fit, it tries these three shorter forms in order. These are actual decoded
field:settingsvalues printed by the report writer at3078962e. The authored fixture adds switches and then rows to reach each length limit; those padding rows are not new menu options.Switch names become a count; the setting rows stay:
PC rows become a count; the known-default changes stay:
Counts only:
Flags and settings
Measurements
Branch state: on main
fbd420241afdba1d5ba2936971eb48549edb85ae; head3078962ee62c268530a8e342c591768ea687c2d3; whole game-free suite on this head:515 passed, 0 failed, 15 skipped(mainfbd420241afdba1d5ba2936971eb48549edb85ae:514 passed, 0 failed, 15 skipped). The rebase onto this main and the last commit of the branch are HumanBean's; the suite was run on his head unchanged, on Arch Linux. Main has gained 10 commits since; the branch still merges cleanly.Review checks at
3078962e: both report checks and 57 other source-reading checks exited 0 on Linux. The defaults check printedreport defaults: 16 cases, 0 failed. The recorder's Windows exit case and the Windows UTF-8 compile probe were skipped. This is targeted coverage, not the whole-suite result in the branch-state block.Earlier native runs
The four runs below keep their original commits and results. Their separate PC section is historical, not the current report layout.
Rows for docs/tested-configurations.md, kept here instead of in the branch (see #45)
Historical main:
d4a69456c9(2026-11-12)Historical PR: 93462ee71d16f1f77284cf120efbaf737ef8f49e
main:
d4a69456c9(2026-11-12)this PR: fix/bug-report-dynamic-defaults @ 93462ee71d16f1f77284cf120efbaf737ef8f49e (the head the four game runs used; the current head is in the branch-state line above)
Hardware: RTX 4090, NVIDIA 615.78.08; Core i9-13900K, 96 GB installed (the generated report says 94 GB); Arch/Omarchy Linux 7.2.15-1-omarchy
Scene: same copied child Hyrule Field save, 1920x1080, Custom, Game's own upscaler 50%, frame generation off, cap 60, precompile off, Aspect ratio Monitor's shape, Text speed Instant; windowless
Method: two interleaved pairs, main / branch / main / branch, 92.2–93.2 s each under shared load. Press the actual Bug report row once per run; inspect its generated text, file inventory, privacy and before/after menu values. Also run the real-source nine-context fixture twice each way in the same order. These are correctness observations, not timing benchmarks.
How to reproduce: on the historical main, leave Aspect ratio on Monitor's shape, open Gameplay → Bug report and save once to see the wrong default. With the new code, a row at the monitor default is omitted; a different shape names the monitor's shape as its default. Fixed-default differences such as Text speed remain in Changes from the defaults. Run
python -I -B tests/test_bug_report_defaults.py;--sourceselects another checkout for the same authored cases.d4a69456Historical main report excerpt:
That is the wrong default from the historical main runs. The earlier fix put PC rows in their own section. HumanBean's commit replaces that layout with the single field shown above.
The fixture's negative control uses
d725b363runtime source, identical to the measured main for this code, plus only the new authored test. Its repeated output is:Both fixed invocations passed; the first also passed the full report-writer fixture. The stable GPU/microphone and fixed-model cases already pass on main and remain covered.
d4a69456d4a69456All four exited0, with no fatal, device-loss, virtual-core escape or kernel fault and complete journal coverage. Each folder contains exactly report.txt, longshot_log.txt and picture.png. Both text files omit the user name, home path and host name. The microphone list's upper bound changed18→2 during enumeration in every run; its selected value and label did not change, so the61-value invariant deliberately does not claim identical dynamic device-list bounds.
The older builds use matching release profiles. Their results keep the measured commits named above. They do not exercise HumanBean's new report layout or known-default changes. The source checks at
3078962ecover those changes, not a new game run.Testing
Tested: at HumanBean's
3078962e, unchanged:python -I -B tests/test_bug_report.pypassed.python -I -B tests/test_bug_report_defaults.pyprintedreport defaults: 16 cases, 0 failed. Another 57 checks that read either changed C++ file exited 0 on Linux. The recorder's Windows exit case and the Windows UTF-8 compile probe were skipped, not passed. The whole-suite result and any failing test names are recorded once in the branch-state block under Measurements. These targeted checks are not a whole-suite result.Earlier game evidence: the four native Linux runs and the two real-source fixture pairs in Measurements keep their original commit names. They show the original wrong default and unchanged option values. They do not exercise the new report layout.
Built for Windows and Linux at the current head, not run: At
3078962ee62c268530a8e342c591768ea687c2d3, the Windows x86-64 Vulkan renderer DLL and all 44 selected host translation units were cross-built on Linux with llvm-mingw 20261104 (Clang 23.1.5), using the project's production flags and pinned dependencies (renderer: 35 newly compiled / 170 reused; host: 1 newly compiled / 43 reused; lifted C: 0 newly compiled / 80 reused; all Windows units: 36 newly compiled / 607 reused from matching object-cache entries), the renderer DLL linked and the full game executable linked; nothing was run on Windows or under Wine, and this is neither the project's hosted builder run nor a Windows UTF-8 run-time path test. The same head also compiled and linked its full native Linux executable and renderer with Clang 23.1.3 through the documented resume path (201 newly compiled units, 284 matching Linux-cache units, source stampc52a6bc9), using the verified target-neutral C from the freshfbd420241alift rather than carrying the patch onto another commit; neither Linux binary was run. An earlier Windows link at9291b06f, before the last commit, gave the same result.Not tested: at
3078962e: a full game build, the revised report in a running game, Windows gameplay or C++ fixture execution, other GPUs or real monitor shapes, physical microphone devices, real-window rendering, and a signed-in form submission. Authored tests cover the shape and engine cases, not those physical configurations. No rendering, default-setting or performance improvement is claimed.Needs other testers:
Screenshots
These pictures are from the older native runs at the commits in their captions. The report illustration shows the old separate-section layout, replaced by the single field above.
Risk and rollback
The report-only classification must stay aligned with rows that Auto or the display/engine controls. The test derives Auto-written members from the real setters and checks that they are included. A wrong classification would hide or misdescribe a diagnostic difference, not change the player's setting. Reverting this fix restores the old report format; no save or renderer interface migration is needed.
Reviewer notes
Start with
report_default_depends_on_pc,report_defaultandchanged_rows_textingpu/vk/vk_menu.cpp, then the settings-field fallbacks ingpu/vk/vk_bug_report.cpp. HumanBean's3078962esupplies the review changes. Auto's choices are not recomputed while writing a report: callingapply_autowould alter the live options the renderer reads. The richer collector is a separate draft and is not part of this review.Issues
See #44
Checklist
For everyone
type(scope): description).main.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.mainand passing here.tests/that reads changed source updated in this PR, not weakened.3078962e: the Linux executable and renderer built and linked; the Windows renderer and game executable cross-built and linked on Linux (see Testing); neither was run.New on this forge; not opened on GitHub.
Assisted-by: GPT-6 Astra
Co-Authored-By: GPT-6 Astra [email protected]
b3851373c09291b06f6cCarried onto main
0c74253d4(head9291b06f6) without its rows for the results table, which are kept in the description instead (see #45); nothing else changed. The whole game-free suite on this head: 506 passed, 2 failed, 15 skipped. Test request T-VNZ6 is open (Windows, or a monitor that is not 16:9). One box is still open, the Windows half of the two-platform build: a cross-build of this head for Windows is being prepared, so this stays WIP until that is settled.The last open box is settled: at
9291b06fthe renderer DLL and the full game executable were cross-built and linked for Windows on Linux with the project's llvm-mingw and its own flags (36 files newly compiled, 607 reused from matching objects); nothing was run on Windows, which test request T-VNZ6 asks for. Carried onto main0edc13771(headc139f9b4e) with the change unchanged; the whole game-free suite on this head: 510 passed, 2 failed, 15 skipped. Ready for review.9291b06f6cc139f9b4e5WIP: fix(menu): stop reporting guessed defaults in Bug reportsto fix(menu): stop reporting guessed defaults in Bug reportsThanks: the bug is real, and the report-writer tests are stronger than before. On main,
tx_upscalerignores its argument andtx_resolution/tx_filter/tx_framegen_byformat the live state, so filed issues carry lines likeUpscaler: AMD FSR 4 (default: AMD FSR 4)andRender resolution: Native (FSR 4, 2560x1440) (default: Native (FSR 4, 2560x1440))(#226), and the Aspect ratio'sOptions{}index means nothing onceoutputs_synchas run. Other good parts:test_bug_report.pynow checks the new call, and nothing in it was weakened.apply_autoitself, so when the Steam Deck profile (#225) lands, itsopts.framegen = 2will fail the check until Frame generation is classified too.Must change
field:settingsand its two fallbacks (vk_bug_report.cpp:525) are built fromin.optionsonly. The issue form has no field called "Depends on this PC", yet report.txt tells players to copy each answer "into the fields of the same names". So for a report filed through the link, Upscaler, Render resolution, Output resolution, Aspect ratio, Quality preset, Window scaling filter, Sharpness, Frame pacing, the VRAM budget, Frame generation by and Game language all drop out of the issue. In #226 (an upscaler bug) and #112, those were most of the lines. The fix hides settings the player did change. Put the PC-dependent lines in the settings field too, for example under aDepends on this PC:line inside "Changes from the defaults", and in the link and its short forms (a count at least).0edc1377. Main has since merged eight commits, among them #55, which changestx_resolutionin vk_menu.cpp (v = ur::snap_render(up, v)). The new fixture copiestx_resolutionout of vk_menu.cpp. A trial merge has no conflicts, but neither check has run on the merged code. Rebase onto main, run the suite again and update the description.Should change
Several reclassified rows have a known default, and listing them as "default depends on this PC" erases real changes. The rule ignores the preset and whether the value changed, so all 13 rows are always listed with no default:
aspect_monitor_value()). Your own example printsMonitor's shape (default depends on this PC), which is at its default.Lines like
Frame pacing: On (default: Off)andFrame generation by: Built-in FG (default: Auto)(from #112) were right. Now nobody can tell whether the player changed them. Where the default is known, report it. For the rows Auto writes, the real default on this PC is whatauto_choicepicks, which the menu already has (it is the source of the log's "Auto (this PC) would be" line). Compute it, for example withapply_autoon a saved and restoredopts, or at least say "(set by Auto (this PC))" or "(changed)". "depends on this PC" alone doesn't say which.Description.
test_updater_asset_digest.pyas the "sole failure", but Measurements names two others (test_display_pointer,test_texture_staging_publication).Not checked: no build and no tests were run for this review (code read only), including whether the fixture compiles against main's
tx_resolutionafter #55. #170 is already in the base, and LongshotLauncher PR 42 writes no settings section, so neither overlaps.@ -521,4 +522,4 @@const std::string named = sw.empty() ? "Switches set: none.\n" : "Switches set (their values are in the attached report.txt): " + names + ".\n";const std::string listed = sw.empty() ? "" : "Switches set: " + count(sw) + ", listed in the attached report.txt.\n";bool link_short = false;for (const std::string& s : {options + named, options + listed,The link fills
field:settingsfromin.optionsonly, and its count fallback leavespc_optionsout too. The form has no field called "Depends on this PC", so a report filed through this link loses Upscaler, Render resolution, Output resolution, Aspect ratio, Sharpness, Frame pacing, the VRAM budget and the other rows the player changed. Please put these lines in the settings field as well (under aDepends on this PC:line), at least as a count in the short forms.@ -542,6 +544,7 @@ std::string write(const Inputs& in) {text += "### Graphics card and driver version\n\n" + gpu + "\n\n";text += "### CPU, RAM and operating system\n\n" + in.system + "\n\n";text += "### Changes from the defaults\n\n" + settings + "\n";if (!in.pc_options.empty()) text += "### Depends on this PC\n\n" + in.pc_options + "\n";This heading isn't one of the issue form's fields, yet the report's first lines say to copy each answer "into the fields of the same names". It would be safer as a sub-list inside "Changes from the defaults", so it reaches the issue whichever way the player files it.
@ -2910,0 +2915,4 @@return r.value == &opts.ultrawide || r.value == &opts.output_res || r.value == &opts.resolution ||r.value == &opts.dlss || r.value == &opts.preset || r.value == &opts.upscale ||r.value == &opts.sharpness || r.value == &opts.pace || r.value == &opts.realtime ||r.value == &opts.handheld || r.value == &opts.vram_budget || r.value == &opts.framegen_by ||framegen_by's default is Auto, a fixed value: onlytx_framegen_by(0)formats the live state. Main'sFrame generation by: Built-in FG (default: Auto)(issue 112) was right. Format the default as plain "Auto" and keep the row in the changes.pace,sharpness,upscale,realtimeandhandheldare also listed unconditionally, so a Custom player's realFrame pacing: On (default: Off)loses its default too.@ -2910,0 +2916,4 @@r.value == &opts.dlss || r.value == &opts.preset || r.value == &opts.upscale ||r.value == &opts.sharpness || r.value == &opts.pace || r.value == &opts.realtime ||r.value == &opts.handheld || r.value == &opts.vram_budget || r.value == &opts.framegen_by ||r.value == &opts.game_lang || (r.value == &opts.dlss_model && dlss_preset_env());Why is Game language here? Its value is language.txt's, else index 0 (
game_lang_index), which doesn't depend on the display, the upscaler or Auto.@ -2922,2 +2937,2 @@out += std::string(r.name) + ": " + now + " (default: " + r.text(was) +(restart_changed(r) ? "; takes effect after a restart" : "") + ")\n";to += r.value == &opts.mic && *r.value >= 2 ? "a microphone by its name" : r.text(*r.value);to += depends ? " (default depends on this PC" : " (default: ";The default is often known: the monitor's shape for Aspect ratio (
aspect_monitor_value()), and whatauto_choicepicks for the rows Auto writes. Saying "default depends on this PC" for every row, changed or not, means a reader can't tell what the player changed (your own example printsAspect ratio: Monitor's shape (default depends on this PC), which is at its default). Report the real default where it is known, or at least say "set by Auto (this PC)" or "changed".c139f9b4e53078962ee6I rebased your three commits onto main @
fbd42024(no conflicts), which includes #55. Your test's cuts still match main'stx_resolutionand the other functions. I addressed my review in one commit on top, pushed as3078962e.Nothing here was compiled or run. The C++ in
vk_menu.cpp,vk_bug_report.cppand the test fixture has only been read. Please run the suite at this head before relying on it.My commit:
The PC rows now reach the issue. The "Depends on this PC" rows now sit inside "Changes from the defaults" (the form's
settingsfield), under their own line, after the fixed-default changes and before the switches. That's true both in report.txt and in the link. When the link is too long it falls back in order:Depends on this PC: N, listed in the attached report.txt., then the switch count;Known defaults are stated. A new
report_default()gives:aspect_of_choice(defaults.output_res, defaults.ultrawide);i18n::game_lang_index();Sharpness, Frame pacing, Game speed, Profile and VRAM budget use the built-in defaults for a Custom player that Auto never chose for (
auto_for == 0). These stay "default depends on this PC":RX_VK_DLSS_PRESET;Auto's picks aren't computed as defaults: that would mean running
apply_autoon the live options the renderer reads.Tests, only added to:
test_bug_report.py: the line and its order, the link'ssettingsfield carrying the rows, the count sentence, and a new long case where they don't fit.test_bug_report_defaults.py: cases for a changed aspect ratio, Custom before and after Auto, Frame generation by, and the game language.Please update the description:
Since leaving the Game language row writes language.txt, that row will rarely show up in a report now.
Thank you for the rebase and for
3078962e. I tested your commit unchanged. No follow-up code commit was needed.On Linux at
3078962e:python -I -B tests/test_bug_report.py:bug report: ok (report.txt as the issue form, both logs redacted, the picture, the long log, the terminal start, switch values, NUL bytes, the folder's reason).python -I -B tests/test_bug_report_defaults.py:report defaults: 16 cases, 0 failed.3078962e: 515 passed, 0 failed, 15 skipped (mainfbd42024: 514 passed, 0 failed, 15 skipped).report.txtand the link.vk_bug_report.cppretains the rows when switch names become a count, then counts PC rows, then uses counts only. The description includes output printed by the real formatter and report writer, including those shorter forms.fbd420241ais kept, including #55. Both report tests compiled and ran against that code. The 16 defaults cases include the updated render-resolution formatter. The whole-suite result is above.report_default()now states the monitor's shape for Aspect ratio, the saved language for Game language, and plain Auto for Frame generation by. Sharpness, Frame pacing, Game speed, Profile and VRAM budget keep built-in defaults on Custom before Auto has chosen. After that, or with a preset, they remain PC-dependent. Output resolution, Upscaler, Render resolution, Window scaling filter and Quality preset still depend on context; DLSS model does too under its override. The description gives these reasons. I kept your choice not to callapply_autoon the live options.Windows execution and a game run of the revised layout were not done in this check.
One box is unticked again on purpose: the Windows link and the Linux build in the description are from
9291b06f, and your commit changes two renderer files since. I am repeating the Windows link at3078962eand tick the box when it has linked; the result goes in a comment here.A correction to my description update of a few minutes ago: it had dropped the two lines that name the measured commits and renamed two column heads and two Testing labels, so the description check failed on them. They are restored; nothing else changed. The check now reports one thing only, the Windows-and-Linux build box, which stays unticked until your commit has been linked for Windows, as said above.
Your commit builds on both platforms. At
3078962ethe Windows renderer and the full game executable cross-built and linked on Linux with llvm-mingw 20261104 (Clang 23.1.5), and the native Linux executable and renderer built and linked with Clang 23.1.3; neither compiler rejected anything. Nothing was run from either build, and nothing ran on Windows. The Windows-and-Linux build box is ticked again with that scope, so the description check passes; the exact counts are under Testing.Thanks for testing
3078962eand for the rewritten description. All four points from my last review are addressed:field:settings, and the three fallbacks keep the changes longest.test_bug_report.pycovers each form.fbd42024and the suite was run at that head.I read the code against the surrounding menu code:
report_defaultagrees withoutputs_syncfor the Aspect ratio, because both callaspect_of_choice(…, 1)on choice 0.auto_foranddlss_preset_envare declared before they are used.auto-profilecase collects every member thatapply_auto/apply_presetwrite, so a new Auto-controlled row can't slip into the fixed list unnoticed.Must change
93462ee7.3078962erewrotechanged_rows_text,report_defaultand the settings field, and the new layout has only been exercised through the authored fixture. AGENTS.md asks for a game run before a PR is marked ready, and the PR otherwise stays WIP until someone has run it. On your RTX 4090 setup, one Bug report press at the current head is enough: paste its "Changes from the defaults" answer and the decodedfield:settings. T-VNZ6 (Windows, other monitor shapes) can stay open alongside it.mainwhile you're at it. It is 10 commits pastfbd42024, so "Rebased on current main" no longer holds. I checked those commits: none touches the report path. The only overlap isdamage_taken = 1inOptions(#138), which the fixture's regexes handle. The game run in point 1 should then be at the rebased head.Suggestion (not blocking)
main, a German player's report saidGame language: Deutsch (default: English (US)). The game log'soe: languageline still has it, but only in the attachment.Not checked: code read only. No build or test was run for this review. CI's Python checks run on
workflow_dispatchonly, so they were skipped on this PR's run 2009, as on every PR. The suite result is the author's own.@ -2906,0 +2930,4 @@static const Options defaults{};*named = nullptr;if (r.value == &opts.ultrawide) return aspect_of_choice(defaults.output_res, defaults.ultrawide);if (r.value == &opts.game_lang) return i18n::game_lang_index();language.txt starts as
en-US(host/nn_applet.c writes it on the first start), andgame_lang_tickrewrites it whenever the player leaves this row. Taking it as the default therefore means the row is always "at its default", so the game language no longer reaches the issue at all; onmain,Game language: Deutsch (default: English (US))did. This is consistent with the README table ("language.txt's, else English (US)"), so it isn't blocking. Still, the language is useful context for text and font bugs. Consider keeping the fixed default (index 0, English (US)), or always listing the row with its value (for example among the PC rows). Thelang-savedcase would then change with it.