Skip to content

feat: initialize from Project Settings with no scene object — BugSplat.Instance, auto-initialize, build validation - #254

Open
bobbyg603 wants to merge 6 commits into
mainfrom
feat/auto-initialize
Open

bobbyg603 wants to merge 6 commits into
mainfrom
feat/auto-initialize

Conversation

@bobbyg603

@bobbyg603 bobbyg603 commented Sep 4, 2026 •

Copy link
Copy Markdown
Member

Closes #253. Closes #174.

What this changes

BugSplat now initializes itself. Select or create a BugSplatOptions asset in Edit > Project Settings > BugSplat and BugSplat starts from it before the first scene loads, in the editor and in every player. Nothing needs to be placed in a scene, and user code reaches the client through BugSplat.Instance.

Setup goes from four steps across two Unity UIs — create an asset from the Asset Create menu, add a BugSplatManager to a GameObject, drag the asset in, make sure that GameObject is in the first scene — each of which failed silently, to one page. It also closes a coverage gap: nothing ran until a manager's Awake, so a native crash during the first scene's load, or in a bootstrap scene without a manager, was never reported. BeforeSceneLoad is earlier than any Awake.

Runtime

  • BugSplat.Instance, BugSplat.IsInitialized, BugSplat.Initialize(BugSplatOptions) (Runtime/BugSplat.Initialization.cs). Initialize is idempotent — a second call warns and returns the existing instance; reporting a crash must not itself become one.
  • [RuntimeInitializeOnLoadMethod(BeforeSceneLoad)] initializes from the configured asset; SubsystemRegistration resets the statics so Enter Play Mode Options without domain reload re-initializes cleanly.
  • BugSplatRuntime (internal, hidden from Add Component) is the host: the body of the old manager — log hooks, background queue, bounded drain — on a DontDestroyOnLoad GameObject that Initialize creates. Coroutines still need a MonoBehaviour; nothing else about it is user-facing.
  • BugSplatOptions gains InitializeAutomatically (default on), plus RegisterLogMessageReceived, CaptureExceptionsOnBackgroundThreads, CaptureUnobservedTaskExceptions, moved from the manager so automatic initialization can honor them. It registers itself in OnEnable when loaded as a preloaded asset, which is how a player finds it.
  • BugSplatManager is [Obsolete] and hidden from Add Component. It adopts the live instance (logging that it is no longer needed) or, with auto-initialize off, initializes from its own asset exactly as before, its own capture flags winning. Two managers, or a manager plus auto-initialize, no longer install two sets of hooks — that is F4: Nothing prevents two managers double-reporting; multi-asset builds pick an arbitrary options asset #174's double-reporting half, closed by construction.
  • BugSplatRef removed.

Editor

  • BugSplatSettingsProvider: Edit > Project Settings > BugSplat (also BugSplat > Settings...). Asset picker with Create, the asset's inspector drawn inline, a status line (unconfigured / empty database / auto-init off / configured), and an advisory when the open scene still has a manager.
  • BugSplatProjectOptions: the selection, stored as an EditorBuildSettings config object. A project with exactly one asset and no selection gets it selected automatically — every 4.x project on upgrade day, no clicks. Several assets and none selected stays null rather than picking one silently, which is F4: Nothing prevents two managers double-reporting; multi-asset builds pick an arbitrary options asset #174's other half. PostBuild and the symbol-upload menu read this instead of FindAssets(...)[0].
  • BugSplatOptionsPreloader: IPreprocessBuildWithReport fails the build when nothing is selected, when several assets exist and none is selected, or when the selected asset has an empty database with auto-initialize on. Otherwise it adds the asset to the preloaded assets for the build and removes it afterward.

Sample and docs

The sample scene has no BugSplatManager anymore; its scripts use BugSplat.Instance, and their "not initialized" messages now say the thing that matters — nothing will be reported — with the settings page named. README configuration, api.md, usage.md, ios.md, android.md, the migration guide, and the changelog are updated.

Turning BugSplat off

A project should not have to fill in a database before it can build, and a team that wants BugSplat only in QA and release needs a switch. Two orthogonal questions, each answerable on the asset or with a define:

On the asset As a define Effect
Does BugSplat run at all? Enabled (default on) BUGSPLAT_DISABLED No initialization, no build validation. An explicit BugSplat.Initialize is still honored.
Who starts it? InitializeAutomatically (default on) BUGSPLAT_MANUAL_INITIALIZE BugSplat does not self-start; the database is still validated, because you still intend to report.

Misconfiguration now fails a release build and only warns on a development build (BuildOptions.Development), so iterating never requires configuring BugSplat first while a release player that silently reports nothing still cannot ship.

Making the axes non-redundant also fixed a wrong behaviour: an empty database with InitializeAutomatically off used to pass validation, even though the project clearly intended to report through that asset. It now fails.

Setup without the Editor UI

Development is increasingly done by scripts and agents that never open a menu, so the page is deliberately a view over files and a public API rather than the only path in:

  • One asset file is enough. A project's single BugSplatOptions asset is selected automatically, so writing Assets/BugSplat/BugSplatOptions.asset (a ~15-line YAML with the package's stable m_Script GUID) configures the project. Documentation~/automation.md gives the template, the .meta, and the m_configObjects entry for the several-assets case.
  • One command. -executeMethod BugSplatUnity.Editor.BugSplatSetup.ConfigureFromCommandLine -bugsplatDatabase <name> [-bugsplatApplication] [-bugsplatVersion] [-bugsplatAssetPath] creates or updates and selects the asset, exiting 0/1. BugSplatSetup.Configure / CreateAsset and BugSplatProjectOptions.Get / Set / FindAll are public; the settings page's Create button calls the same code.
  • Every "not configured" message names the file fix next to the menu, via one shared BugSplatOptions.ConfigureHint, so a build log or a startup warning is enough for a script to act on.
  • BUGSPLAT_MANUAL_INITIALIZE for code-only projects: no auto-init, no startup warning, no build check. The preprocessor reads the define for the target being built (PlayerSettings.GetScriptingDefineSymbols for the report's platform group), falling back to the editor's compile-time value.

Verification

  • PlayMode suite against this branch in a CI-style host project: 148 / 148 (baseline 136, minus 3 BugSplatRef tests, plus 15 new). BugSplatManagerTest — the 4.x wiring — passes unchanged apart from a #pragma for the obsolete warning.
  • New BugSplatInitializationTests: Initialize sets Instance once and warns on a second call; the host reports a main-thread exception exactly once; Shutdown stops reporting; the manager adopts an existing instance without starting a second host (counting hosts rather than one reporter's calls, because under the old behaviour the duplicate report went through a second client), initializes from its own asset when nothing else has, and tears down only an instance it created. Under UNITY_EDITOR, AutoInitialize is driven through a real EditorBuildSettings config object: initializes from it, stays quiet when auto-initialize is off, warns when nothing is selected.
  • Non-vacuity: with Initialize's idempotency guard and the manager's adopt branch both removed — the 4.x behaviour — the suite goes 143 / 148, failing exactly the four tests that pin them (Initialize_Twice_WarnsAndReturnsTheExistingInstance, Manager_WhenAlreadyInitialized_AdoptsTheInstanceAndWarns, Manager_WhenAlreadyInitialized_DoesNotStartASecondHost, Manager_Destroyed_LeavesAnAdoptedInstanceAlone) plus BackgroundThreadFlood_DrainsOneQueuePerFrameAndWarnsOnceAboutDrops as collateral from the leaked second host. Files restored byte-identical (cmp).
  • Editor smoke test in batch mode against the real EditorBuildSettings / PlayerSettings: no assets → null; two assets, none selected → null; one asset → auto-selected and persisted; empty database → BuildFailedException naming the database; preprocess adds the asset to preloaded assets and postprocess removes it; auto-initialize off with an empty database builds; nothing configured → BuildFailedException naming the settings page.
  • Setup smoke test, batch mode: Configure with no database and no asset throws before creating anything; creates at the default path and selects it; a second call updates the same asset in place and saves it; HasManualInitializeDefine handles a list, whitespace, a longer symbol, and null — 12/12.
  • The real command line: -executeMethod BugSplatUnity.Editor.BugSplatSetup.ConfigureFromCommandLine -bugsplatDatabase cli-db -bugsplatApplication CliApp exits 0, writes Assets/BugSplat/BugSplatOptions.asset with those values, and writes m_configObjects: com.bugsplat.unity.options: {fileID: 11400000, guid: …, type: 2} into ProjectSettings/EditorBuildSettings.asset — the exact shape automation.md documents. With no database and nothing to update it exits 1 with the reason logged.
  • Suite after the automation and on/off additions: 151 / 151.
  • Upgrade smoke: an options asset written before Enabled and the capture flags existed loads with all of them on (Unity keeps field initializers for keys absent from the file), so upgrading cannot silently disable reporting or log capture. This one is worth the check — had it gone the other way, every existing project would have upgraded into silence with no error anywhere.
  • The sample's four scripts compile against the rebuilt runtime under UNITY_IOS, UNITY_STANDALONE_OSX, and UNITY_STANDALONE_WIN.
  • PostBuild.cs keeps its CRLF line endings; its diff is 15 lines.

What still needs a device

Initialization moved from scene-0 Awake to BeforeSceneLoad, so the init-timing rows in #200 need one re-run on release-candidate builds: native crash → report next launch, attachments seeded before start, Player.log, hang thresholds, and the Android bridge being started before the first scene. The reporter logic under them is untouched. The build-pipeline rows (dSYM upload, credentials, pbxproj, bridge compiles, symbol upload) stand.

One thing worth eyes on that no test can see: the four README screenshots of the old flow were removed rather than replaced. A screenshot of the settings page would be worth adding before the launch post.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NKSbnZWCkxX1tN4wbgyJ9R

…t.Instance, auto-initialize before the first scene, edit-time validation

Closes #253. Closes #174.

BugSplat now initializes itself from the BugSplatOptions asset selected in
Edit > Project Settings > BugSplat, at RuntimeInitializeLoadType.BeforeSceneLoad,
and exposes the client as BugSplat.Instance. Nothing needs to be placed in a
scene, and a native crash during the first scene's load is no longer lost.

- BugSplat.Instance / IsInitialized / Initialize(options); Initialize is idempotent.
- BugSplatRuntime hosts the log hooks and main-thread posting (the old manager body).
- BugSplatOptions.InitializeAutomatically plus the three capture flags moved from
  the manager; the asset registers itself when loaded as a preloaded asset.
- BugSplatManager is [Obsolete]: adopts the live instance or initializes from its
  own asset as before. Two managers no longer double-report.
- Project Settings page, EditorBuildSettings config object for the selection
  (auto-selected when exactly one asset exists), build preprocessor that fails an
  unconfigured build and preloads the asset. PostBuild reads the selection
  instead of FindAssets(...)[0].
- Sample scene drops its manager; scripts use BugSplat.Instance.
- BugSplatRef removed. Docs, migration guide, changelog updated.

Tests: 148/148 (baseline 136 - 3 BugSplatRef + 15 new). Mutation run with the
idempotency guard and adopt branch removed fails exactly the four tests that pin
them. Editor smoke test covers the settings store and build preloader.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKSbnZWCkxX1tN4wbgyJ9R
Copilot AI lite review requested due to automatic review settings September 4, 2026 20:29
@bobbyg603 bobbyg603 added this to the 5.0.0 milestone Sep 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are confirmed runtime-breaking identifier ambiguities in BugSplatManager plus a confirmed build-preload cleanup bug that can remove the wrong preloaded asset on subsequent builds.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the BugSplat Unity SDK initialization model so projects configure a single BugSplatOptions asset in Edit > Project Settings > BugSplat, and the SDK auto-initializes before the first scene loads (no scene BugSplatManager required), exposing the live client via BugSplat.Instance.

Changes:

  • Added BugSplat.Instance/BugSplat.Initialize(...) plus BeforeSceneLoad auto-initialization with a runtime host BugSplatRuntime MonoBehaviour.
  • Added editor Project Settings UI + build-time validation/preloading to ensure the selected options asset is present in players (and misconfigurations fail builds).
  • Updated tests, sample, and docs for the new initialization flow; obsoleted BugSplatManager and removed BugSplatRef.
File summaries
File Description
Tests/Runtime/Manager/BugSplatRefTest.cs Removed tests for deleted BugSplatRef.
Tests/Runtime/Manager/BugSplatManagerTest.cs Updated manager tests; suppress obsolete warning.
Tests/Runtime/Manager/BackgroundLogMessageQueueTest.cs Updated tests to call BugSplatRuntime.EnqueueUnobservedTaskException.
Tests/Runtime/BugSplatInitializationTests.cs.meta Added meta for new initialization test file.
Tests/Runtime/BugSplatInitializationTests.cs Added tests for Instance/auto-init/manager adoption/shutdown behavior.
Samples~/my-unity-crasher/Scripts/FeedbackPopup.cs Switched sample to BugSplat.Instance usage and new error messaging.
Samples~/my-unity-crasher/Scripts/CrashScenarioMenu.cs Switched sample to BugSplat.Instance + updated status/error text.
Samples~/my-unity-crasher/Scripts/BugSplatSettings.cs Switched sample initialization checks to BugSplat.IsInitialized.
Samples~/my-unity-crasher/Scenes/Sample.unity Removed BugSplatManager component from sample scene.
Samples~/my-unity-crasher/README.md Updated sample setup steps to Project Settings flow.
Runtime/Manager/BugSplatRuntime.cs.meta Added meta for new runtime host component.
Runtime/Manager/BugSplatRuntime.cs Added new internal host MonoBehaviour for log hooks + background queue draining.
Runtime/Manager/BugSplatRef.cs Removed obsolete internal wrapper type.
Runtime/Manager/BugSplatManager.cs Marked obsolete and updated to adopt/initialize via new initialization path.
Runtime/link.xml Updated linker doc comment for preloaded asset serialization.
Runtime/Client/BugSplatOptions.cs Added auto-init + capture flags; added config resolution & preloaded registration.
Runtime/BugSplat.Initialization.cs.meta Added meta for new initialization partial.
Runtime/BugSplat.Initialization.cs Added Instance/Initialize/Shutdown/auto-init logic + static reset.
Runtime/BugSplat.cs Made BugSplat partial; added InternalsVisibleTo for editor assembly.
README.md Updated primary setup docs to Project Settings + BugSplat.Instance.
Editor/PostBuild.cs Switched post-build options resolution to BugSplatProjectOptions.Get().
Editor/BugSplatSettingsProvider.cs.meta Added meta for new Project Settings provider.
Editor/BugSplatSettingsProvider.cs Added Project Settings UI for selecting/creating and editing options inline.
Editor/BugSplatProjectOptions.cs.meta Updated meta guid.
Editor/BugSplatProjectOptions.cs Added editor-side source of truth for selected options asset via config object.
Editor/BugSplatOptionsPreloader.cs.meta Updated meta guid.
Editor/BugSplatOptionsPreloader.cs Added build preprocessor/postprocessor to preload selected options asset + validate config.
Documentation~/usage.md Updated usage docs to reference BugSplat.Instance and options-asset flags.
Documentation~/migrating-from-4x.md Added migration guidance for new auto-init + obsolete manager.
Documentation~/ios.md Updated iOS config docs to point to options asset in Project Settings.
Documentation~/api.md Updated API docs for initialization + moved settings to options asset.
Documentation~/android.md Updated Android config docs to point to options asset in Project Settings.
CHANGELOG.md Documented new initialization flow, API additions, and removals.
Review details

Files not reviewed (4)

  • Editor/BugSplatSettingsProvider.cs.meta: Generated file
  • Runtime/BugSplat.Initialization.cs.meta: Generated file
  • Runtime/Manager/BugSplatRuntime.cs.meta: Generated file
  • Tests/Runtime/BugSplatInitializationTests.cs.meta: Generated file

Suppressed comments (2)

Runtime/Manager/BugSplatManager.cs:63

  • This call site has the same BugSplat identifier ambiguity as the property above; qualify the type to ensure Initialize is invoked on BugSplatUnity.BugSplat rather than via the instance property (which would recurse).
				BugSplat.Initialize(

Runtime/Manager/BugSplatManager.cs:87

  • This Shutdown call has the same BugSplat identifier ambiguity as the property above; qualify the type to avoid accidental recursion through the BugSplat instance property.
				BugSplat.Shutdown();
  • Files reviewed: 29/33 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +43 to +47
public BugSplat BugSplat => BugSplat.Instance;

private void Awake()
{
if (bugSplatOptions == null)
if (BugSplat.IsInitialized)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Declined — the code is correct as written, and the failure mode described cannot occur.

The property is public BugSplat BugSplat => BugSplat.Instance;, so its name and its type are the same identifier. That triggers C#'s identical simple names and type names rule (spec §12.8.7): in E.I where E could be either, a static I binds to the type and an instance I binds to the member. Instance, IsInitialized, Initialize and Shutdown are all static, so all four bind to BugSplatUnity.BugSplat.

Binding to the property instead could not silently recurse — it would not compile. Reaching a static member through an instance reference is CS0176, an error, not a warning. Evidence: the assembly builds with zero CS0176/CS0229, and the five Manager_* tests in BugSplatInitializationTests exercise every one of these members (Manager_WhenNotInitialized_InitializesFromItsOwnOptions reads manager.BugSplat, Manager_Destroyed_ShutsDownTheInstanceItCreated goes through OnDestroy → BugSplat.Shutdown()), and all pass. Infinite recursion through the property would fail them instantly.

The pattern is genuinely easy to misread, though, so cdc2d78 adds a comment above the property recording why these bind to the type. Note this shape predates the PR — 4.x had public BugSplat BugSplat => bugsplatRef.BugSplat;.

Comment on lines +25 to +27
public void OnPreprocessBuild(BuildReport report)
{
var options = BugSplatProjectOptions.Get();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in cdc2d78 — good catch.

OnPreprocessBuild now clears added as its first statement, so OnPostprocessBuild only ever undoes what the current build did:

public void OnPreprocessBuild(BuildReport report)
{
    // First, so postprocess only ever undoes what this build did. A build that fails after
    // this callback never reaches postprocess, which would otherwise leave a stale value
    // here and remove a preloaded asset the next build did not add.
    added = null;

The early-return path is deliberately left as a return: an entry that was already in Preloaded Assets before this build belongs to someone else, so with added now null postprocess correctly leaves it alone.

Covered by two new assertions in the editor smoke test, which simulate a build that set added and never reached postprocess: the next preprocess clears the marker, and the pre-existing entry survives postprocess.

One residual worth naming: a build that fails after we add the asset leaves that entry in Preloaded Assets, and the next build now (correctly) declines to remove it. Removing an entry we did not add is the worse of the two, and the new conflict check below surfaces the leftover if it is ever a different asset.

…code-only via BUGSPLAT_MANUAL_INITIALIZE

Scripts, CI, and AI agents never open a menu, so the settings page is now
one of three equal paths rather than the only one:

- A project's single BugSplatOptions asset is selected automatically, so
  writing one file configures the project. Documentation~/automation.md gives
  the YAML template (stable m_Script GUID), the .meta, and the m_configObjects
  entry for the several-assets case.
- BugSplatSetup.Configure / CreateAsset and BugSplatProjectOptions are public;
  -executeMethod BugSplatUnity.Editor.BugSplatSetup.ConfigureFromCommandLine
  -bugsplatDatabase <name> creates or updates and selects the asset, exiting
  0/1. The settings page's Create button calls the same code.
- Every "not configured" message ends with a shared BugSplatOptions.ConfigureHint
  naming the file fix next to the menu, so a log is enough to act on.
- BUGSPLAT_MANUAL_INITIALIZE hands initialization to the project: no auto-init,
  no startup warning, no build check. The preprocessor reads the define for the
  target being built.

Verified in batch mode: setup smoke 12/12, the command line succeeds (0) and
fails (1) as documented and writes the documented on-disk shapes; PlayMode
suite 148/148.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKSbnZWCkxX1tN4wbgyJ9R
Copilot AI review requested due to automatic review settings September 4, 2026 20:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The build preloader can silently allow multiple preloaded BugSplatOptions, which can make player initialization order-dependent and select the wrong configuration at runtime.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (4)

  • Editor/BugSplatSettingsProvider.cs.meta: Generated file
  • Editor/BugSplatSetup.cs.meta: Generated file
  • Runtime/BugSplat.Initialization.cs.meta: Generated file
  • Runtime/Manager/BugSplatRuntime.cs.meta: Generated file
  • Files reviewed: 32/36 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread Editor/BugSplatOptionsPreloader.cs Outdated
Comment on lines +55 to +66
// Added even when Initialize Automatically is off: BugSplat still reads the asset at
// startup to learn that it should stay quiet, and without it would warn that nothing is
// configured.
var preloaded = PlayerSettings.GetPreloadedAssets();
if (preloaded.Contains(options))
{
return;
}

PlayerSettings.SetPreloadedAssets(preloaded.Append(options).ToArray());
added = options;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Both halves fixed in cdc2d78. The first is the sharper of the two review points on this PR — thank you.

A different preloaded BugSplatOptions now fails the build. You are right that ResolveConfigured() in a player depends on OnEnable first-wins, so two preloaded options assets make the result depend on load order — which is precisely the arbitrary-asset problem this PR exists to remove, resurfacing at runtime where nothing can report it. OnPreprocessBuild now refuses it:

var conflicting = preloaded.OfType<BugSplatOptions>().Where(asset => asset != options).ToArray();
if (conflicting.Length > 0) { throw new BuildFailedException(/* names each path and where to remove it */); }

Skipped under BUGSPLAT_MANUAL_INITIALIZE, where AutoInitialize returns before reading the preloaded asset at all. The selected asset appearing twice is not treated as a conflict — same asset, same outcome.

Defence in depth at runtime. If an asset is preloaded some other way, OnEnable now warns naming both assets rather than losing silently. That code and its static are also now #if !UNITY_EDITOR: the editor resolves from EditorBuildSettings, and OnEnable fires for any asset the editor merely loads, so merely inspecting one would have set the static.

added on the early return — fixed as described in the other thread; it is cleared at the top of preprocess.

Verified by four new assertions in the editor smoke test (fails and names the offending asset, says where to remove it, leaves added clear, and does not flag the same asset twice), plus a note in automation.md and the changelog, since a script hand-editing ProjectSettings.asset is exactly who would trip over this.

…cond preloaded options asset

Copilot review on #254.

- BugSplatOptionsPreloader.OnPreprocessBuild clears `added` first, so
  OnPostprocessBuild only ever undoes what this build did. A build that failed
  after preprocess left a stale marker behind, and the next build's postprocess
  would then remove a preloaded entry it had not added.
- A BugSplatOptions in Player Settings > Preloaded Assets other than the
  selected one now fails the build, naming it. A player resolves its options
  from the first preloaded asset loaded, so two would make the choice depend on
  load order — the arbitrary-asset problem this flow exists to prevent. Skipped
  under BUGSPLAT_MANUAL_INITIALIZE, where nothing reads the preloaded asset.
- BugSplatOptions.OnEnable and its static are now player-only (the editor reads
  the selection from EditorBuildSettings, and OnEnable fires for any asset the
  editor merely loads), and a second asset arriving there warns instead of
  losing silently.
- Documented the Preloaded Assets rule in automation.md and the changelog, and
  noted in BugSplatManager why "BugSplat.X" binds to the type, not the
  same-named property.

Verified: PlayMode suite 148/148, zero compile errors or warnings; new preload
smoke 12/12 covering the stale-marker and conflict paths; settings-store and
setup smokes green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKSbnZWCkxX1tN4wbgyJ9R
Copilot AI review requested due to automatic review settings September 4, 2026 21:15
@bobbyg603

Copy link
Copy Markdown
Member Author

Copilot review triaged — 2 of 3 valid, both fixed in cdc2d78.

Thread Verdict
BugSplatOptionsPreloader.cs — stale static added on the early return Fixed. Cleared at the top of OnPreprocessBuild, so postprocess only undoes this build.
BugSplatOptionsPreloader.cs — a second preloaded BugSplatOptions makes runtime resolution order-dependent Fixed. The build now fails and names it; OnEnable warns as a backstop and is now player-only.
BugSplatManager.cs — BugSplat property hides the type, BugSplat.Instance recurses Declined. Static members bind to the type under the identical-names rule; binding to the property would be CS0176, a compile error, not silent recursion. Zero such diagnostics, and the five Manager_* tests exercising these members pass. Comment added so it is not re-read as a bug.

Verification: PlayMode suite 148/148, zero compile errors or warnings. New preload smoke 12/12 covering the stale-marker and conflict paths; the settings-store and setup smokes are green.

One honest note on my own tooling: re-running the older settings-store smoke surfaced a failure that turned out to be a stale assertion in that throwaway script — it still expected the pre-e18f744 wording "no BugSplat Options asset" after that commit renamed it to "no BugSplatOptions asset". Product behaviour was correct; the script was not. Fixed and re-run green.

Left for the reviewer to resolve rather than auto-resolving.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

BugSplatSetup.CreateAsset can create duplicate options assets (non-idempotent scripted setup) when an explicit -bugsplatAssetPath already exists but no asset is selected.

Review details

Files not reviewed (4)

  • Editor/BugSplatSettingsProvider.cs.meta: Generated file
  • Editor/BugSplatSetup.cs.meta: Generated file
  • Runtime/BugSplat.Initialization.cs.meta: Generated file
  • Runtime/Manager/BugSplatRuntime.cs.meta: Generated file

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

Editor/BugSplatSetup.cs:95

  • CreateAsset always calls AssetDatabase.GenerateUniqueAssetPath(assetPath), so if the caller passes an explicit -bugsplatAssetPath that already contains a BugSplatOptions asset (and no asset is currently selected), this will create a second options asset at a suffixed path instead of selecting/updating the existing one. That makes scripted setup non-idempotent and can accidentally multiply assets in multi-asset projects. Prefer reusing an existing BugSplatOptions at the requested path when present, falling back to GenerateUniqueAssetPath only when no options asset exists there.
    Documentation~/automation.md:73
  • This sentence implies the command will create the asset at exactly the provided -bugsplatAssetPath, but BugSplatSetup.CreateAsset currently may choose a suffixed unique path if the target file already exists. Either clarify here that the path may be made unique, or (preferably) update CreateAsset to reuse an existing BugSplatOptions at the requested path so scripted setup remains deterministic.
  • Files reviewed: 32/36 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

…LED define, dev builds warn instead of failing

A project should not have to fill in a database before it can build, and a team
that wants BugSplat only in QA and release needs a switch. Both escape hatches
that existed were mislabelled for the purpose: "Initialize Automatically" says
who starts BugSplat, not whether it runs.

- BugSplatOptions.Enabled (default on): off means BugSplat does not start itself
  and builds are not validated. An explicit BugSplat.Initialize call is still
  honored. Logs one line at Log level in a development build, so a build that
  reports nothing is never a mystery.
- BUGSPLAT_DISABLED define: the same, per build target, for build scripts and CI
  that keep BugSplat out of development builds.
- Misconfiguration now fails a *release* build and only warns on a development
  build (BuildOptions.Development), so iterating never requires configuring
  BugSplat first.
- The two axes are now non-redundant: Enabled gates initialization and
  validation; InitializeAutomatically gates only self-start, so an empty
  database with it off now correctly fails rather than passing silently.

Verified: PlayMode suite 151/151. Smokes green, including a new upgrade check
proving an options asset written before these fields loads with Enabled and all
three capture options ON — Unity keeps field initializers for absent keys, so
upgrading cannot silently disable reporting.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKSbnZWCkxX1tN4wbgyJ9R
Copilot AI review requested due to automatic review settings September 4, 2026 21:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are verified runtime/configuration edge cases (empty Database causing startup exceptions, editor single-asset auto-selection not applied in ResolveConfigured, and conflicting changelog guidance) that should be fixed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (4)

  • Editor/BugSplatSettingsProvider.cs.meta: Generated file
  • Editor/BugSplatSetup.cs.meta: Generated file
  • Runtime/BugSplat.Initialization.cs.meta: Generated file
  • Runtime/Manager/BugSplatRuntime.cs.meta: Generated file

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

Runtime/Client/BugSplatOptions.cs:204

  • In the editor, ResolveConfigured only checks EditorBuildSettings and returns null otherwise. That means a project with exactly one BugSplatOptions asset but no selection (the 4.x upgrade case) will still be treated as unconfigured in play mode until something else writes the config object, which conflicts with the documented/desired "single asset is selected automatically" behavior. Consider adding the same single-asset fallback here (only when exactly one exists) and persisting it into EditorBuildSettings.
    CHANGELOG.md:59
  • The changelog says BugSplatRef "moved to BugSplatUnity.Runtime.Manager" here, but this PR removes BugSplatRef entirely (and the "Removed" section below also states it was removed). This is conflicting guidance for upgraders; update this bullet to avoid implying BugSplatRef still exists under a new namespace.
  • Files reviewed: 32/36 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread Runtime/BugSplat.Initialization.cs
The sample exists to demonstrate what a real integration looks like, and an
unsymbolicated stack is most of the value missing. macOS and Android were off,
and Windows was absent from the asset entirely - true only because the class
initializer says so, which would silently flip if that default ever changed.
All four are now stated explicitly.

Also drops the asset's SymbolUploadClientId and SymbolUploadClientSecret keys.
Both fields were removed from BugSplatOptions in 5.0.0 precisely so a secret
could not be serialized into version control, so shipping them in the sample
named an API that no longer exists.

Credentials are still never stored on the asset; without them a build warns and
succeeds, which the sample README now explains along with the extra Windows
requirements.

Verified by loading the edited asset in Unity: it parses, all four flags arrive
true, and nothing else in it shifted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKSbnZWCkxX1tN4wbgyJ9R
Copilot AI review requested due to automatic review settings September 4, 2026 22:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Build/setup validation and documentation currently treat whitespace-only database values and manual-init validation rules inconsistently, which can allow “configured but reports nothing” states to slip through.

Review details

Files not reviewed (4)

  • Editor/BugSplatSettingsProvider.cs.meta: Generated file
  • Editor/BugSplatSetup.cs.meta: Generated file
  • Runtime/BugSplat.Initialization.cs.meta: Generated file
  • Runtime/Manager/BugSplatRuntime.cs.meta: Generated file

Suppressed comments (7)

Previously missed (5) — in code that hasn't changed since the last review.

Editor/BugSplatOptionsPreloader.cs:82

  • Build validation only checks string.IsNullOrEmpty(options.Database), so a whitespace-only Database can slip through release builds even though the player will report nothing. Use IsNullOrWhiteSpace so whitespace-only database values fail/warn consistently.
    Editor/BugSplatSettingsProvider.cs:128
  • The Project Settings page treats a whitespace-only Database as configured (IsNullOrEmpty check). That can show a misleading status and also disagrees with build/runtime intent. Use IsNullOrWhiteSpace so " " is considered empty.
    Editor/BugSplatSetup.cs:39
  • BugSplatSetup.Configure treats a whitespace-only -bugsplatDatabase value as "present" (IsNullOrEmpty), which can create/select an options asset with an invalid Database and still exit 0. Use IsNullOrWhiteSpace so " " is rejected like an empty string.

This issue also appears in the following locations of the same file:

  • line 48
  • line 63
    Documentation~/api.md:26
  • The BUGSPLAT_MANUAL_INITIALIZE row says "The database is still validated", but BugSplatOptionsPreloader intentionally skips build validation when that define is set (manual mode). The table should match the implemented behavior to avoid misleading users.
    Documentation~/api.md:31
  • This note states that misconfiguration always fails release builds, but builds are explicitly not validated when BUGSPLAT_DISABLED or BUGSPLAT_MANUAL_INITIALIZE is defined, or when Enabled is unchecked. Clarify the note so it reflects the actual build behavior.

Editor/BugSplatSetup.cs:51

  • BugSplatSetup.Configure will overwrite options.Database with a whitespace-only database argument (because the guard uses IsNullOrEmpty). That makes it easy to accidentally blank/invalid-configure the project from CI/CLI while still passing the later IsNullOrEmpty check. Guard with IsNullOrWhiteSpace instead.
			if (!string.IsNullOrEmpty(database))
			{
				options.Database = database;
			}

Editor/BugSplatSetup.cs:67

  • BugSplatSetup.Configure validates Database with IsNullOrEmpty, so a whitespace-only Database (" ") is treated as valid and the command exits successfully, but BugSplat would still effectively be unconfigured. Validate with IsNullOrWhiteSpace to reject whitespace-only values.
			if (string.IsNullOrEmpty(options.Database))
			{
				throw new ArgumentException(
					$"BugSplat: a database is required. Pass one, or set Database on {AssetDatabase.GetAssetPath(options)}.");
			}
  • Files reviewed: 33/37 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@bobbyg603 bobbyg603 mentioned this pull request Sep 4, 2026
46 of 96 tasks
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 4, 2026 23:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Editor auto-initialization doesn’t currently implement the “single options asset auto-selected” upgrade behavior, and the obsolete BugSplatManager bypasses BugSplatOptions.Enabled, undermining the documented “off” switch when a leftover manager exists.

Review details

Files not reviewed (4)

  • Editor/BugSplatSettingsProvider.cs.meta: Generated file
  • Editor/BugSplatSetup.cs.meta: Generated file
  • Runtime/BugSplat.Initialization.cs.meta: Generated file
  • Runtime/Manager/BugSplatRuntime.cs.meta: Generated file

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

Runtime/Client/BugSplatOptions.cs:207

  • In the editor, ResolveConfigured() only reads the EditorBuildSettings config object. That means a project with exactly one BugSplatOptions asset (the intended 4.x upgrade case) will still be treated as “not configured” in play mode until something calls BugSplatProjectOptions.Get()/Set (e.g., opening the settings page or running a build), contradicting the “single asset is selected automatically / initializes in the editor” behavior described in the PR.
    Runtime/Manager/BugSplatManager.cs:74
  • BugSplatOptions.Enabled is documented as the project-wide “BugSplat off” switch, but the obsolete BugSplatManager still calls BugSplat.Initialize even when its referenced BugSplatOptions has Enabled unchecked. This makes it easy for upgraded projects to think BugSplat is disabled (via the asset) while a leftover manager in a scene still starts reporting.
    Editor/BugSplatOptionsPreloader.cs:21
  • The class summary says nothing is checked under BUGSPLAT_MANUAL_INITIALIZE, but OnPreprocessBuild still checks for conflicting preloaded BugSplatOptions (and may fail the build) even when manual=true. The comment should reflect the actual behavior so readers don’t assume manual-init builds skip all validation.
  • Files reviewed: 33/37 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants