[InspectorV2] Particle attractor PR comments (#17014)

Addressing comments from
#17012 after it merged

---------

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

Author: georginahalpern

packages/dev/inspector-v2/src/components/properties/particles/attractor.tsx

@@ -7,10 +7,10 @@ import type { Color3 } from "core/Maths";
 import { Color4, Matrix } from "core/Maths";
 import type { AbstractMesh } from "core/Meshes";
 import { CreateSphere } from "core/Meshes";
-import type { Observer } from "core/Misc";
 import type { Attractor } from "core/Particles";
 import type { Scene } from "core/scene";
-import { useCallback, useEffect, useRef, useState, type FunctionComponent } from "react";
+import { useCallback, useEffect, useState } from "react";
+import type { FunctionComponent } from "react";
 import { SyncedSliderInput } from "shared-ui-components/fluent/primitives/syncedSlider";
 import { ToggleButton } from "shared-ui-components/fluent/primitives/toggleButton";
 import { useAsyncResource, useResource } from "../../../hooks/resourceHooks";
@@ -46,15 +46,14 @@ const CreateImpostor = (id: number, scene: Scene, attractor: Attractor, initialS
     return impostor;
 };
 
-async function CreateTextRendererAsync(scene: Scene, impostor: AbstractMesh, index: number, color: Color3) {
+async function CreateTextRendererAsync(id: number, scene: Scene, impostor: AbstractMesh, color: Color3) {
     const sdfFontDefinition = await (await fetch("https://assets.babylonjs.com/fonts/roboto-regular.json")).text();
     const fontAsset = new FontAsset(sdfFontDefinition, "https://assets.babylonjs.com/fonts/roboto-regular.png");
 
     const textRenderer = await TextRenderer.CreateTextRendererAsync(fontAsset, scene.getEngine());
-    textRenderer.addParagraph("#" + index, {}, Matrix.Scaling(0.5, 0.5, 0.5).multiply(Matrix.Translation(0, 1, 0)));
+    textRenderer.addParagraph("#" + id, {}, Matrix.Scaling(0.5, 0.5, 0.5).multiply(Matrix.Translation(0, 1, 0)));
     textRenderer.isBillboard = true;
     textRenderer.color = Color4.FromColor3(color, 1.0);
-    textRenderer.render(scene.getViewMatrix(), scene.getProjectionMatrix());
     textRenderer.parent = impostor;
     return textRenderer;
 }
@@ -65,33 +64,32 @@ async function CreateTextRendererAsync(scene: Scene, impostor: AbstractMesh, ind
  * @returns
  */
 export const AttractorComponent: FunctionComponent<AttractorProps> = (props) => {
-    const { attractor, id, impostorScale, impostorMaterial, scene } = props;
+    const { attractor, id, impostorScale, impostorMaterial, impostorColor, scene, onControl, isControlled } = props;
     const classes = useAttractorStyles();
     const [shown, setShown] = useState(true);
 
-    // Create observer and cleanup on unmount (we can't use useResource since Observer is not an IDisposable)
-    const sceneOnAfterRenderObserverRef = useRef<Observer<Scene>>();
-    useEffect(() => () => sceneOnAfterRenderObserverRef.current?.remove(), []);
-
     // We only want to recreate the impostor mesh and associated if id, scene, or attractor/impostor changes
     const impostor = useResource(useCallback(() => CreateImpostor(id, scene, attractor, impostorScale, impostorMaterial), [id, scene, attractor]));
-    const label = useAsyncResource(useCallback(async () => await CreateTextRendererAsync(scene, impostor, id, props.impostorColor), [scene, impostor, id]));
+    const label = useAsyncResource(useCallback(async () => await CreateTextRendererAsync(id, scene, impostor, impostorColor), [id, scene, impostor]));
 
     // If impostor, color, or label change, recreate the observer function so that it isnt hooked to old state
     useEffect(() => {
-        sceneOnAfterRenderObserverRef.current?.remove();
-        sceneOnAfterRenderObserverRef.current = scene.onAfterRenderObservable.add(() => {
+        const onAfterRender = scene.onAfterRenderObservable.add(() => {
             attractor.position.copyFrom(impostor.position);
             if (label) {
-                label.color = Color4.FromColor3(props.impostorColor);
+                label.color = Color4.FromColor3(impostorColor);
                 label.render(scene.getViewMatrix(), scene.getProjectionMatrix());
             }
         });
-    }, [impostor, label, props.impostorColor]);
+        return () => {
+            onAfterRender.remove();
+        };
+    }, [impostor, scene, label, impostorColor]);
 
+    // If impostor or impostorScale change, update impostor scaling
     useEffect(() => {
         impostor.scaling.setAll(impostorScale);
-    }, [impostorScale]);
+    }, [impostor, impostorScale]);
 
     return (
         <div className={classes.container}>
@@ -109,8 +107,8 @@ export const AttractorComponent: FunctionComponent<AttractorProps> = (props) =>
             <ToggleButton
                 title="Add / remove position gizmo from particle attractor"
                 enabledIcon={ArrowMoveFilled}
-                value={props.isControlled(impostor)}
-                onChange={(control: boolean) => props.onControl(control ? impostor : undefined)}
+                value={isControlled(impostor)}
+                onChange={(control: boolean) => onControl(control ? impostor : undefined)}
             />
         </div>
     );

packages/dev/inspector-v2/src/components/properties/particles/attractorList.tsx

@@ -15,7 +15,6 @@ import { useResource } from "../../../hooks/resourceHooks";
 import { AttractorComponent } from "./attractor";
 type AttractorListProps = {
     scene: Scene;
-    gizmoManager: GizmoManager;
     attractors: Array<Attractor>;
     system: ParticleSystem;
 };
@@ -52,7 +51,7 @@ export const AttractorList: FunctionComponent<AttractorListProps> = (props) => {
 
     // All impostors share a scale and material/color (for now!)
     const [impostorScale, setImpostorScale] = useState(1);
-    const [impostorColor, setImpostorColor] = useState<Color3>(() => Color3.White());
+    const [impostorColor, setImpostorColor] = useState(() => Color3.White());
     const impostorMaterial = useResource(useCallback(() => CreateSharedMaterial(scene, impostorColor), [scene]));
 
     // All impostors share a gizmoManager. controlledImpostor state ensures re-render of children so that their gizmoEnabled toggle is accurate

packages/dev/inspector-v2/src/components/properties/particles/particleSystemProperties.tsx

@@ -1,5 +1,4 @@
 import type { FactorGradient, ColorGradient as Color4Gradient, IValueGradient, ParticleSystem } from "core/index";
-import { GizmoManager } from "core/Gizmos";
 
 import { Color3, Color4 } from "core/Maths/math.color";
 import { useCallback } from "react";
@@ -9,6 +8,7 @@ import { useInterceptObservable } from "../../../hooks/instrumentationHooks";
 import { useObservableState } from "../../../hooks/observableHooks";
 import { Color4GradientList, FactorGradientList } from "shared-ui-components/fluent/hoc/gradientList";
 import { AttractorList } from "./attractorList";
+import { MessageBar } from "shared-ui-components/fluent/primitives/messageBar";
 
 export const ParticleSystemEmissionProperties: FunctionComponent<{ particleSystem: ParticleSystem }> = (props) => {
     const { particleSystem: system } = props;
@@ -72,13 +72,18 @@ export const ParticleSystemColorProperties: FunctionComponent<{ particleSystem:
 
 export const ParticleSystemAttractorProperties: FunctionComponent<{ particleSystem: ParticleSystem }> = (props) => {
     const { particleSystem: system } = props;
-    const gizmoManager = new GizmoManager(system.getScene()!);
 
     const attractors = useParticleSystemProperty(system, "attractors", "property", "addAttractor", "removeAttractor");
+    const scene = system.getScene();
 
     return (
         <>
-            <AttractorList gizmoManager={gizmoManager} attractors={attractors} scene={system.getScene()!} system={system} />
+            {scene ? (
+                <AttractorList attractors={attractors} scene={scene} system={system} />
+            ) : (
+                // Should never get here since sceneExplorer only displays if there is a scene, but adding UX in case that assumption changes in future
+                <MessageBar intent="info" title="No Scene Available" message="Cannot display attractors without a scene" />
+            )}
         </>
     );
 };

packages/dev/inspector-v2/src/hooks/resourceHooks.ts

@@ -1,6 +1,6 @@
 import type { IDisposable } from "core/index";
 
-import { useRef, useEffect } from "react";
+import { useRef, useEffect, useState } from "react";
 
 /**
  * Custom hook to manage a resource with automatic disposal. The resource is created once initially, and recreated
@@ -44,46 +44,41 @@ export function useResource<T extends IDisposable>(factory: () => T): T {
  * @returns The created resource.
  */
 export function useAsyncResource<T extends IDisposable>(factory: (abortSignal: AbortSignal) => Promise<T>): T | undefined {
-    const resourceRef = useRef<T>();
+    const [resource, setResource] = useState<T | undefined>();
     const factoryRef = useRef(factory);
 
     // Update refs to capture latest values
     factoryRef.current = factory;
 
     useEffect(() => {
-        const abortController = new AbortController(); // Create AbortController
-        const currentResource: T | undefined = resourceRef.current;
+        const abortController = new AbortController();
 
         // Dispose old resource if it exists
-        currentResource?.dispose();
-        resourceRef.current = undefined;
+        resource?.dispose();
+        setResource(undefined);
 
         // Create new resource
         void (async () => {
             try {
-                const newVal = await factory(abortController.signal); // Pass the signal
+                const newVal = await factory(abortController.signal);
                 if (!abortController.signal.aborted) {
-                    resourceRef.current = newVal;
+                    setResource(newVal); // This will trigger a re-render so the new resource is returned to caller
                 } else {
                     newVal.dispose();
                 }
             } catch (error) {
-                // Handle abortion gracefully
                 if (error instanceof Error && error.name === "AbortError") {
-                    // Request was aborted, this is expected
                     return;
                 }
-                // Optionally handle other errors here
-                global.console.error("Failed to create async resource:", error);
             }
         })();
 
         return () => {
-            abortController.abort(); // Abort the operation
-            resourceRef.current?.dispose();
-            resourceRef.current = undefined;
+            abortController.abort();
+            resource?.dispose();
+            setResource(undefined);
         };
     }, [factory]);
 
-    return resourceRef.current;
+    return resource;
 }