update perf monitor#836
Closed
Devin T. Currie (DTCurrie) wants to merge 1 commit into
Closed
Conversation
🦋 Changeset detectedLatest commit: 90e8f10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
2c6a430 to
90e8f10
Compare
Contributor
|
Member
Author
|
I do not think this is a good solution, and I think this may be worth pushing an update to |
|
|
||
| let perf: ThreePerf | ||
|
|
||
| $effect.pre(() => { |
There was a problem hiding this comment.
It's weird that claude is still $effect.pre happy, neither of these should be pre.
Member
Author
There was a problem hiding this comment.
yeah can't seem to get it off them
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the three-perf performance monitor when the visualizer is embedded in a host page that serves a strict Content-Security-Policy (for example app.viam.com). Previously the monitor anchored to the corner of the whole viewport instead of the visualizer container, and its numeric counters (GPU/CPU/FPS plus the geometry/texture/shader stats) were blank while only the line graphs drew.
Frontend
PerfMonitor.svelte(new): a local replacement for@threlte/extras'sPerfMonitor. It mounts three-perf into Threlte's canvas wrapper (useThrelte().dom) instead ofdocument.body, overrides three-perf's hardcodedposition: fixedtoabsoluteso the monitor anchors to the visualizer container, and setsz-index: 10so it sits above the overlay panels (Details and FloatingPanel atz-4/z-5) that share its top-right corner but below the fullscreen and file-drop layers.Scene.svelte: render the localPerfMonitorinstead of the one from@threlte/extras.App.svelte: callconfigureTextBuilder({ useWorker: false })fromtroika-three-textin the component'smodulescript so troika typesets text on the main thread instead of in ablob:worker. It runs on import, before any troikaTextmounts, which is required because troika ignores the setting after its first font request. Note this switches every troikaTexton the page to main-thread typesetting, which is intended.vite-env.d.ts: ambient module declaration fortroika-three-text, which ships no types, covering the single export we import.package.json: addthree-perfandtroika-three-textas direct dependencies. Both were already present transitively (three-perf via@threlte/extras, troika via both), declaring them matches the new direct imports, and they dedupe to a singletroika-three-text@0.52.4.Two changes outside
src/: a drive-by fix to theMakefileuptarget, which ranpnpm up(pnpm resolves bareupto its built-inupdatealias, so it mutatedpackage.json/pnpm-lock.yamlinstead of running theupscript) and now runspnpm run up, and a new.claude/skills/verifyguide documenting how to build and drive the app locally.Why?
Why disable troika's worker instead of allowing it in the host CSP?
troika-worker-utils loads its worker in two stages: it creates the worker from a small bootstrap
blob:, then the worker callsimportScripts()on a secondblob:URL to pull in the real typesetting code. A strict host CSP allows the first step (worker creation, governed byworker-src) but blocks the second (importScriptsof ablob:, governed by the worker'sscript-src), so the worker starts, fails to load its code, and every troikaTextrenders blank. This is also why troika's ownsupportsWorkers()fallback does not save it: it only checks whethernew Worker()throws synchronously, which succeeds, so troika commits to the worker path and only fails later in stage two. Relaxing the host CSP is the wrong fix. We do not own that policy, it would have to be repeated for every host that embeds the visualizer, and addingblob:toscript-srcweakens the host's XSS posture just so a debug overlay can typeset numbers off the main thread. Disabling the worker makes the library self-sufficient on any host, and the cost is negligible here: we render only short labels, and the expensive part (SDF glyph generation) already runs on the main thread via WebGL regardless of this setting.Why do our other blob workers, like the PCD loader, keep working under the same CSP?
They are self-contained. The PCD loader inlines its entire parser into one worker string and never calls
importScripts, so it only needs worker creation, which the CSP permits. troika is the only worker that fetches its code in a secondimportScripts(blob:)step, which is the step the CSP blocks. troika-worker-utils exposes no option to emit a self-contained worker, soconfigureTextBuilder({ useWorker: false })is the supported escape hatch.Why fork the Threlte component instead of passing
domElementplus a CSS rule?Threlte's
PerfMonitordoes accept adomElementprop, but three-perf hardcodesposition: fixedon its wrapper, so correcting the anchor still requires overriding that style. Doing it with a global CSS rule fails in the exact embedded case this PR targets: the standaloneapp.cssdoes not ship with the packaged library (svelte-package builds fromsrc/lib, which imports no CSS), and having a library leak a global#three-perf-uirule into a host page is a worse smell than the self-contained JS override. The fork duplicates a small amount of stable stage-wiring, which is the accepted tradeoff.Testing
Ran
pnpm check(svelte-check + go vet) andpnpm lint(prettier + eslint), both clean.Verified behavior by driving the running app headless with Playwright against the snapshot scene, since the bug is only observable at runtime. Toggled Settings > Debug > Render stats and confirmed the monitor anchors to the container's top-right corner rather than the viewport, with all numeric counters rendering. Intercepted every
Workerconstruction and confirmed none originate from troika (only the PCD, GLTF, and Sentry blob workers appear). With an entity selected, confirmed the monitor paints above the Details panel that shares its corner. No automated tests were added; this is a UI and debug-overlay behavior change verified by hand.