Inspector v2: Perf viewer bug fixes (#17736)

I noticed a handful of bugs in the perf viewer implementation for
Inspector v2. This PR addresses them:
- The `Start Recording` button is not re-rendered to say `Stop
Recording` because no state changes so the component isn't re-rendered.
The quick fix is to just add an `isRecording` state.
- The side pane text is always white, regardless of theme. The fix is to
use Fluent components instead of raw `<span>`s.

I also made a few other changes for general cleanup:
- Used Switch instead of Checkbox as I think this is more consistent
with the rest of the Inspector v2 UI (open to other opinions on this
though).
- Fixed various spacing and alignment issues.
- Used `mergeClasses` as class name concatenation is documented as not
being guaranteed to behave the way you want with Fluent.

<img width="1229" height="774" alt="image"
src="https://github.com/user-attachments/assets/dfa9aea3-0bc1-4a19-a356-94a81ad6c068"
/>

---------

Co-authored-by: Georgina Halpern <[email protected]>
Co-authored-by: Georgie <[email protected]>

Author: 3 people

packages/dev/inspector-v2/src/components/performanceViewer/performanceSidebar.tsx

@@ -1,17 +1,17 @@
+import type { FunctionComponent } from "react";
+
+import type { Color4 } from "core/Maths/math.color";
 import type { IPerfMetadata } from "core/Misc/interfaces/iPerfViewer";
-import type { PerformanceViewerCollector } from "core/Misc/PerformanceViewer/performanceViewerCollector";
 import type { Observable } from "core/Misc/observable";
-import type { FunctionComponent } from "react";
+import type { PerformanceViewerCollector } from "core/Misc/PerformanceViewer/performanceViewerCollector";
 import type { PerfMinMax, VisibleRangeChangedObservableProps } from "./graphSupportingTypes";
 
-import { makeStyles, tokens } from "@fluentui/react-components";
+import { Body1, makeStyles, mergeClasses, Subtitle2Stronger, tokens } from "@fluentui/react-components";
 import { useEffect, useState } from "react";
 
-import type { Color4 } from "core/Maths/math.color";
 import { Color3 } from "core/Maths/math.color";
-import { AbstractEngine } from "core/Engines/abstractEngine";
-import { Checkbox } from "shared-ui-components/fluent/primitives/checkbox";
 import { ColorPickerPopup } from "shared-ui-components/fluent/primitives/colorPicker";
+import { Switch } from "shared-ui-components/fluent/primitives/switch";
 
 const useStyles = makeStyles({
     sidebar: {
@@ -28,26 +28,16 @@ const useStyles = makeStyles({
         display: "grid",
         width: "100%",
         minHeight: "30px",
-        fontSize: "14px",
-        padding: "2.5px 0px",
+        padding: `${tokens.spacingVerticalXXS} 0`,
         alignItems: "center",
     },
     header: {
-        color: tokens.colorNeutralForegroundOnBrand,
-        backgroundColor: tokens.colorBrandBackground,
-        gridTemplateColumns: "10px 9fr 1fr 10px",
-    },
-    versionHeader: {
-        backgroundColor: tokens.colorNeutralBackground3,
         color: tokens.colorNeutralForeground1,
-        gridTemplateColumns: "10px 1fr 1fr 10px",
-        fontSize: "14px",
-        minHeight: "35px",
+        backgroundColor: tokens.colorBrandBackground,
+        gridTemplateColumns: "10px 9fr 1fr 8px",
     },
     categoryHeader: {
         backgroundColor: tokens.colorNeutralBackground4,
-        textTransform: "uppercase",
-        fontSize: "14px",
         minHeight: "30px",
     },
     categoryColumn2: {
@@ -60,7 +50,7 @@ const useStyles = makeStyles({
     },
     measure: {
         color: tokens.colorNeutralForeground1,
-        gridTemplateColumns: "18px 6fr 1fr",
+        gridTemplateColumns: "4px 6fr 1fr 10px",
     },
     measureOdd: {
         backgroundColor: tokens.colorNeutralBackground2,
@@ -70,7 +60,7 @@ const useStyles = makeStyles({
     },
     measureCategory: {
         display: "grid",
-        gridTemplateColumns: "18px 7px 18px 10px 1fr",
+        gridTemplateColumns: "auto 7px 18px 10px 1fr",
         gridColumn: "2",
         alignItems: "center",
     },
@@ -82,6 +72,7 @@ const useStyles = makeStyles({
     },
     measureValue: {
         gridColumn: "3",
+        textAlign: "right",
     },
 });
 
@@ -175,33 +166,28 @@ export const PerformanceSidebar: FunctionComponent<IPerformanceSidebarProps> = (
                 metadataCategories.map((category) => (
                     <div key={`category-${category || "version"}`}>
                         {category ? (
-                            <div className={`${classes.sidebarItem} ${classes.header} ${classes.categoryHeader}`} key={`header-${category}`}>
-                                <span className={classes.categoryColumn2}>{category}</span>
+                            <div className={mergeClasses(classes.sidebarItem, classes.header, classes.categoryHeader)} key={`header-${category}`}>
+                                <Subtitle2Stronger className={classes.categoryColumn2}>{category}</Subtitle2Stronger>
                                 <div className={classes.categoryColumn3}>
-                                    <Checkbox value={metadataCategoryChecked?.get(category) === metadataCategoryId?.get(category)?.length} onChange={onCheckAllChange(category)} />
+                                    <Switch value={metadataCategoryChecked?.get(category) === metadataCategoryId?.get(category)?.length} onChange={onCheckAllChange(category)} />
                                 </div>
                             </div>
-                        ) : (
-                            <div className={`${classes.sidebarItem} ${classes.versionHeader}`} key={"header-version"}>
-                                <span className={classes.categoryColumn2}>Version:</span>
-                                <span className={classes.categoryColumn3}>{AbstractEngine.Version}</span>
-                            </div>
-                        )}
+                        ) : null}
                         {metadataCategoryId?.get(category)?.map((id, index) => {
                             const metadata = metadataMap?.get(id);
                             const range = valueMap?.get(id);
                             return (
                                 metadata && (
                                     <div
                                         key={`perf-sidebar-item-${id}`}
-                                        className={`${classes.sidebarItem} ${classes.measure} ${index % 2 === 0 ? classes.measureEven : classes.measureOdd}`}
+                                        className={mergeClasses(classes.sidebarItem, classes.measure, index % 2 === 0 ? classes.measureEven : classes.measureOdd)}
                                     >
                                         <div className={classes.measureCategory}>
-                                            <Checkbox value={!metadata.hidden} onChange={onCheckChange(id)} />
+                                            <Switch value={!metadata.hidden} onChange={onCheckChange(id)} />
                                             <div className={classes.measureColorPicker}>
                                                 <ColorPickerPopup value={Color3.FromHexString(metadata.color ?? "#000")} onChange={onColorChange(id)} />
                                             </div>
-                                            <span className={classes.measureLabel}>{id}</span>
+                                            <Body1 className={classes.measureLabel}>{id}</Body1>
                                         </div>
                                         {range && <div className={classes.measureValue}> {((range.min + range.max) / 2).toFixed(2)} </div>}
                                     </div>

…ormanceViewer/performanceViewerPopup.tsx …/performanceViewer/performanceViewer.tsxpackages/dev/inspector-v2/src/components/performanceViewer/performanceViewerPopup.tsx renamed to packages/dev/inspector-v2/src/components/performanceViewer/performanceViewer.tsx packages/dev/inspector-v2/src/components/performanceViewer/performanceViewerPopup.tsx renamed to packages/dev/inspector-v2/src/components/performanceViewer/performanceViewer.tsx

@@ -41,15 +41,15 @@ const useStyles = makeStyles({
     },
 });
 
-interface IPerformanceViewerPopupProps {
+type PerformanceViewerProps = {
     scene: Scene;
     layoutObservable: Observable<PerfLayoutSize>;
     returnToLiveObservable: Observable<void>;
     performanceCollector: PerformanceViewerCollector;
     initialGraphSize?: Vector2;
-}
+};
 
-export const PerformanceViewerPopup: FunctionComponent<IPerformanceViewerPopupProps> = (props) => {
+export const PerformanceViewer: FunctionComponent<PerformanceViewerProps> = (props) => {
     const { scene, layoutObservable, returnToLiveObservable, performanceCollector, initialGraphSize } = props;
     const classes = useStyles();
     const [onVisibleRangeChangedObservable] = useState(() => new BabylonObservable<VisibleRangeChangedObservableProps>());

packages/dev/inspector-v2/src/components/stats/performanceStats.tsx

@@ -1,10 +1,11 @@
-import type { PerformanceViewerCollector, Scene } from "core/index";
-import { Vector2 } from "core/Maths/math.vector";
-
 import type { FunctionComponent } from "react";
 
+import type { PerformanceViewerCollector, Scene } from "core/index";
+
+import { ArrowDownloadRegular, RecordRegular, StopRegular } from "@fluentui/react-icons";
 import { useCallback, useEffect, useRef, useState } from "react";
 
+import { Vector2 } from "core/Maths/math.vector";
 import { Observable } from "core/Misc/observable";
 import { PerfCollectionStrategy } from "core/Misc/PerformanceViewer/performanceViewerCollectionStrategies";
 import "core/Misc/PerformanceViewer/performanceViewerSceneExtension";
@@ -14,7 +15,30 @@ import { ButtonLine } from "shared-ui-components/fluent/hoc/buttonLine";
 import { ChildWindow } from "shared-ui-components/fluent/hoc/childWindow";
 import { FileUploadLine } from "shared-ui-components/fluent/hoc/fileUploadLine";
 import type { PerfLayoutSize } from "../performanceViewer/graphSupportingTypes";
-import { PerformanceViewerPopup } from "../performanceViewer/performanceViewerPopup";
+import { PerformanceViewer } from "../performanceViewer/performanceViewer";
+
+function AddStrategies(perfCollector: PerformanceViewerCollector) {
+    perfCollector.addCollectionStrategies(...DefaultStrategiesList);
+    if (PressureObserverWrapper.IsAvailable) {
+        // Do not enable for now as the Pressure API does not
+        // report factors at the moment.
+        // perfCollector.addCollectionStrategies({
+        //     strategyCallback: PerfCollectionStrategy.ThermalStrategy(),
+        //     category: IPerfMetadataCategory.FrameSteps,
+        //     hidden: true,
+        // });
+        // perfCollector.addCollectionStrategies({
+        //     strategyCallback: PerfCollectionStrategy.PowerSupplyStrategy(),
+        //     category: IPerfMetadataCategory.FrameSteps,
+        //     hidden: true,
+        // });
+        perfCollector.addCollectionStrategies({
+            strategyCallback: PerfCollectionStrategy.PressureStrategy(),
+            category: PerfMetadataCategory.FrameSteps,
+            hidden: true,
+        });
+    }
+}
 
 const enum PerfMetadataCategory {
     Count = "Count",
@@ -53,18 +77,20 @@ const InitialGraphSize = new Vector2(724, 512);
 
 export const PerformanceStats: FunctionComponent<{ context: Scene }> = ({ context: scene }) => {
     const [isOpen, setIsOpen] = useState(false);
+    const [isRecording, setIsRecording] = useState(false);
     const [isLoadedFromCsv, setIsLoadedFromCsv] = useState(false);
     const [performanceCollector, setPerformanceCollector] = useState<PerformanceViewerCollector | undefined>();
     const [layoutObservable] = useState(() => new Observable<PerfLayoutSize>());
     const [returnToLiveObservable] = useState(() => new Observable<void>());
-    const childWindowRef = useRef<{ open: (options?: { defaultWidth?: number; defaultHeight?: number; title?: string }) => void; close: () => void }>(null);
+    const childWindowRef = useRef<ChildWindow>(null);
 
     useEffect(() => {
         if (!isLoadedFromCsv) {
             if (performanceCollector) {
+                setIsRecording(false);
                 performanceCollector.stop();
                 performanceCollector.clear(false);
-                addStrategies(performanceCollector);
+                AddStrategies(performanceCollector);
             }
         }
     }, [isLoadedFromCsv, performanceCollector]);
@@ -95,6 +121,7 @@ export const PerformanceStats: FunctionComponent<{ context: Scene }> = ({ contex
 
     const onPerformanceButtonClick = () => {
         setIsOpen(true);
+        setIsRecording(true);
         performanceCollector?.start(true);
         startPerformanceViewerPopup();
     };
@@ -104,11 +131,13 @@ export const PerformanceStats: FunctionComponent<{ context: Scene }> = ({ contex
             // reopen window and load data!
             setIsOpen(false);
             setIsLoadedFromCsv(true);
+            setIsRecording(false);
             performanceCollector?.stop();
             const isValid = performanceCollector?.loadFromFileData(data);
             if (!isValid) {
-                // if our data isnt valid we close the window.
+                // if our data isn't valid we close the window.
                 setIsOpen(false);
+                setIsRecording(true);
                 performanceCollector?.start(true);
             } else {
                 startPerformanceViewerPopup();
@@ -121,39 +150,20 @@ export const PerformanceStats: FunctionComponent<{ context: Scene }> = ({ contex
     };
 
     const onToggleRecording = () => {
-        if (!performanceCollector?.isStarted) {
-            performanceCollector?.start(true);
-        } else {
-            performanceCollector?.stop();
-        }
-    };
-
-    const addStrategies = (perfCollector: PerformanceViewerCollector) => {
-        perfCollector.addCollectionStrategies(...DefaultStrategiesList);
-        if (PressureObserverWrapper.IsAvailable) {
-            // Do not enable for now as the Pressure API does not
-            // report factors at the moment.
-            // perfCollector.addCollectionStrategies({
-            //     strategyCallback: PerfCollectionStrategy.ThermalStrategy(),
-            //     category: IPerfMetadataCategory.FrameSteps,
-            //     hidden: true,
-            // });
-            // perfCollector.addCollectionStrategies({
-            //     strategyCallback: PerfCollectionStrategy.PowerSupplyStrategy(),
-            //     category: IPerfMetadataCategory.FrameSteps,
-            //     hidden: true,
-            // });
-            perfCollector.addCollectionStrategies({
-                strategyCallback: PerfCollectionStrategy.PressureStrategy(),
-                category: PerfMetadataCategory.FrameSteps,
-                hidden: true,
-            });
+        if (performanceCollector) {
+            if (!performanceCollector.isStarted) {
+                setIsRecording(true);
+                performanceCollector.start(true);
+            } else {
+                setIsRecording(false);
+                performanceCollector.stop();
+            }
         }
     };
 
     useEffect(() => {
         const perfCollector = scene.getPerfCollector();
-        addStrategies(perfCollector);
+        AddStrategies(perfCollector);
         setPerformanceCollector(perfCollector);
     }, [scene]);
 
@@ -173,11 +183,11 @@ export const PerformanceStats: FunctionComponent<{ context: Scene }> = ({ contex
         <>
             {!isOpen && <ButtonLine label="Open Realtime Perf Viewer" onClick={onPerformanceButtonClick} />}
             {!isOpen && <FileUploadLine label="Load Perf Viewer using CSV" accept=".csv" onClick={onLoadClick} />}
-            <ButtonLine label="Export Perf to CSV" onClick={onExportClick} />
-            {!isOpen && <ButtonLine label={performanceCollector?.isStarted ? "Stop Recording" : "Begin Recording"} onClick={onToggleRecording} />}
+            <ButtonLine label="Export Perf to CSV" icon={ArrowDownloadRegular} onClick={onExportClick} />
+            {!isOpen && <ButtonLine label={isRecording ? "Stop Recording" : "Begin Recording"} icon={isRecording ? StopRegular : RecordRegular} onClick={onToggleRecording} />}
             <ChildWindow id="performance-viewer" imperativeRef={childWindowRef} onOpenChange={(open) => !open && onClosePerformanceViewer()}>
                 {performanceCollector && (
-                    <PerformanceViewerPopup
+                    <PerformanceViewer
                         scene={scene}
                         layoutObservable={layoutObservable}
                         returnToLiveObservable={returnToLiveObservable}