From 98b27e4b1a4292ae953b7d4816d6bf1d87a3b666 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 25 Sep 2024 14:49:49 -0400 Subject: [PATCH 01/30] Bump strawberry-graphql from 0.138.1 to 0.243.0 in /requirements (#4842) Bumps [strawberry-graphql](https://github.com/strawberry-graphql/strawberry) from 0.138.1 to 0.243.0. - [Release notes](https://github.com/strawberry-graphql/strawberry/releases) - [Changelog](https://github.com/strawberry-graphql/strawberry/blob/main/CHANGELOG.md) - [Commits](https://github.com/strawberry-graphql/strawberry/compare/0.138.1...0.243.0) --- updated-dependencies: - dependency-name: strawberry-graphql dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- requirements/common.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/common.txt b/requirements/common.txt index ee6a2d44cf..b0a2a3b338 100644 --- a/requirements/common.txt +++ b/requirements/common.txt @@ -28,6 +28,6 @@ setuptools>=45.2.0,<71 sseclient-py>=1.7.2 sse-starlette>=0.10.3 starlette==0.36.2 -strawberry-graphql==0.138.1 +strawberry-graphql==0.243.0 tabulate==0.8.10 xmltodict==0.12.0 From 4845d52c4fc20a92bed5872a97ecb0cdcf469247 Mon Sep 17 00:00:00 2001 From: Sashank Aryal <66688606+sashankaryal@users.noreply.github.com> Date: Wed, 25 Sep 2024 14:24:12 -0500 Subject: [PATCH 02/30] add flags that allow both pcd and ply geometries to be centered (#4843) * center geometry by default for ply and not for pcd * add docs --- app/packages/looker-3d/src/fo3d/mesh/Ply.tsx | 15 ++++++++++++--- .../looker-3d/src/fo3d/point-cloud/Pcd.tsx | 13 ++++++------- app/packages/looker-3d/src/hooks/use-fo3d.ts | 12 ++++++++---- fiftyone/core/threed/mesh.py | 10 +++++++++- fiftyone/core/threed/pointcloud.py | 5 +++++ 5 files changed, 40 insertions(+), 15 deletions(-) diff --git a/app/packages/looker-3d/src/fo3d/mesh/Ply.tsx b/app/packages/looker-3d/src/fo3d/mesh/Ply.tsx index 2c6a2e0f0c..7af96ea9d3 100644 --- a/app/packages/looker-3d/src/fo3d/mesh/Ply.tsx +++ b/app/packages/looker-3d/src/fo3d/mesh/Ply.tsx @@ -124,7 +124,13 @@ const PlyWithNoMaterialOverride = ({ export const Ply = ({ name, - ply: { plyPath, preTransformedPlyPath, defaultMaterial, isPcd }, + ply: { + plyPath, + preTransformedPlyPath, + defaultMaterial, + isPcd, + centerGeometry, + }, position, quaternion, scale, @@ -161,7 +167,10 @@ export const Ply = ({ !geometry.attributes.normal?.count ) { geometry.computeVertexNormals(); - geometry.center(); + + if (centerGeometry) { + geometry.center(); + } } if (geometry.attributes?.color?.count) { @@ -169,7 +178,7 @@ export const Ply = ({ } setIsGeometryResolved(true); - }, [geometry]); + }, [geometry, centerGeometry]); const mesh = useMemo(() => { if (!isGeometryResolved) { diff --git a/app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx b/app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx index 7192d82caf..7427d3d316 100644 --- a/app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx +++ b/app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx @@ -10,7 +10,7 @@ import { usePcdMaterial } from "./use-pcd-material"; export const Pcd = ({ name, - pcd: { pcdPath, preTransformedPcdPath, defaultMaterial }, + pcd: { pcdPath, preTransformedPcdPath, defaultMaterial, centerGeometry }, position, quaternion, scale, @@ -37,12 +37,11 @@ export const Pcd = ({ // todo: hack until https://github.com/pmndrs/react-three-fiber/issues/245 is fixed const points = useMemo(() => points_.clone(false), [points_]); - // todo: expose centering of points as an opt-in behavior from the sdk - // useEffect(() => { - // if (points) { - // points.geometry.center(); - // } - // }, [points]); + useEffect(() => { + if (points && centerGeometry) { + points.geometry.center(); + } + }, [points, centerGeometry]); const pcdContainerRef = useRef(); diff --git a/app/packages/looker-3d/src/hooks/use-fo3d.ts b/app/packages/looker-3d/src/hooks/use-fo3d.ts index 308fc921bc..0ae68e1de7 100644 --- a/app/packages/looker-3d/src/hooks/use-fo3d.ts +++ b/app/packages/looker-3d/src/hooks/use-fo3d.ts @@ -94,7 +94,8 @@ export class PcdAsset { constructor( readonly pcdPath?: string, readonly preTransformedPcdPath?: string, - readonly defaultMaterial?: FoPointcloudMaterialProps + readonly defaultMaterial?: FoPointcloudMaterialProps, + readonly centerGeometry?: boolean ) {} } @@ -103,7 +104,8 @@ export class PlyAsset { readonly plyPath?: string, readonly preTransformedPlyPath?: string, readonly defaultMaterial?: FoMeshMaterial, - readonly isPcd?: boolean + readonly isPcd?: boolean, + readonly centerGeometry?: boolean ) {} } @@ -306,7 +308,8 @@ export const useFo3d = ( node["plyPath"], node["preTransformedPlyPath"], material as FoMeshMaterial, - node["isPointCloud"] ?? false + node["isPointCloud"] ?? false, + node["centerGeometry"] ?? true ); } } @@ -315,7 +318,8 @@ export const useFo3d = ( asset = new PcdAsset( node["pcdPath"], node["preTransformedPcdPath"], - material as FoPointcloudMaterialProps + material as FoPointcloudMaterialProps, + node["centerGeometry"] ?? false ); } } else if (node["_type"].endsWith("Geometry")) { diff --git a/fiftyone/core/threed/mesh.py b/fiftyone/core/threed/mesh.py index 9cb0b4eb12..dd34493b39 100644 --- a/fiftyone/core/threed/mesh.py +++ b/fiftyone/core/threed/mesh.py @@ -269,6 +269,8 @@ class PlyMesh(Mesh): absolute or relative to the directory containing the ``.fo3d`` file is_point_cloud (bool): whether the PLY file is a point cloud. Defaults to ``False`` + center_geometry (bool): whether to center the geometry. Defaults to + ``True`` material (:class:`fiftyone.core.threed.MeshMaterial`, optional): default material for the mesh if PLY file does not contain vertex colors. Defaults to @@ -291,6 +293,7 @@ def __init__( name: str, ply_path: str, is_point_cloud: bool = False, + center_geometry: bool = True, default_material: Optional[MeshMaterial] = None, visible=True, position: Optional[Vec3UnionType] = None, @@ -309,13 +312,18 @@ def __init__( if not ply_path.lower().endswith(".ply"): raise ValueError("PLY mesh must be a .ply file") + self.center_geometry = center_geometry self.ply_path = ply_path self.is_point_cloud = is_point_cloud def _to_dict_extra(self): r = { **super()._to_dict_extra(), - **{"plyPath": self.ply_path, "isPointCloud": self.is_point_cloud}, + **{ + "centerGeometry": self.center_geometry, + "plyPath": self.ply_path, + "isPointCloud": self.is_point_cloud, + }, } if hasattr(self, "_pre_transformed_ply_path"): diff --git a/fiftyone/core/threed/pointcloud.py b/fiftyone/core/threed/pointcloud.py index a84b94fddc..ecd35abe08 100644 --- a/fiftyone/core/threed/pointcloud.py +++ b/fiftyone/core/threed/pointcloud.py @@ -24,6 +24,8 @@ class PointCloud(Object3D): the material of the point cloud. If not specified, defaults to a new instance of :class:`fiftyone.core.threed.PointCloudMaterial` with its default parameters + center_geometry (bool): whether to center the geometry of the point + cloud. Defaults to ``False`` flag_for_projection (bool): whether to flag the point cloud for usage in orthographic projection. Each :class:`fiftyone.core.threed.Scene` can have at most one asset @@ -45,6 +47,7 @@ def __init__( name: str, pcd_path: str, material: Optional[PointCloudMaterial] = None, + center_geometry: bool = False, flag_for_projection: bool = False, visible=True, position: Optional[Vec3UnionType] = None, @@ -67,6 +70,7 @@ def __init__( if isinstance(material, dict): material = PointCloudMaterial._from_dict(material) + self.center_geometry = center_geometry self.default_material = material or PointCloudMaterial() self.flag_for_projection = flag_for_projection @@ -84,6 +88,7 @@ def set_default_material(self, material: PointCloudMaterial): def _to_dict_extra(self): """Extra properties to include in dictionary representation.""" r = { + "centerGeometry": self.center_geometry, "pcdPath": self.pcd_path, "defaultMaterial": self.default_material.as_dict(), "flagForProjection": self.flag_for_projection, From fc22e3a3da47baeb6dd76b5ce7916797a29b1e0e Mon Sep 17 00:00:00 2001 From: imanjra Date: Wed, 25 Sep 2024 17:21:33 -0400 Subject: [PATCH 03/30] add set_group_slice operator --- .../operators/src/built-in-operators.ts | 24 +++++++++++++++++++ fiftyone/operators/builtin.py | 15 ++++++++++-- fiftyone/operators/operations.py | 8 +++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/app/packages/operators/src/built-in-operators.ts b/app/packages/operators/src/built-in-operators.ts index 610baab04b..7d7560279e 100644 --- a/app/packages/operators/src/built-in-operators.ts +++ b/app/packages/operators/src/built-in-operators.ts @@ -1225,6 +1225,29 @@ export class ApplyPanelStatePath extends Operator { } } +export class SetGroupSlice extends Operator { + get config(): OperatorConfig { + return new OperatorConfig({ + name: "set_group_slice", + label: "Set group slice", + // unlisted: true, + }); + } + useHooks() { + const setSlice = fos.useSetGroupSlice(); + return { setSlice }; + } + async resolveInput(): Promise { + const inputs = new types.Object(); + inputs.str("slice", { label: "Group slice", required: true }); + return new types.Property(inputs); + } + async execute(ctx: ExecutionContext): Promise { + const { slice } = ctx.params; + ctx.hooks.setSlice(slice); + } +} + export function registerBuiltInOperators() { try { _registerBuiltInOperator(CopyViewAsJSON); @@ -1271,6 +1294,7 @@ export function registerBuiltInOperators() { _registerBuiltInOperator(TrackEvent); _registerBuiltInOperator(SetPanelTitle); _registerBuiltInOperator(ApplyPanelStatePath); + _registerBuiltInOperator(SetGroupSlice); } catch (e) { console.error("Error registering built-in operators"); console.error(e); diff --git a/fiftyone/operators/builtin.py b/fiftyone/operators/builtin.py index 9b0d0aa00f..22ebd6ee8c 100644 --- a/fiftyone/operators/builtin.py +++ b/fiftyone/operators/builtin.py @@ -1518,7 +1518,8 @@ def execute(self, ctx): new_name = ctx.params["new_name"] ctx.dataset.rename_group_slice(name, new_name) - ctx.trigger("reload_dataset") + ctx.ops.set_group_slice(new_name) + ctx.ops.reload_dataset() class DeleteGroupSlice(foo.Operator): @@ -1564,7 +1565,17 @@ def execute(self, ctx): name = ctx.params["name"] ctx.dataset.delete_group_slice(name) - ctx.trigger("reload_dataset") + group_slices = ctx.dataset.group_slices + selected_group_slice = next( + ( + group_slice + for group_slice in group_slices + if group_slice != name + ), + None, + ) + ctx.ops.set_group_slice(selected_group_slice) + ctx.ops.reload_dataset() class ListSavedViews(foo.Operator): diff --git a/fiftyone/operators/operations.py b/fiftyone/operators/operations.py index 1f1a8673a7..98f9ab735b 100644 --- a/fiftyone/operators/operations.py +++ b/fiftyone/operators/operations.py @@ -629,6 +629,14 @@ def set_panel_title(self, id=None, title=None): "set_panel_title", params={"id": id, "title": title} ) + def set_group_slice(self, slice): + """Set the group slices in the App. + + Args: + slice: the group slice to set + """ + return self._ctx.trigger("set_group_slice", {"slice": slice}) + def _serialize_view(view): return json.loads(json_util.dumps(view._serialize())) From cbb8b159a9c7b02ab83efe383e07df19bf3146b0 Mon Sep 17 00:00:00 2001 From: brimoor Date: Wed, 25 Sep 2024 18:09:00 -0400 Subject: [PATCH 04/30] use default_group_slice --- fiftyone/core/dataset.py | 5 ++++- fiftyone/operators/builtin.py | 21 ++++++++++----------- fiftyone/operators/operations.py | 4 ++-- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/fiftyone/core/dataset.py b/fiftyone/core/dataset.py index 9ac678dc77..6ff45f8927 100644 --- a/fiftyone/core/dataset.py +++ b/fiftyone/core/dataset.py @@ -573,7 +573,10 @@ def group_slice(self, slice_name): if slice_name is None: slice_name = self._doc.default_group_slice - if slice_name not in self._doc.group_media_types: + if ( + slice_name is not None + and slice_name not in self._doc.group_media_types + ): raise ValueError("Dataset has no group slice '%s'" % slice_name) self._group_slice = slice_name diff --git a/fiftyone/operators/builtin.py b/fiftyone/operators/builtin.py index 22ebd6ee8c..d879405b5b 100644 --- a/fiftyone/operators/builtin.py +++ b/fiftyone/operators/builtin.py @@ -1518,7 +1518,11 @@ def execute(self, ctx): new_name = ctx.params["new_name"] ctx.dataset.rename_group_slice(name, new_name) - ctx.ops.set_group_slice(new_name) + + # @todo ideally we would only set this if the group slice we renamed is + # currently loaded in the App + ctx.ops.set_group_slice(ctx.dataset.default_group_slice) + ctx.ops.reload_dataset() @@ -1565,16 +1569,11 @@ def execute(self, ctx): name = ctx.params["name"] ctx.dataset.delete_group_slice(name) - group_slices = ctx.dataset.group_slices - selected_group_slice = next( - ( - group_slice - for group_slice in group_slices - if group_slice != name - ), - None, - ) - ctx.ops.set_group_slice(selected_group_slice) + + # @todo ideally we would only set this if the group slice we deleted is + # currently loaded in the App + ctx.ops.set_group_slice(ctx.dataset.default_group_slice) + ctx.ops.reload_dataset() diff --git a/fiftyone/operators/operations.py b/fiftyone/operators/operations.py index 98f9ab735b..af452b5d0e 100644 --- a/fiftyone/operators/operations.py +++ b/fiftyone/operators/operations.py @@ -630,10 +630,10 @@ def set_panel_title(self, id=None, title=None): ) def set_group_slice(self, slice): - """Set the group slices in the App. + """Set the active group slice in the App. Args: - slice: the group slice to set + slice: the group slice to activate """ return self._ctx.trigger("set_group_slice", {"slice": slice}) From 6a358a03fffe5f0899aaefd65b36d271cae8ded6 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 25 Sep 2024 16:45:46 -0500 Subject: [PATCH 05/30] add reload_on_navigation --- app/packages/operators/src/Panel/register.tsx | 1 + app/packages/plugins/src/index.ts | 6 ++++++ app/packages/spaces/src/components/Panel.tsx | 17 +++++++++++++---- fiftyone/operators/operations.py | 5 +++++ fiftyone/operators/panel.py | 10 ++++++++++ 5 files changed, 35 insertions(+), 4 deletions(-) diff --git a/app/packages/operators/src/Panel/register.tsx b/app/packages/operators/src/Panel/register.tsx index 24a8fa973d..01e4522690 100644 --- a/app/packages/operators/src/Panel/register.tsx +++ b/app/packages/operators/src/Panel/register.tsx @@ -23,6 +23,7 @@ export default function registerPanel(ctx: ExecutionContext) { panelOptions: { allowDuplicates: ctx.params.allow_duplicates, helpMarkdown: ctx.params.help_markdown, + reloadOnNavigation: ctx.params.reload_on_navigation, surfaces: ctx.params.surfaces, }, }); diff --git a/app/packages/plugins/src/index.ts b/app/packages/plugins/src/index.ts index cd527a74c8..a14e1423e4 100644 --- a/app/packages/plugins/src/index.ts +++ b/app/packages/plugins/src/index.ts @@ -325,6 +325,12 @@ type PanelOptions = { * Content displayed on the right side of the label in the panel title bar. */ TabIndicator?: React.ComponentType; + + /** + * If true, the plugin will be remounted when the user navigates to a different sample or group. + * This is only applicable to plugins that are in a modal. + */ + reloadOnNavigation?: boolean; }; type PluginComponentProps = T & { diff --git a/app/packages/spaces/src/components/Panel.tsx b/app/packages/spaces/src/components/Panel.tsx index 6a2fc56d1e..3b8c9859ee 100644 --- a/app/packages/spaces/src/components/Panel.tsx +++ b/app/packages/spaces/src/components/Panel.tsx @@ -1,7 +1,7 @@ import { CenteredStack, scrollable } from "@fiftyone/components"; import * as fos from "@fiftyone/state"; import React, { useEffect } from "react"; -import { useSetRecoilState } from "recoil"; +import { useRecoilValue, useSetRecoilState } from "recoil"; import { PANEL_LOADING_TIMEOUT } from "../constants"; import { PanelContext } from "../contexts"; import { useReactivePanel } from "../hooks"; @@ -20,14 +20,17 @@ function Panel(props: PanelProps) { const setPanelIdToScope = useSetRecoilState(panelIdToScopeAtom); const scope = isModalPanel ? "modal" : "grid"; + const thisModalUniqueId = useRecoilValue(fos.currentModalUniqueId); + useEffect(() => { setPanelIdToScope((ids) => ({ ...ids, [node.id]: scope })); }, [scope, setPanelIdToScope, node.id]); const panelContentTestId = `panel-content-${panelName}`; + if (!panel) { return ( - + {pending ? ( @@ -39,7 +42,9 @@ function Panel(props: PanelProps) { ); } - const { component: Component } = panel; + const { component: Component, panelOptions } = panel; + + const shouldKeyComponent = isModalPanel && panelOptions?.reloadOnNavigation; return ( - + ); diff --git a/fiftyone/operators/operations.py b/fiftyone/operators/operations.py index 1f1a8673a7..2cd150e19d 100644 --- a/fiftyone/operators/operations.py +++ b/fiftyone/operators/operations.py @@ -305,6 +305,7 @@ def register_panel( light_icon=None, dark_icon=None, surfaces="grid", + reload_on_navigation=False, on_load=None, on_unload=None, on_change=None, @@ -331,6 +332,9 @@ def register_panel( is in dark mode surfaces ('grid'): surfaces in which to show the panel. Must be one of 'grid', 'modal', or 'grid modal' + reload_on_navigation (False): whether to reload the panel when the + user navigates to a new page. This is only applicable to panels + that are not shown in a modal on_load (None): an operator to invoke when the panel is loaded on_unload (None): an operator to invoke when the panel is unloaded on_change (None): an operator to invoke when the panel state @@ -360,6 +364,7 @@ def register_panel( "light_icon": light_icon, "dark_icon": dark_icon, "surfaces": surfaces, + "reload_on_navigation": reload_on_navigation, "on_load": on_load, "on_unload": on_unload, "on_change": on_change, diff --git a/fiftyone/operators/panel.py b/fiftyone/operators/panel.py index 8f645d9b9d..c2a032ec51 100644 --- a/fiftyone/operators/panel.py +++ b/fiftyone/operators/panel.py @@ -28,6 +28,12 @@ class PanelConfig(OperatorConfig): in dark mode allow_multiple (False): whether to allow multiple instances of the panel to be opened + reload_on_navigation (False): whether to reload the panel when the + user navigates to a new page. This is only applicable to panels + that are not shown in a modal + surfaces ("grid"): the surfaces on which the panel can be displayed + help_markdown (None): a markdown string to display in the panel's help + tooltip """ def __init__( @@ -40,6 +46,7 @@ def __init__( dark_icon=None, allow_multiple=False, surfaces: PANEL_SURFACE = "grid", + reload_on_navigation=False, **kwargs ): super().__init__(name) @@ -52,6 +59,7 @@ def __init__( self.allow_multiple = allow_multiple self.unlisted = True self.on_startup = True + self.reload_on_navigation = reload_on_navigation self.surfaces = surfaces self.kwargs = kwargs # unused, placeholder for future extensibility @@ -66,6 +74,7 @@ def to_json(self): "allow_multiple": self.allow_multiple, "on_startup": self.on_startup, "unlisted": self.unlisted, + "reload_on_navigation": self.reload_on_navigation, "surfaces": self.surfaces, } @@ -105,6 +114,7 @@ def on_startup(self, ctx): "dark_icon": self.config.dark_icon, "light_icon": self.config.light_icon, "surfaces": self.config.surfaces, + "reload_on_navigation": self.config.reload_on_navigation, } methods = ["on_load", "on_unload", "on_change"] ctx_change_events = [ From a84d41ed5f43db2b3c1aa1175747dc3c8453ad73 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 25 Sep 2024 16:48:48 -0500 Subject: [PATCH 06/30] optional chaining for group id --- app/packages/state/src/recoil/groups.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/packages/state/src/recoil/groups.ts b/app/packages/state/src/recoil/groups.ts index 080ccc9a52..3f426f9cc0 100644 --- a/app/packages/state/src/recoil/groups.ts +++ b/app/packages/state/src/recoil/groups.ts @@ -389,7 +389,7 @@ export const groupField = selector({ export const groupId = selector({ key: "groupId", - get: ({ get }) => get(modalSelector).groupId || null, + get: ({ get }) => get(modalSelector)?.groupId || null, }); export const refreshGroupQuery = atom({ From f8a2f341e5ce055defc0dba550a85b0e6f132be6 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 25 Sep 2024 14:38:21 -0700 Subject: [PATCH 07/30] add timeline ops --- .../operators/src/built-in-operators.ts | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/app/packages/operators/src/built-in-operators.ts b/app/packages/operators/src/built-in-operators.ts index 610baab04b..bc453c9f73 100644 --- a/app/packages/operators/src/built-in-operators.ts +++ b/app/packages/operators/src/built-in-operators.ts @@ -31,6 +31,8 @@ import { } from "./operators"; import { useShowOperatorIO } from "./state"; import usePanelEvent from "./usePanelEvent"; +import { useAtomValue } from "jotai"; +import * as fop from "@fiftyone/playback"; // // BUILT-IN OPERATORS @@ -1211,6 +1213,56 @@ export class SetPanelTitle extends Operator { } } +type SetPlayheadStateHooks = { + setPlayheadState: ReturnType; +}; +type SetPlayheadStateParams = { state: any; timeline_name?: string }; +export class SetPlayheadState extends Operator { + get config(): OperatorConfig { + return new OperatorConfig({ + name: "set_playhead_state", + label: "Set playhead state", + unlisted: true, + }); + } + useHooks(): SetPlayheadStateHooks { + const timeline = fop.useTimeline(); + return { + setPlayheadState: (state: fop.PlayheadState) => { + timeline.setPlayHeadState(state); + }, + }; + } + async execute({ hooks, params }: ExecutionContext): Promise { + const { setPlayheadState } = hooks as SetPlayheadStateHooks; + const { state, timeline_name } = params as SetPlayheadStateParams; + setPlayheadState(state, timeline_name); + } +} + +type SetFrameNumberParams = { timeline_name?: string; frame_number: number }; +class SetFrameNumber extends Operator { + get config(): OperatorConfig { + return new OperatorConfig({ + name: "set_frame_number", + label: "Set frame number", + }); + } + async resolveInput(): Promise { + const inputs = new types.Object(); + inputs.str("timeline_name", { label: "Timeline name" }); + inputs.int("frame_number", { label: "Frame number", required: true }); + return new types.Property(inputs); + } + async execute(ctx: ExecutionContext): Promise { + const { frame_number, timeline_name } = ctx.params as SetFrameNumberParams; + fop.dispatchTimelineSetFrameNumberEvent({ + timelineName: timeline_name, + newFrameNumber: frame_number, + }); + } +} + export class ApplyPanelStatePath extends Operator { get config(): OperatorConfig { return new OperatorConfig({ @@ -1271,6 +1323,8 @@ export function registerBuiltInOperators() { _registerBuiltInOperator(TrackEvent); _registerBuiltInOperator(SetPanelTitle); _registerBuiltInOperator(ApplyPanelStatePath); + _registerBuiltInOperator(SetPlayheadState); + _registerBuiltInOperator(SetFrameNumber); } catch (e) { console.error("Error registering built-in operators"); console.error(e); From 353772eeffc0a47bbec53e6f01e9f85d2a01ee19 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 25 Sep 2024 14:40:05 -0700 Subject: [PATCH 08/30] cleanup imports --- app/packages/operators/src/built-in-operators.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/packages/operators/src/built-in-operators.ts b/app/packages/operators/src/built-in-operators.ts index bc453c9f73..a974d95424 100644 --- a/app/packages/operators/src/built-in-operators.ts +++ b/app/packages/operators/src/built-in-operators.ts @@ -12,6 +12,7 @@ import * as types from "./types"; import { useTrackEvent } from "@fiftyone/analytics"; import { setPathUserUnchanged } from "@fiftyone/core/src/plugins/SchemaIO/hooks"; +import * as fop from "@fiftyone/playback"; import { LOAD_WORKSPACE_OPERATOR } from "@fiftyone/spaces/src/components/Workspaces/constants"; import { toSlug } from "@fiftyone/utilities"; import copyToClipboard from "copy-to-clipboard"; @@ -31,8 +32,6 @@ import { } from "./operators"; import { useShowOperatorIO } from "./state"; import usePanelEvent from "./usePanelEvent"; -import { useAtomValue } from "jotai"; -import * as fop from "@fiftyone/playback"; // // BUILT-IN OPERATORS From 087d8ccd8497dff1ea8102f90fe8f03d1b38be18 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 25 Sep 2024 14:41:08 -0700 Subject: [PATCH 09/30] fix types --- app/packages/operators/src/built-in-operators.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/packages/operators/src/built-in-operators.ts b/app/packages/operators/src/built-in-operators.ts index a974d95424..d178504b44 100644 --- a/app/packages/operators/src/built-in-operators.ts +++ b/app/packages/operators/src/built-in-operators.ts @@ -1213,7 +1213,7 @@ export class SetPanelTitle extends Operator { } type SetPlayheadStateHooks = { - setPlayheadState: ReturnType; + setPlayheadState: (state: fop.PlayheadState, timeline_name?: string) => void; }; type SetPlayheadStateParams = { state: any; timeline_name?: string }; export class SetPlayheadState extends Operator { From 8fb25bddcf49c903b12526b785ec203c74d7f712 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 25 Sep 2024 14:53:59 -0700 Subject: [PATCH 10/30] fix timeline ops --- app/packages/operators/src/built-in-operators.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/app/packages/operators/src/built-in-operators.ts b/app/packages/operators/src/built-in-operators.ts index d178504b44..df9cbe2bc3 100644 --- a/app/packages/operators/src/built-in-operators.ts +++ b/app/packages/operators/src/built-in-operators.ts @@ -1224,8 +1224,14 @@ export class SetPlayheadState extends Operator { unlisted: true, }); } - useHooks(): SetPlayheadStateHooks { - const timeline = fop.useTimeline(); + async resolveInput(): Promise { + const inputs = new types.Object(); + inputs.enum("state", ["playing", "paused"], { label: "State" }); + inputs.str("timeline_name", { label: "Timeline name" }); + return new types.Property(inputs); + } + useHooks(ctx: ExecutionContext): SetPlayheadStateHooks { + const timeline = fop.useTimeline(ctx.params.timeline_name); return { setPlayheadState: (state: fop.PlayheadState) => { timeline.setPlayHeadState(state); @@ -1235,7 +1241,7 @@ export class SetPlayheadState extends Operator { async execute({ hooks, params }: ExecutionContext): Promise { const { setPlayheadState } = hooks as SetPlayheadStateHooks; const { state, timeline_name } = params as SetPlayheadStateParams; - setPlayheadState(state, timeline_name); + setPlayheadState(state); } } From 52e84fcdb93641c201462c0b6e3a55cba884490c Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 25 Sep 2024 14:56:28 -0700 Subject: [PATCH 11/30] more timeline ops fixes --- app/packages/operators/src/built-in-operators.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/packages/operators/src/built-in-operators.ts b/app/packages/operators/src/built-in-operators.ts index df9cbe2bc3..0ea77b4618 100644 --- a/app/packages/operators/src/built-in-operators.ts +++ b/app/packages/operators/src/built-in-operators.ts @@ -1240,7 +1240,7 @@ export class SetPlayheadState extends Operator { } async execute({ hooks, params }: ExecutionContext): Promise { const { setPlayheadState } = hooks as SetPlayheadStateHooks; - const { state, timeline_name } = params as SetPlayheadStateParams; + const { state } = params as SetPlayheadStateParams; setPlayheadState(state); } } @@ -1256,7 +1256,11 @@ class SetFrameNumber extends Operator { async resolveInput(): Promise { const inputs = new types.Object(); inputs.str("timeline_name", { label: "Timeline name" }); - inputs.int("frame_number", { label: "Frame number", required: true }); + inputs.int("frame_number", { + label: "Frame number", + required: true, + min: 0, + }); return new types.Property(inputs); } async execute(ctx: ExecutionContext): Promise { From db50f5ddc13363daf812bf1b23d1c19523b65c30 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 25 Sep 2024 14:57:45 -0700 Subject: [PATCH 12/30] timeline ops cleanup --- app/packages/operators/src/built-in-operators.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/packages/operators/src/built-in-operators.ts b/app/packages/operators/src/built-in-operators.ts index 0ea77b4618..2999b754e7 100644 --- a/app/packages/operators/src/built-in-operators.ts +++ b/app/packages/operators/src/built-in-operators.ts @@ -1215,7 +1215,11 @@ export class SetPanelTitle extends Operator { type SetPlayheadStateHooks = { setPlayheadState: (state: fop.PlayheadState, timeline_name?: string) => void; }; -type SetPlayheadStateParams = { state: any; timeline_name?: string }; +type SetPlayheadStateParams = { + state: fop.PlayheadState; + timeline_name?: string; +}; + export class SetPlayheadState extends Operator { get config(): OperatorConfig { return new OperatorConfig({ From 52403e4b5fdfa207f2c94003c2e44623beb4df50 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 25 Sep 2024 17:26:31 -0500 Subject: [PATCH 13/30] add docs on panel surfaces --- docs/source/plugins/developing_plugins.rst | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/source/plugins/developing_plugins.rst b/docs/source/plugins/developing_plugins.rst index f5bd36f141..8be31afcac 100644 --- a/docs/source/plugins/developing_plugins.rst +++ b/docs/source/plugins/developing_plugins.rst @@ -1773,6 +1773,12 @@ in the App. Panels can be defined in either Python or JS, and FiftyOne comes with a number of :ref:`builtin panels ` for common tasks. +Depending on the ``surfaces`` panel config, panels can be scoped to either +the grid or the modal, and can be opened from the "+" menu - available both +in the grid and the modal. Whereas grid panels unlock extensibility in the +macro level of datasets, modal panels unlock extensibility in the micro level +of samples. + Panels, like :ref:`operators `, can make use of the :mod:`fiftyone.operators.types` module and the :js:mod:`@fiftyone/operators <@fiftyone/operators>` package, which define a @@ -1829,6 +1835,15 @@ subsequent sections. # Whether to allow multiple instances of the panel to be opened allow_multiple=False, + + # Whether the panel should be made available in the grid, or + # the modal, or both (default = grid only) + surfaces="grid modal" + + # Markdown-formatted text that describes the panel. This is + # rendererd in a tooltip when the help icon in the panel + # title is hovered over + help_markdown="A description of the panel", ) def render(self, ctx): From 55e26f08a1781f4be831a46761f3b5ca159b81bc Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 25 Sep 2024 17:31:13 -0500 Subject: [PATCH 14/30] clarify modal panels doc --- docs/source/plugins/developing_plugins.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/source/plugins/developing_plugins.rst b/docs/source/plugins/developing_plugins.rst index 8be31afcac..24c66d30c7 100644 --- a/docs/source/plugins/developing_plugins.rst +++ b/docs/source/plugins/developing_plugins.rst @@ -1774,10 +1774,11 @@ Panels can be defined in either Python or JS, and FiftyOne comes with a number of :ref:`builtin panels ` for common tasks. Depending on the ``surfaces`` panel config, panels can be scoped to either -the grid or the modal, and can be opened from the "+" menu - available both -in the grid and the modal. Whereas grid panels unlock extensibility in the -macro level of datasets, modal panels unlock extensibility in the micro level -of samples. +the grid or the modal. You can open these panels from the "+" menu, which +is available in both the grid and modal views. Whereas grid panels enable +extensibility at the macro level, allowing you to work with entire datasets, +modal panels provide extensibility at the micro level, focusing on individual +samples and scenarios. Panels, like :ref:`operators `, can make use of the :mod:`fiftyone.operators.types` module and the From 324a0cf29f1ec01add5e245d6f483cf6f71ba71f Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 25 Sep 2024 17:33:00 -0500 Subject: [PATCH 15/30] explicate all possible values --- docs/source/plugins/developing_plugins.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/source/plugins/developing_plugins.rst b/docs/source/plugins/developing_plugins.rst index 24c66d30c7..3032689bdc 100644 --- a/docs/source/plugins/developing_plugins.rst +++ b/docs/source/plugins/developing_plugins.rst @@ -1837,9 +1837,10 @@ subsequent sections. # Whether to allow multiple instances of the panel to be opened allow_multiple=False, - # Whether the panel should be made available in the grid, or - # the modal, or both (default = grid only) - surfaces="grid modal" + # Whether the panel should be available in the grid view + # modal view, or both + # Possible values: "grid", "modal", "grid modal" + surfaces="grid modal" # default = "grid" # Markdown-formatted text that describes the panel. This is # rendererd in a tooltip when the help icon in the panel From 6ea76d61e42874e391a17d1067d7e8bc932346e1 Mon Sep 17 00:00:00 2001 From: imanjra Date: Thu, 26 Sep 2024 10:20:24 -0400 Subject: [PATCH 16/30] add group slice in operator context --- app/packages/operators/src/CustomPanel.tsx | 2 ++ app/packages/operators/src/hooks.ts | 3 +++ app/packages/operators/src/operators.ts | 9 +++++++++ app/packages/operators/src/state.ts | 5 +++++ app/packages/operators/src/useCustomPanelHooks.ts | 7 +++++++ docs/source/plugins/developing_plugins.rst | 13 +++++++++++++ fiftyone/operators/executor.py | 5 +++++ fiftyone/operators/operations.py | 3 +++ fiftyone/operators/panel.py | 1 + 9 files changed, 48 insertions(+) diff --git a/app/packages/operators/src/CustomPanel.tsx b/app/packages/operators/src/CustomPanel.tsx index 8c7c1e07e9..cf9f82a5b3 100644 --- a/app/packages/operators/src/CustomPanel.tsx +++ b/app/packages/operators/src/CustomPanel.tsx @@ -114,6 +114,7 @@ export function defineCustomPanel({ on_change_selected, on_change_selected_labels, on_change_extended_selection, + on_change_group_slice, panel_name, panel_label, }) { @@ -130,6 +131,7 @@ export function defineCustomPanel({ onChangeSelected={on_change_selected} onChangeSelectedLabels={on_change_selected_labels} onChangeExtendedSelection={on_change_extended_selection} + onChangeGroupSlice={on_change_group_slice} dimensions={dimensions} panelName={panel_name} panelLabel={panel_label} diff --git a/app/packages/operators/src/hooks.ts b/app/packages/operators/src/hooks.ts index 5eac79790c..a636f1b263 100644 --- a/app/packages/operators/src/hooks.ts +++ b/app/packages/operators/src/hooks.ts @@ -25,6 +25,7 @@ function useOperatorThrottledContextSetter() { const filters = useRecoilValue(fos.filters); const selectedSamples = useRecoilValue(fos.selectedSamples); const selectedLabels = useRecoilValue(fos.selectedLabels); + const groupSlice = useRecoilValue(fos.groupSlice); const currentSample = useCurrentSample(); const setContext = useSetRecoilState(operatorThrottledContext); const setThrottledContext = useMemo(() => { @@ -47,6 +48,7 @@ function useOperatorThrottledContextSetter() { selectedLabels, currentSample, viewName, + groupSlice, }); }, [ setThrottledContext, @@ -58,6 +60,7 @@ function useOperatorThrottledContextSetter() { selectedLabels, currentSample, viewName, + groupSlice, ]); } diff --git a/app/packages/operators/src/operators.ts b/app/packages/operators/src/operators.ts index 621554d47c..f11aa3d880 100644 --- a/app/packages/operators/src/operators.ts +++ b/app/packages/operators/src/operators.ts @@ -90,6 +90,7 @@ export type RawContext = { selection: string[] | null; scope: string; }; + groupSlice: string; }; export class ExecutionContext { @@ -132,6 +133,9 @@ export class ExecutionContext { public get extendedSelection(): any { return this._currentContext.extendedSelection; } + public get groupSlice(): any { + return this._currentContext.groupSlice; + } getCurrentPanelId(): string | null { return this.params.panel_id || this.currentPanel?.id || null; } @@ -538,6 +542,7 @@ async function executeOperatorAsGenerator( selected_labels: formatSelectedLabels(currentContext.selectedLabels), view: currentContext.view, view_name: currentContext.viewName, + group_slice: currentContext.groupSlice, }, "json-stream" ); @@ -700,6 +705,7 @@ export async function executeOperatorWithContext( selected_labels: formatSelectedLabels(currentContext.selectedLabels), view: currentContext.view, view_name: currentContext.viewName, + group_slice: currentContext.groupSlice, } ); result = serverResult.result; @@ -802,6 +808,7 @@ export async function resolveRemoteType( selected_labels: formatSelectedLabels(currentContext.selectedLabels), view: currentContext.view, view_name: currentContext.viewName, + group_slice: currentContext.groupSlice, } ); @@ -875,6 +882,7 @@ export async function resolveExecutionOptions( selected_labels: formatSelectedLabels(currentContext.selectedLabels), view: currentContext.view, view_name: currentContext.viewName, + group_slice: currentContext.groupSlice, } ); @@ -905,6 +913,7 @@ export async function fetchRemotePlacements(ctx: ExecutionContext) { selected_labels: formatSelectedLabels(currentContext.selectedLabels), current_sample: currentContext.currentSample, view_name: currentContext.viewName, + group_slice: currentContext.groupSlice, } ); if (result && result.error) { diff --git a/app/packages/operators/src/state.ts b/app/packages/operators/src/state.ts index f489395f1d..97e675c5d1 100644 --- a/app/packages/operators/src/state.ts +++ b/app/packages/operators/src/state.ts @@ -93,6 +93,7 @@ const globalContextSelector = selector({ const selectedLabels = get(fos.selectedLabels); const viewName = get(fos.viewName); const extendedSelection = get(fos.extendedSelection); + const groupSlice = get(fos.groupSlice); return { datasetName, @@ -103,6 +104,7 @@ const globalContextSelector = selector({ selectedLabels, viewName, extendedSelection, + groupSlice, }; }, }); @@ -142,6 +144,7 @@ const useExecutionContext = (operatorName, hooks = {}) => { selectedLabels, viewName, extendedSelection, + groupSlice, } = curCtx; const [analyticsInfo] = useAnalyticsInfo(); const ctx = useMemo(() => { @@ -158,6 +161,7 @@ const useExecutionContext = (operatorName, hooks = {}) => { viewName, extendedSelection, analyticsInfo, + groupSlice, }, hooks ); @@ -172,6 +176,7 @@ const useExecutionContext = (operatorName, hooks = {}) => { hooks, viewName, currentSample, + groupSlice, ]); return ctx; diff --git a/app/packages/operators/src/useCustomPanelHooks.ts b/app/packages/operators/src/useCustomPanelHooks.ts index ef0fc8088b..afbaf851dd 100644 --- a/app/packages/operators/src/useCustomPanelHooks.ts +++ b/app/packages/operators/src/useCustomPanelHooks.ts @@ -25,6 +25,7 @@ export interface CustomPanelProps { onChangeSelected?: string; onChangeSelectedLabels?: string; onChangeExtendedSelection?: string; + onChangeGroupSlice?: string; dimensions: DimensionsType | null; panelName?: string; panelLabel?: string; @@ -129,6 +130,12 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks { ctx.selectedLabels, props.onChangeSelectedLabels ); + useCtxChangePanelEvent( + isLoaded, + panelId, + ctx.groupSlice, + props.onChangeGroupSlice + ); useEffect(() => { onLoad(); diff --git a/docs/source/plugins/developing_plugins.rst b/docs/source/plugins/developing_plugins.rst index 3032689bdc..0bec46198d 100644 --- a/docs/source/plugins/developing_plugins.rst +++ b/docs/source/plugins/developing_plugins.rst @@ -2047,6 +2047,19 @@ subsequent sections. } ctx.panel.set_state("event", "on_change_extended_selection") ctx.panel.set_data("event_data", event) + + def on_change_group_slice(self, ctx): + """Implement this method to set panel state/data when the current + group slice changes. + + The current group slice will be available via ``ctx.group_slice``. + """ + event = { + "data": ctx.group_slice, + "description": "the current group slice", + } + ctx.panel.set_state("event", "on_change_group_slice") + ctx.panel.set_data("event_data", event) ####################################################################### # Custom events diff --git a/fiftyone/operators/executor.py b/fiftyone/operators/executor.py index 151343cd42..57df1b4b88 100644 --- a/fiftyone/operators/executor.py +++ b/fiftyone/operators/executor.py @@ -697,6 +697,11 @@ def ops(self): """ return self._ops + @property + def group_slice(self): + """The group slice of the view.""" + return self.request_params.get("group_slice", None) + def prompt( self, operator_uri, diff --git a/fiftyone/operators/operations.py b/fiftyone/operators/operations.py index 01d46dcc22..2af0e1a4fe 100644 --- a/fiftyone/operators/operations.py +++ b/fiftyone/operators/operations.py @@ -316,6 +316,7 @@ def register_panel( on_change_selected=None, on_change_selected_labels=None, on_change_extended_selection=None, + on_change_group_slice=None, allow_duplicates=False, ): """Registers a panel with the given name and lifecycle callbacks. @@ -353,6 +354,7 @@ def register_panel( current selected labels changes on_change_extended_selection (None): an operator to invoke when the current extended selection changes + on_change_group_slice (None): an operator to invoke when the group slice changes allow_duplicates (False): whether to allow multiple instances of the panel to the opened """ @@ -375,6 +377,7 @@ def register_panel( "on_change_selected": on_change_selected, "on_change_selected_labels": on_change_selected_labels, "on_change_extended_selection": on_change_extended_selection, + "on_change_group_slice": on_change_group_slice, "allow_duplicates": allow_duplicates, } return self._ctx.trigger("register_panel", params=params) diff --git a/fiftyone/operators/panel.py b/fiftyone/operators/panel.py index c2a032ec51..b9d897c2fd 100644 --- a/fiftyone/operators/panel.py +++ b/fiftyone/operators/panel.py @@ -125,6 +125,7 @@ def on_startup(self, ctx): "on_change_selected", "on_change_selected_labels", "on_change_extended_selection", + "on_change_group_slice", ] for method in methods + ctx_change_events: if hasattr(self, method) and callable(getattr(self, method)): From 60de3644cfdcb49324b238245f82b320223c30ee Mon Sep 17 00:00:00 2001 From: imanjra Date: Thu, 26 Sep 2024 10:21:23 -0400 Subject: [PATCH 17/30] group slice operator tweaks --- fiftyone/operators/builtin.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/fiftyone/operators/builtin.py b/fiftyone/operators/builtin.py index d879405b5b..1005cc44be 100644 --- a/fiftyone/operators/builtin.py +++ b/fiftyone/operators/builtin.py @@ -1519,9 +1519,8 @@ def execute(self, ctx): ctx.dataset.rename_group_slice(name, new_name) - # @todo ideally we would only set this if the group slice we renamed is - # currently loaded in the App - ctx.ops.set_group_slice(ctx.dataset.default_group_slice) + if ctx.group_slice == name: + ctx.ops.set_group_slice(ctx.dataset.default_group_slice) ctx.ops.reload_dataset() @@ -1567,13 +1566,9 @@ def resolve_input(self, ctx): def execute(self, ctx): name = ctx.params["name"] - ctx.dataset.delete_group_slice(name) - - # @todo ideally we would only set this if the group slice we deleted is - # currently loaded in the App - ctx.ops.set_group_slice(ctx.dataset.default_group_slice) - + if ctx.group_slice == name: + ctx.ops.set_group_slice(ctx.dataset.default_group_slice) ctx.ops.reload_dataset() From e152852f905a10b89c239882cedcae61ae6f40fe Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Thu, 26 Sep 2024 15:57:57 -0400 Subject: [PATCH 18/30] uppercase (#4852) --- app/packages/state/src/utils.test.ts | 10 ++++++++++ app/packages/state/src/utils.ts | 8 ++++---- 2 files changed, 14 insertions(+), 4 deletions(-) create mode 100644 app/packages/state/src/utils.test.ts diff --git a/app/packages/state/src/utils.test.ts b/app/packages/state/src/utils.test.ts new file mode 100644 index 0000000000..94e7b0cd8c --- /dev/null +++ b/app/packages/state/src/utils.test.ts @@ -0,0 +1,10 @@ +import { describe, expect, it } from "vitest"; +import { convertTargets } from "./utils"; + +describe("convertTargets", () => { + it("upper cases rgb hex targets", () => { + expect( + convertTargets([{ target: "#ffffff", value: "white" }]) + ).toStrictEqual({ "#FFFFFF": { label: "white", intTarget: 1 } }); + }); +}); diff --git a/app/packages/state/src/utils.ts b/app/packages/state/src/utils.ts index f3c1c7d191..1bb9e0c99a 100644 --- a/app/packages/state/src/utils.ts +++ b/app/packages/state/src/utils.ts @@ -113,22 +113,22 @@ export const getStandardizedUrls = ( return Object.fromEntries(urls.map(({ field, url }) => [field, url])); }; -const convertTargets = ( +export const convertTargets = ( targets: { target: string; value: string; }[] -) => { +): { [key: string]: { label: string; intTarget: number } | string } => { return Object.fromEntries( (targets || []).map(({ target, value }, i) => { - if (!isNaN(Number(target))) { + if (!Number.isNaN(Number(target))) { // masks targets is for non-rgb masks return [target, value]; } // convert into RGB mask representation // offset of 1 in intTarget because 0 has a special significance - return [target, { label: value, intTarget: i + 1 }]; + return [target.toUpperCase(), { label: value, intTarget: i + 1 }]; }) ); }; From 808260c40b8a88df660a84278fba56a9cba59a8e Mon Sep 17 00:00:00 2001 From: brimoor Date: Thu, 26 Sep 2024 16:45:31 -0400 Subject: [PATCH 19/30] more group_slice updates --- docs/source/plugins/developing_plugins.rst | 1 + fiftyone/operators/builtin.py | 5 +++-- fiftyone/operators/executor.py | 13 ++++++++++++- fiftyone/operators/operations.py | 3 ++- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/docs/source/plugins/developing_plugins.rst b/docs/source/plugins/developing_plugins.rst index 0bec46198d..6ac02fc7f9 100644 --- a/docs/source/plugins/developing_plugins.rst +++ b/docs/source/plugins/developing_plugins.rst @@ -979,6 +979,7 @@ contains the following properties: - `ctx.selected_labels` - the list of currently selected labels in the App, if any - `ctx.extended_selection` - the extended selection of the view, if any +- `ctx.group_slice` - the active group slice in the App, if any - `ctx.user_id` - the ID of the user that invoked the operator, if known - `ctx.panel_id` - the ID of the panel that invoked the operator, if any - `ctx.panel` - a :class:`PanelRef ` diff --git a/fiftyone/operators/builtin.py b/fiftyone/operators/builtin.py index 1005cc44be..bb07d5b3d1 100644 --- a/fiftyone/operators/builtin.py +++ b/fiftyone/operators/builtin.py @@ -1518,9 +1518,8 @@ def execute(self, ctx): new_name = ctx.params["new_name"] ctx.dataset.rename_group_slice(name, new_name) - if ctx.group_slice == name: - ctx.ops.set_group_slice(ctx.dataset.default_group_slice) + ctx.ops.set_group_slice(new_name) ctx.ops.reload_dataset() @@ -1566,9 +1565,11 @@ def resolve_input(self, ctx): def execute(self, ctx): name = ctx.params["name"] + ctx.dataset.delete_group_slice(name) if ctx.group_slice == name: ctx.ops.set_group_slice(ctx.dataset.default_group_slice) + ctx.ops.reload_dataset() diff --git a/fiftyone/operators/executor.py b/fiftyone/operators/executor.py index 57df1b4b88..8b5da0d5ea 100644 --- a/fiftyone/operators/executor.py +++ b/fiftyone/operators/executor.py @@ -17,6 +17,7 @@ import fiftyone as fo import fiftyone.core.dataset as fod +import fiftyone.core.media as fom import fiftyone.core.odm.utils as focu import fiftyone.core.utils as fou import fiftyone.core.view as fov @@ -507,11 +508,13 @@ def dataset(self): """The :class:`fiftyone.core.dataset.Dataset` being operated on.""" if self._dataset is not None: return self._dataset + # Since dataset may have been renamed, always resolve the dataset by # id if it is available uid = self.request_params.get("dataset_id", None) if uid: self._dataset = focu.load_dataset(id=uid) + # Set the dataset_name using the dataset object in case the dataset # has been renamed or changed since the context was created self.request_params["dataset_name"] = self._dataset.name @@ -519,10 +522,18 @@ def dataset(self): uid = self.request_params.get("dataset_name", None) if uid: self._dataset = focu.load_dataset(name=uid) + # TODO: refactor so that this additional reload post-load is not # required if self._dataset is not None: self._dataset.reload() + + if ( + self.group_slice is not None + and self._dataset.media_type == fom.GROUP + ): + self._dataset.group_slice = self.group_slice + return self._dataset @property @@ -699,7 +710,7 @@ def ops(self): @property def group_slice(self): - """The group slice of the view.""" + """The current group slice of the view (if any).""" return self.request_params.get("group_slice", None) def prompt( diff --git a/fiftyone/operators/operations.py b/fiftyone/operators/operations.py index 2af0e1a4fe..933f6f3d55 100644 --- a/fiftyone/operators/operations.py +++ b/fiftyone/operators/operations.py @@ -354,7 +354,8 @@ def register_panel( current selected labels changes on_change_extended_selection (None): an operator to invoke when the current extended selection changes - on_change_group_slice (None): an operator to invoke when the group slice changes + on_change_group_slice (None): an operator to invoke when the group + slice changes allow_duplicates (False): whether to allow multiple instances of the panel to the opened """ From e38dfd0a4552a579161b6be01a015f35d0d9fe51 Mon Sep 17 00:00:00 2001 From: brimoor Date: Thu, 26 Sep 2024 16:50:31 -0400 Subject: [PATCH 20/30] intelligent default --- fiftyone/operators/builtin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fiftyone/operators/builtin.py b/fiftyone/operators/builtin.py index bb07d5b3d1..d754ee5019 100644 --- a/fiftyone/operators/builtin.py +++ b/fiftyone/operators/builtin.py @@ -1487,7 +1487,7 @@ def resolve_input(self, ctx): inputs.enum( "name", slice_selector.values(), - default=None, + default=ctx.group_slice, required=True, label="Group slice", description="The group slice to rename", @@ -1552,7 +1552,7 @@ def resolve_input(self, ctx): inputs.enum( "name", slice_selector.values(), - default=None, + default=ctx.group_slice, required=True, label="Group slice", description="The group slice to delete", From 00e5335e221e935770da99e10c5153e5f02af0f5 Mon Sep 17 00:00:00 2001 From: brimoor Date: Thu, 26 Sep 2024 16:59:35 -0400 Subject: [PATCH 21/30] no monkey business --- fiftyone/core/odm/document.py | 82 ++++++++++++++--------------------- 1 file changed, 32 insertions(+), 50 deletions(-) diff --git a/fiftyone/core/odm/document.py b/fiftyone/core/odm/document.py index e8f774eb1f..5ba479532a 100644 --- a/fiftyone/core/odm/document.py +++ b/fiftyone/core/odm/document.py @@ -713,63 +713,45 @@ def _save( # Update existing document updates = {} - try: - # OPTIMIZATION: we monkey patch `to_mongo()` and - # `_get_changed_fields()` here so that mongoengine's - # implementations of `_delta()` and `_clear_changed_fields()`, - # which call these methods, will not serialize unnecessary - # fields and recompute changed fields unecessarily - self._get_changed_fields_orig = self._get_changed_fields - self._to_mongo_orig = self.to_mongo - - if hasattr(self, "_changed_fields"): - changed_fields = self._get_changed_fields() - roots, paths = self._parse_changed_fields(changed_fields) - else: - # Changes aren't yet tracked, so validate everything - roots, paths = None, None - - if validate: - self._validate_updates( - roots, - paths, - clean=clean, - enforce_read_only=enforce_read_only, - ) + if hasattr(self, "_changed_fields"): + changed_fields = self._get_changed_fields() + roots, paths = self._parse_changed_fields(changed_fields) + else: + # Changes aren't yet tracked, so validate everything + roots, paths = None, None - # OPTIMIZATION: apply monkey patch - doc = self.to_mongo(fields=roots) - self._get_changed_fields = lambda: changed_fields - self.to_mongo = lambda: doc + if validate: + self._validate_updates( + roots, + paths, + clean=clean, + enforce_read_only=enforce_read_only, + ) - sets, unsets = self._delta() + sets, unsets = self._delta() - if sets: - updates["$set"] = sets + if sets: + updates["$set"] = sets - if unsets: - updates["$unset"] = unsets + if unsets: + updates["$unset"] = unsets + + if updates: + ops, updated_existing = self._update( + _id, + updates, + deferred=deferred, + upsert=upsert, + **kwargs, + ) - if updates: - ops, updated_existing = self._update( - _id, - updates, - deferred=deferred, - upsert=upsert, - **kwargs, + if not deferred and not upsert and not updated_existing: + raise ValueError( + "Failed to update %s with ID '%s'" + % (self._doc_name().lower(), str(_id)) ) - if not deferred and not upsert and not updated_existing: - raise ValueError( - "Failed to update %s with ID '%s'" - % (self._doc_name().lower(), str(_id)) - ) - - self._clear_changed_fields() - finally: - # OPTIMIZATION: revert monkey patch - self._get_changed_fields = self._get_changed_fields_orig - self.to_mongo = self._to_mongo_orig + self._clear_changed_fields() return ops From 1839099274d4b8042b764665d07dbf8c576d9eb9 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Thu, 26 Sep 2024 15:00:12 -0700 Subject: [PATCH 22/30] fix noisy analtyics --- app/packages/analytics/src/usingAnalytics.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/packages/analytics/src/usingAnalytics.ts b/app/packages/analytics/src/usingAnalytics.ts index 24c1c160e9..7c468534dd 100644 --- a/app/packages/analytics/src/usingAnalytics.ts +++ b/app/packages/analytics/src/usingAnalytics.ts @@ -56,8 +56,10 @@ export class Analytics { if (this._segment) return; this._debug = info?.debug; if (!info || info.doNotTrack) { - console.warn("Analytics disabled"); - console.log(info); + if (this._debug) { + console.warn("Analytics disabled"); + console.log(info); + } this.disable(); return; } @@ -66,7 +68,9 @@ export class Analytics { } this._disableUrlTracking = info.disableUrlTracking; if (!info.writeKey) { - console.warn("Analytics disabled (no write key)"); + if (this._debug) { + console.warn("Analytics disabled (no write key)"); + } this.disable(); return; } From b305252a70956291d534d7cc711df8074500016a Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Thu, 26 Sep 2024 19:24:38 -0700 Subject: [PATCH 23/30] fix url tracking --- app/packages/analytics/src/analytics.test.ts | 4 ++-- app/packages/analytics/src/usingAnalytics.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/packages/analytics/src/analytics.test.ts b/app/packages/analytics/src/analytics.test.ts index 0520bf5585..9a29014374 100644 --- a/app/packages/analytics/src/analytics.test.ts +++ b/app/packages/analytics/src/analytics.test.ts @@ -151,7 +151,7 @@ describe("Analytics", () => { // segment should be called with context.page.url = undefined expect(mockSegment.track).toHaveBeenCalledWith("custom_event", undefined, { context: { - page: { url: undefined }, + page: { url: null, path: null, title: null }, }, }); }); @@ -208,7 +208,7 @@ describe("Analytics", () => { version: "1.0.0", }); }); - + describe("analytics.page()", () => { it("should call segment.page()", () => { analytics = new Analytics(); diff --git a/app/packages/analytics/src/usingAnalytics.ts b/app/packages/analytics/src/usingAnalytics.ts index 7c468534dd..9e0ded2031 100644 --- a/app/packages/analytics/src/usingAnalytics.ts +++ b/app/packages/analytics/src/usingAnalytics.ts @@ -123,7 +123,7 @@ export class Analytics { if (!this._segment) return; let opts; if (this._disableUrlTracking) { - opts = { context: { page: { url: undefined } } }; + opts = { context: { page: { url: null, path: null, title: null } } }; } if (this._version) { opts = { ...opts, version: this._version }; From 81037fa6ff282fc671a4c89ab8811cdc6479da09 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Fri, 27 Sep 2024 13:36:32 -0400 Subject: [PATCH 24/30] Render summary fields in modal sidebar (#4851) * render summary fields in modal sidebar * lint log * fix urls * e2e summary fields * e2e fixes --- .../Sidebar/Entries/PathValueEntry.tsx | 139 +++++++++++++++--- app/packages/state/src/recoil/schema.ts | 10 +- app/packages/utilities/src/index.ts | 24 +-- e2e-pw/src/oss/poms/modal/modal-sidebar.ts | 12 ++ .../specs/smoke-tests/summary-fields.spec.ts | 70 +++++++++ 5 files changed, 226 insertions(+), 29 deletions(-) create mode 100644 e2e-pw/src/oss/specs/smoke-tests/summary-fields.spec.ts diff --git a/app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx b/app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx index 4c62764487..d4114821fa 100644 --- a/app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx +++ b/app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx @@ -1,6 +1,11 @@ import { LoadingDots, useTheme } from "@fiftyone/components"; import * as fos from "@fiftyone/state"; -import { formatPrimitive, makePseudoField } from "@fiftyone/utilities"; +import type { Primitive, Schema } from "@fiftyone/utilities"; +import { + EMBEDDED_DOCUMENT_FIELD, + formatPrimitive, + makePseudoField, +} from "@fiftyone/utilities"; import { KeyboardArrowDown, KeyboardArrowUp } from "@mui/icons-material"; import { useSpring } from "@react-spring/core"; import React, { Suspense, useMemo, useState } from "react"; @@ -51,6 +56,10 @@ const ScalarDiv = styled.div` &.expanded > div { white-space: unset; } + + & a { + color: ${({ theme }) => theme.text.primary}; + } `; const ScalarValueEntry = ({ @@ -116,9 +125,11 @@ const ListContainer = styled(ScalarDiv)` color: ${({ theme }) => theme.text.secondary}; margin-top: 0.25rem; padding: 0.25rem 0.5rem; + display: flex; + flex-direction: column; + row-gap: 0.5rem; & > div { - margin-bottom: 0.5rem; white-space: unset; } `; @@ -186,6 +197,7 @@ const ListValueEntry = ({ { event.preventDefault(); @@ -226,20 +238,31 @@ const LengthLoadable = ({ path }: { path: string }) => { }; const ListLoadable = ({ path }: { path: string }) => { - const data = useData(path); + const data = useData(path); + const { fields, ftype, subfield } = fos.useAssertedRecoilValue( + fos.field(path) + ); + const timeZone = useRecoilValue(fos.timeZone); + + const field = subfield || ftype; + if (!field) { + throw new Error(`expected an ftype for ${path}`); + } + const values = useMemo(() => { - return data - ? Array.from(data).map((value) => prettify(value as string)) - : []; - }, [data]); + return Array.from(data || []).map((value) => + format({ fields, ftype: field, value, timeZone }) + ); + }, [data, field, fields, timeZone]); + return ( - + {values.map((v, i) => ( -
- {v} +
+ {v === null ? "None" : v}
))} - {values.length === 0 && <>No results} + {values.length === 0 && "No results"} ); }; @@ -263,9 +286,9 @@ const SlicesListLoadable = ({ path }: { path: string }) => { {slice}
{(data || []).map((value, i) => ( -
{prettify(value as string)}
+
{prettify(value as string)}
))} - {(!data || !data.length) && <>No results} + {(!data || !data.length) && "No results"} ); })} @@ -286,7 +309,7 @@ const SlicesLoadable = ({ path }: { path: string }) => { <> {Object.entries(values).map(([slice, value], i) => { const none = value === null || value === undefined; - const formatted = formatPrimitive({ ftype, value, timeZone }); + const formatted = format({ ftype, value, timeZone }); const add = none ? { color } : {}; return ( @@ -297,7 +320,7 @@ const SlicesLoadable = ({ path }: { path: string }) => { columnGap: "0.5rem", marginBottom: "0.5rem", }} - key={i} + key={i.toString()} >
{slice}
(path: string) => { const Loadable = ({ path }: { path: string }) => { const value = useData(path); const none = value === null || value === undefined; - const { ftype } = useRecoilValue(fos.field(path)) ?? makePseudoField(path); + const { fields, ftype } = + useRecoilValue(fos.field(path)) ?? makePseudoField(path); const color = useRecoilValue(fos.pathColor(path)); const timeZone = useRecoilValue(fos.timeZone); - const formatted = formatPrimitive({ ftype, value, timeZone }); + + const formatted = useMemo( + () => format({ fields, ftype, timeZone, value }), + [fields, ftype, timeZone, value] + ); return (
e.stopPropagation()} onClick={(e) => e.stopPropagation()} style={none ? { color } : {}} title={typeof formatted === "string" ? formatted : undefined} @@ -439,4 +468,80 @@ const PathValueEntry = ({ ); }; +interface PrimitivesObject { + [key: string]: Primitive; +} + +type Primitives = Primitive | PrimitivesObject; + +const format = ({ + fields, + ftype, + timeZone, + value, +}: { + fields?: Schema; + ftype: string; + timeZone: string; + value: Primitives; +}) => { + if (ftype === EMBEDDED_DOCUMENT_FIELD && typeof value === "object") { + return formatObject({ fields, timeZone, value: value as object }); + } + + return formatPrimitiveOrURL({ ftype, value: value as Primitive, timeZone }); +}; + +const formatPrimitiveOrURL = (params: { + fields?: Schema; + ftype: string; + timeZone: string; + value: Primitive; +}) => { + const result = formatPrimitive(params); + + return result instanceof URL ? ( + + {result.toString()} + + ) : ( + result + ); +}; + +const formatObject = ({ + fields, + timeZone, + value, +}: { + fields?: Schema; + timeZone: string; + value: object; +}) => { + return Object.entries(value) + .map(([k, v]) => { + if (!fields?.[k]?.ftype) { + return null; + } + + const text = formatPrimitiveOrURL({ + ftype: fields?.[k]?.ftype, + timeZone, + value: v, + }); + + return ( +
+ {k} + {text} +
+ ); + }) + .filter((entry) => Boolean(entry)); +}; + export default React.memo(PathValueEntry); diff --git a/app/packages/state/src/recoil/schema.ts b/app/packages/state/src/recoil/schema.ts index 715562ba86..2a746f44ac 100644 --- a/app/packages/state/src/recoil/schema.ts +++ b/app/packages/state/src/recoil/schema.ts @@ -17,13 +17,13 @@ import { LABEL_LISTS, LABEL_LISTS_MAP, LIST_FIELD, - meetsFieldType, OBJECT_ID_FIELD, + STRING_FIELD, Schema, StrictField, - STRING_FIELD, VALID_NUMERIC_TYPES, VALID_PRIMITIVE_TYPES, + meetsFieldType, withPath, } from "@fiftyone/utilities"; import { RecoilState, selector, selectorFamily } from "recoil"; @@ -786,7 +786,11 @@ export const isOfDocumentFieldList = selectorFamily({ get: (path: string) => ({ get }) => { - const f = get(field(path.split(".")[0])); + const parent = path.split(".").slice(0, -1).join("."); + if (!parent) { + return false; + } + const f = get(field(parent)); return [ DYNAMIC_EMBEDDED_DOCUMENT_FIELD, diff --git a/app/packages/utilities/src/index.ts b/app/packages/utilities/src/index.ts index 8a85bdd8fa..3f1618a21c 100644 --- a/app/packages/utilities/src/index.ts +++ b/app/packages/utilities/src/index.ts @@ -3,13 +3,13 @@ import _ from "lodash"; import mime from "mime"; import { Field } from "./schema"; +export * from "./Resource"; export * from "./buffer-manager"; export * from "./color"; export * from "./errors"; export * from "./fetch"; export * from "./order"; export * from "./paths"; -export * from "./Resource"; export * from "./schema"; export * as styles from "./styles"; export * from "./type-check"; @@ -618,6 +618,13 @@ export const formatDate = (timeStamp: number): string => { .replaceAll("/", "-"); }; +export type Primitive = + | number + | null + | string + | undefined + | { datetime: number }; + export const formatPrimitive = ({ ftype, timeZone, @@ -625,24 +632,23 @@ export const formatPrimitive = ({ }: { ftype: string; timeZone: string; - value: unknown; + value: Primitive; }) => { - if (value === null || value === undefined) return undefined; + if (value === null || value === undefined) return null; switch (ftype) { case FRAME_SUPPORT_FIELD: - value = `[${value[0]}, ${value[1]}]`; - break; + return `[${value[0]}, ${value[1]}]`; case DATE_FIELD: // @ts-ignore - value = formatDate(value.datetime as number); - break; + return formatDate(value?.datetime as number); case DATE_TIME_FIELD: // @ts-ignore - value = formatDateTime(value.datetime as number, timeZone); + return formatDateTime(value?.datetime as number, timeZone); } - return prettify(String(value)); + // @ts-ignore + return prettify(value); }; export const makePseudoField = (path: string): Field => ({ diff --git a/e2e-pw/src/oss/poms/modal/modal-sidebar.ts b/e2e-pw/src/oss/poms/modal/modal-sidebar.ts index 665c0eb1ee..7a7ae699cc 100644 --- a/e2e-pw/src/oss/poms/modal/modal-sidebar.ts +++ b/e2e-pw/src/oss/poms/modal/modal-sidebar.ts @@ -126,6 +126,18 @@ class SidebarAsserter { ); } + async verifyObject(key: string, obj: { [key: string]: string }) { + const locator = this.modalSidebarPom.getSidebarEntry(key); + + for (const k in obj) { + const v = obj[k]; + const entry = locator.getByTestId(`key-value-${k}-${v}`); + + await expect(entry.getByTestId(`key-${k}`)).toHaveText(k); + await expect(entry.getByTestId(`value-${v}`)).toHaveText(v); + } + } + async verifyLabelTagCount(count: number) { await this.modalSidebarPom.page.waitForFunction( (count_) => { diff --git a/e2e-pw/src/oss/specs/smoke-tests/summary-fields.spec.ts b/e2e-pw/src/oss/specs/smoke-tests/summary-fields.spec.ts new file mode 100644 index 0000000000..42d492153d --- /dev/null +++ b/e2e-pw/src/oss/specs/smoke-tests/summary-fields.spec.ts @@ -0,0 +1,70 @@ +import { test as base } from "src/oss/fixtures"; +import { GridPom } from "src/oss/poms/grid"; +import { ModalPom } from "src/oss/poms/modal"; +import { getUniqueDatasetNameWithPrefix } from "src/oss/utils"; + +const test = base.extend<{ grid: GridPom; modal: ModalPom }>({ + grid: async ({ page, eventUtils }, use) => { + await use(new GridPom(page, eventUtils)); + }, + modal: async ({ page, eventUtils }, use) => { + await use(new ModalPom(page, eventUtils)); + }, +}); + +const datasetName = getUniqueDatasetNameWithPrefix("summary-fields"); + +test.describe("summary fields", () => { + test.beforeAll(async ({ fiftyoneLoader }) => { + await fiftyoneLoader.executePythonCode(` + import fiftyone as fo + + dataset = fo.Dataset("${datasetName}") + dataset.persistent = True + dataset.add_sample( + fo.Sample( + filepath=f"image.png", + summary=fo.DynamicEmbeddedDocument(one="two", three="four"), + summaries=[ + fo.DynamicEmbeddedDocument(five="six", seven="eight"), + fo.DynamicEmbeddedDocument(nine="ten"), + ], + ) + ) + dataset.app_config.sidebar_groups = [ + fo.SidebarGroupDocument( + name="summaries", paths=["summary", "summaries"], expanded=True + ) + ] + dataset.save() + dataset.add_dynamic_sample_fields() + `); + }); + + test("modal sidebar summary fields render", async ({ + eventUtils, + fiftyoneLoader, + grid, + modal, + page, + }) => { + await fiftyoneLoader.waitUntilGridVisible(page, datasetName); + await grid.openFirstSample(); + await modal.waitForSampleLoadDomAttribute(true); + await modal.sidebar.assert.verifyObject("summary", { + one: "two", + three: "four", + }); + const entryExpandPromise = eventUtils.getEventReceivedPromiseForPredicate( + "animation-onRest", + () => true + ); + await modal.sidebar.clickFieldDropdown("summaries"); + await entryExpandPromise; + await modal.sidebar.assert.verifyObject("summaries", { + five: "six", + seven: "eight", + nine: "ten", + }); + }); +}); From d3960f888657ee6fa44454907a899afc4b02e773 Mon Sep 17 00:00:00 2001 From: imanjra Date: Fri, 27 Sep 2024 14:06:47 -0400 Subject: [PATCH 25/30] fix state scope missing in some panel hooks --- app/packages/spaces/src/state.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/app/packages/spaces/src/state.ts b/app/packages/spaces/src/state.ts index 7cb54b6e69..360ab4caae 100644 --- a/app/packages/spaces/src/state.ts +++ b/app/packages/spaces/src/state.ts @@ -59,14 +59,18 @@ export const panelStateSelector = selectorFamily({ (params: PanelStateParameter) => ({ get }) => { const { panelId, local, scope } = params; - const stateAtom = getStateAtom(local, scope); + const fallbackScope = get(panelIdToScopeAtom)[panelId]; + const computedScope = scope ?? fallbackScope; + const stateAtom = getStateAtom(local, computedScope); return get(stateAtom).get(panelId); }, set: (params: PanelStateParameter) => ({ get, set }, newValue) => { const { panelId, local, scope } = params; - const stateAtom = getStateAtom(local, scope); + const fallbackScope = get(panelIdToScopeAtom)[panelId]; + const computedScope = scope ?? fallbackScope; + const stateAtom = getStateAtom(local, computedScope); const newState = new Map(get(stateAtom)); newState.set(panelId, newValue); set(stateAtom, newState); @@ -125,7 +129,7 @@ export const savedWorkspacesAtom = atom({ }, }); -export const panelIdToScopeAtom = atom({ +export const panelIdToScopeAtom = atom({ key: "panelIdToScopeAtom", default: {}, }); @@ -134,3 +138,7 @@ function getStateAtom(local?: boolean, scope?: string) { const nonGridScope = scope !== "grid"; return local || nonGridScope ? panelsLocalStateAtom : panelsStateAtom; } + +type PanelIdToScopeType = { + [panelId: string]: string; +}; From f5e8a5a2c1673a40e251874b5623f40550dde8ef Mon Sep 17 00:00:00 2001 From: Sashank Aryal <66688606+sashankaryal@users.noreply.github.com> Date: Fri, 27 Sep 2024 13:30:46 -0500 Subject: [PATCH 26/30] timeline fix: loadrange in imavid (#4857) * add dark orange indicator for buffering status * shouldn't play if timeline not initialized * fix edge cases in buffer manager * add onAnimationStutter * add onAnimationStutter * loadrange for imavid * remove log * use 30 as default frame rate / multiplier * add speed text on hover * fix effect dep bug * resolve promise when all samples addressed --- .../src/components/Modal/ImaVidLooker.tsx | 112 ++++++++++++------ .../looker/src/elements/imavid/index.ts | 21 +++- .../looker/src/lookers/imavid/controller.ts | 44 ++++--- app/packages/playback/src/lib/constants.ts | 2 +- app/packages/playback/src/lib/state.ts | 12 +- .../playback/src/lib/use-create-timeline.ts | 27 ++++- .../playback/src/views/PlaybackElements.tsx | 18 ++- .../src/buffer-manager/buffer-manager.test.ts | 12 ++ .../utilities/src/buffer-manager/index.ts | 22 ++-- 9 files changed, 190 insertions(+), 80 deletions(-) diff --git a/app/packages/core/src/components/Modal/ImaVidLooker.tsx b/app/packages/core/src/components/Modal/ImaVidLooker.tsx index 1f1c179d4d..7af9d2b911 100644 --- a/app/packages/core/src/components/Modal/ImaVidLooker.tsx +++ b/app/packages/core/src/components/Modal/ImaVidLooker.tsx @@ -1,6 +1,5 @@ import { useTheme } from "@fiftyone/components"; import { AbstractLooker, ImaVidLooker } from "@fiftyone/looker"; -import { BUFFERING_PAUSE_TIMEOUT } from "@fiftyone/looker/src/lookers/imavid/constants"; import { BaseState } from "@fiftyone/looker/src/state"; import { FoTimelineConfig, useCreateTimeline } from "@fiftyone/playback"; import { useDefaultTimelineNameImperative } from "@fiftyone/playback/src/lib/use-default-timeline-name"; @@ -54,6 +53,8 @@ export const ImaVidLookerReact = React.memo( }); const { activeLookerRef, setActiveLookerRef } = useModalContext(); + const imaVidLookerRef = + activeLookerRef as unknown as React.MutableRefObject; const looker = React.useMemo( () => createLooker.current(sampleDataWithExtraParams), @@ -152,19 +153,59 @@ export const ImaVidLookerReact = React.memo( ); }, [ref]); - const loadRange = React.useCallback(async (range: BufferRange) => { - // todo: implement - return new Promise((resolve) => { - setTimeout(() => { - resolve(); - }, BUFFERING_PAUSE_TIMEOUT); - }); - }, []); + const loadRange = React.useCallback( + async (range: Readonly) => { + const storeBufferManager = + imaVidLookerRef.current.frameStoreController.storeBufferManager; + const fetchBufferManager = + imaVidLookerRef.current.frameStoreController.fetchBufferManager; + + if (storeBufferManager.containsRange(range)) { + return; + } + + const unprocessedStoreBufferRange = + storeBufferManager.getUnprocessedBufferRange(range); + const unprocessedBufferRange = + fetchBufferManager.getUnprocessedBufferRange( + unprocessedStoreBufferRange + ); + + if (!unprocessedBufferRange) { + return; + } + + imaVidLookerRef.current.frameStoreController.enqueueFetch( + unprocessedBufferRange + ); + + return new Promise((resolve) => { + const fetchMoreListener = (e: CustomEvent) => { + if ( + e.detail.id === imaVidLookerRef.current.frameStoreController.key + ) { + if (storeBufferManager.containsRange(unprocessedBufferRange)) { + resolve(); + window.removeEventListener( + "fetchMore", + fetchMoreListener as EventListener + ); + } + } + }; + + window.addEventListener( + "fetchMore", + fetchMoreListener as EventListener, + { once: true } + ); + }); + }, + [] + ); const renderFrame = React.useCallback((frameNumber: number) => { - ( - activeLookerRef.current as unknown as ImaVidLooker - )?.element.drawFrameNoAnimation(frameNumber); + imaVidLookerRef.current?.element.drawFrameNoAnimation(frameNumber); }, []); const { getName } = useDefaultTimelineNameImperative(); @@ -189,7 +230,7 @@ export const ImaVidLookerReact = React.memo( const readyWhen = useCallback(async () => { return new Promise((resolve) => { - // wait for total frame count to be resolved + // hack: wait for total frame count to be resolved let intervalId; intervalId = setInterval(() => { if (totalFrameCountRef.current) { @@ -200,6 +241,10 @@ export const ImaVidLookerReact = React.memo( }); }, []); + const onAnimationStutter = useCallback(() => { + imaVidLookerRef.current?.element.checkFetchBufferManager(); + }, []); + const { isTimelineInitialized, registerOnPauseCallback, @@ -210,6 +255,10 @@ export const ImaVidLookerReact = React.memo( name: timelineName, config: timelineCreationConfig, waitUntilInitialized: readyWhen, + // using this mechanism to resume fetch if it was paused + // ideally we have control of fetch in this component but can't do that yet + // since imavid is part of the grid too + onAnimationStutter, }); /** @@ -224,35 +273,27 @@ export const ImaVidLookerReact = React.memo( }); registerOnPlayCallback(() => { - (activeLookerRef.current as unknown as ImaVidLooker).element.update( - () => ({ - playing: true, - }) - ); + imaVidLookerRef.current?.element?.update(() => ({ + playing: true, + })); }); registerOnPauseCallback(() => { - (activeLookerRef.current as unknown as ImaVidLooker).element.update( - () => ({ - playing: false, - }) - ); + imaVidLookerRef.current?.element?.update(() => ({ + playing: false, + })); }); registerOnSeekCallbacks({ start: () => { - (activeLookerRef.current as unknown as ImaVidLooker).element.update( - () => ({ - seeking: true, - }) - ); + imaVidLookerRef.current?.element?.update(() => ({ + seeking: true, + })); }, end: () => { - (activeLookerRef.current as unknown as ImaVidLooker).element.update( - () => ({ - seeking: false, - }) - ); + imaVidLookerRef.current?.element?.update(() => ({ + seeking: false, + })); }, }); } @@ -265,9 +306,8 @@ export const ImaVidLookerReact = React.memo( // hack: poll every 10ms for total frame count // replace with event listener or callback let intervalId = setInterval(() => { - const totalFrameCount = ( - activeLookerRef.current as unknown as ImaVidLooker - ).frameStoreController.totalFrameCount; + const totalFrameCount = + imaVidLookerRef.current.frameStoreController.totalFrameCount; if (totalFrameCount) { setTotalFrameCount(totalFrameCount); clearInterval(intervalId); diff --git a/app/packages/looker/src/elements/imavid/index.ts b/app/packages/looker/src/elements/imavid/index.ts index 2545f48d92..b6038dcaf3 100644 --- a/app/packages/looker/src/elements/imavid/index.ts +++ b/app/packages/looker/src/elements/imavid/index.ts @@ -362,6 +362,8 @@ export class ImaVidElement extends BaseElement { /** * Queue up frames to be fetched if necessary. * This method is not blocking, it merely enqueues a fetch job. + * + * This is for legacy imavid, which is used for thumbnail imavid. */ private ensureBuffers(state: Readonly) { if (!this.framesController.totalFrameCount) { @@ -407,6 +409,19 @@ export class ImaVidElement extends BaseElement { } } + /** + * Starts fetch if there are buffers in the fetch buffer manager + */ + public checkFetchBufferManager() { + if (!this.framesController.totalFrameCount) { + return; + } + + if (this.framesController.fetchBufferManager.buffers.length > 0) { + this.framesController.resumeFetch(); + } + } + renderSelf(state: Readonly) { const { options: { playbackRate, loop }, @@ -443,7 +458,11 @@ export class ImaVidElement extends BaseElement { this.framesController.destroy(); } - this.ensureBuffers(state); + if (this.isThumbnail) { + this.ensureBuffers(state); + } else { + this.checkFetchBufferManager(); + } if (!playing && this.isAnimationActive) { // this flag will be picked up in `drawFrame`, that in turn will call `pause` diff --git a/app/packages/looker/src/lookers/imavid/controller.ts b/app/packages/looker/src/lookers/imavid/controller.ts index 816a4ce278..7ea1b8040e 100644 --- a/app/packages/looker/src/lookers/imavid/controller.ts +++ b/app/packages/looker/src/lookers/imavid/controller.ts @@ -123,8 +123,8 @@ export class ImaVidFramesController { BUFFER_METADATA_FETCHING ); - // subtract by two because 1) cursor is one based and 2) cursor here translates to "after" the cursor - return this.fetchMore(range[0] - 2, range[1] - range[0] || 2).finally( + // subtract/add by two because 1) cursor is one based and 2) cursor here translates to "after" the cursor + return this.fetchMore(range[0] - 2, range[1] - range[0] + 2).finally( () => { this.fetchBufferManager.removeMetadataFromBufferRange(index); } @@ -165,7 +165,7 @@ export class ImaVidFramesController { return this.config.page; } - private get key() { + public get key() { return this.config.key; } @@ -257,23 +257,35 @@ export class ImaVidFramesController { const frameIndices = imageFetchPromisesMap.keys(); const imageFetchPromises = imageFetchPromisesMap.values(); - Promise.all(imageFetchPromises).then((sampleIds) => { - for (let i = 0; i < sampleIds.length; i++) { - const frameIndex = frameIndices.next().value; - const sampleId = sampleIds[i]; - this.store.frameIndex.set(frameIndex, sampleId); - this.store.reverseFrameIndex.set(sampleId, frameIndex); - - this.storeBufferManager.addNewRange([ + Promise.all(imageFetchPromises) + .then((sampleIds) => { + for (let i = 0; i < sampleIds.length; i++) { + const frameIndex = frameIndices.next().value; + const sampleId = sampleIds[i]; + this.store.frameIndex.set(frameIndex, sampleId); + this.store.reverseFrameIndex.set(sampleId, frameIndex); + } + resolve(); + }) + .then(() => { + const newRange = [ Number(data.samples.edges[0].cursor) + 1, Number( data.samples.edges[data.samples.edges.length - 1].cursor ) + 1, - ]); - - resolve(); - } - }); + ] as BufferRange; + + this.storeBufferManager.addNewRange(newRange); + + window.dispatchEvent( + new CustomEvent("fetchMore", { + detail: { + id: this.key, + }, + bubbles: false, + }) + ); + }); } }, }); diff --git a/app/packages/playback/src/lib/constants.ts b/app/packages/playback/src/lib/constants.ts index a48ad2fd2e..f711f22dd4 100644 --- a/app/packages/playback/src/lib/constants.ts +++ b/app/packages/playback/src/lib/constants.ts @@ -1,7 +1,7 @@ export const DEFAULT_FRAME_NUMBER = 1; export const DEFAULT_LOOP = false; export const DEFAULT_SPEED = 1; -export const DEFAULT_TARGET_FRAME_RATE = 29.97; +export const DEFAULT_TARGET_FRAME_RATE = 30; export const DEFAULT_USE_TIME_INDICATOR = false; export const GLOBAL_TIMELINE_ID = "fo-timeline-global"; export const LOAD_RANGE_SIZE = 250; diff --git a/app/packages/playback/src/lib/state.ts b/app/packages/playback/src/lib/state.ts index 6674f2d932..66702627dc 100644 --- a/app/packages/playback/src/lib/state.ts +++ b/app/packages/playback/src/lib/state.ts @@ -133,6 +133,11 @@ export type CreateFoTimeline = { * If true, the creator will be responsible for managing the animation loop. */ optOutOfAnimation?: boolean; + + /** + * Callback to be called when the animation stutters. + */ + onAnimationStutter?: () => void; }; const _frameNumbers = atomFamily((_timelineName: TimelineName) => @@ -339,7 +344,7 @@ export const setFrameNumberAtom = atom( set(_currentBufferingRange(name), newLoadRange); try { - await Promise.all(rangeLoadPromises); + await Promise.allSettled(rangeLoadPromises); bufferManager.addNewRange(newLoadRange); } catch (e) { // todo: handle error better, maybe retry @@ -358,9 +363,8 @@ export const setFrameNumberAtom = atom( renderPromises.push(subscriber.renderFrame(newFrameNumber)); }); - Promise.all(renderPromises).then(() => { - set(_frameNumbers(name), newFrameNumber); - }); + await Promise.allSettled(renderPromises); + set(_frameNumbers(name), newFrameNumber); } ); diff --git a/app/packages/playback/src/lib/use-create-timeline.ts b/app/packages/playback/src/lib/use-create-timeline.ts index 2610682eee..b70719b009 100644 --- a/app/packages/playback/src/lib/use-create-timeline.ts +++ b/app/packages/playback/src/lib/use-create-timeline.ts @@ -52,6 +52,13 @@ export const useCreateTimeline = ( const setFrameNumber = useSetAtom(setFrameNumberAtom); const setPlayHeadState = useSetAtom(updatePlayheadStateAtom); + /** + * this effect syncs onAnimationStutter ref from props + */ + useEffect(() => { + onAnimationStutterRef.current = newTimelineProps.onAnimationStutter; + }, [newTimelineProps.onAnimationStutter]); + /** * this effect creates the timeline */ @@ -137,6 +144,7 @@ export const useCreateTimeline = ( const isAnimationActiveRef = useRef(false); const isLastDrawFinishedRef = useRef(true); const frameNumberRef = useRef(frameNumber); + const onAnimationStutterRef = useRef(newTimelineProps.onAnimationStutter); const onPlayListenerRef = useRef<() => void>(); const onPauseListenerRef = useRef<() => void>(); const onSeekCallbackRefs = useRef<{ start: () => void; end: () => void }>(); @@ -145,11 +153,15 @@ export const useCreateTimeline = ( const updateFreqRef = useRef(updateFreq); const play = useCallback(() => { + if (!isTimelineInitialized) { + return; + } + setPlayHeadState({ name: timelineName, state: "playing" }); if (onPlayListenerRef.current) { onPlayListenerRef.current(); } - }, [timelineName]); + }, [timelineName, isTimelineInitialized]); const pause = useCallback(() => { setPlayHeadState({ name: timelineName, state: "paused" }); @@ -167,7 +179,7 @@ export const useCreateTimeline = ( play(); e.stopPropagation(); }, - [timelineName] + [timelineName, play] ); const onPauseEvent = useCallback( @@ -179,7 +191,7 @@ export const useCreateTimeline = ( pause(); e.stopPropagation(); }, - [timelineName] + [timelineName, pause] ); const onSeek = useCallback( @@ -258,13 +270,18 @@ export const useCreateTimeline = ( // queue next animation before draw animationId.current = requestAnimationFrame(animate); + // usually happens when we're out of frames in store if (!isLastDrawFinishedRef.current) { + queueMicrotask(() => { + onAnimationStutterRef.current?.(); + }); return; } // drawing logic is owned by subscribers and invoked by setFrameNumber // we don't increase frame number until the draw is complete isLastDrawFinishedRef.current = false; + setFrameNumber({ name: timelineName, newFrameNumber: targetFrameNumber, @@ -272,6 +289,9 @@ export const useCreateTimeline = ( .then(() => { frameNumberRef.current = targetFrameNumber; }) + .catch((e) => { + console.error("error setting frame number", e); + }) .finally(() => { isLastDrawFinishedRef.current = true; }); @@ -292,6 +312,7 @@ export const useCreateTimeline = ( const cancelAnimation = useCallback(() => { cancelAnimationFrame(animationId.current); isAnimationActiveRef.current = false; + lastDrawTime.current = -1; }, []); useEventHandler(window, "play", onPlayEvent); diff --git a/app/packages/playback/src/views/PlaybackElements.tsx b/app/packages/playback/src/views/PlaybackElements.tsx index 28a1fe02df..0b3ffeece8 100644 --- a/app/packages/playback/src/views/PlaybackElements.tsx +++ b/app/packages/playback/src/views/PlaybackElements.tsx @@ -97,7 +97,7 @@ export const Seekbar = React.forwardRef< unBuffered: "var(--fo-palette-neutral-softBorder)", currentProgress: "var(--fo-palette-primary-plainColor)", buffered: "var(--fo-palette-secondary-main)", - loading: "red", + loading: "#a86738", }), [loadedScaled, loadingScaled, value] ); @@ -161,9 +161,6 @@ export const Speed = React.forwardRef< >(({ speed, setSpeed, ...props }, ref) => { const { style, className, ...otherProps } = props; - const [isPlaybackConfigurerOpen, setIsPlaybackConfigurerOpen] = - React.useState(false); - const onChangeSpeed = React.useCallback( (e: React.ChangeEvent) => { setSpeed(parseFloat(e.target.value)); @@ -173,6 +170,10 @@ export const Speed = React.forwardRef< const rangeValue = React.useMemo(() => (speed / 2) * 100, [speed]); + const resetSpeed = React.useCallback(() => { + setSpeed(1); + }, []); + return ( { - setIsPlaybackConfigurerOpen(false); - }} + title={`${speed}x (click to reset)`} > { - setIsPlaybackConfigurerOpen(true); - }} + onClick={resetSpeed} /> { expect(mergedBuffers[0][1]).toBe(20); }); + test("addBufferRangeToBuffer method - same ranges", async () => { + bufferManager.addNewRange([1, 10]); + bufferManager.addNewRange([1, 10]); + + const mergedBuffers = bufferManager.buffers; + + // we expect [1, 10] + expect(mergedBuffers.length).toBe(1); + expect(mergedBuffers[0][0]).toBe(1); + expect(mergedBuffers[0][1]).toBe(10); + }); + test("addBufferRangeToBuffer method - multiple merges", async () => { bufferManager.addNewRange([1, 4]); bufferManager.addNewRange([5, 7]); diff --git a/app/packages/utilities/src/buffer-manager/index.ts b/app/packages/utilities/src/buffer-manager/index.ts index bdfbbeda2c..278162c034 100644 --- a/app/packages/utilities/src/buffer-manager/index.ts +++ b/app/packages/utilities/src/buffer-manager/index.ts @@ -11,8 +11,8 @@ export class BufferManager { [rangeIndex: number]: string; }; - constructor(buffers: Buffers = []) { - this.buffers = buffers; + constructor(buffers: Readonly = []) { + this.buffers = [...buffers]; this.bufferMetadata = {}; } @@ -53,21 +53,21 @@ export class BufferManager { * Time complexity: O(nlogn) */ public addNewRange( - range: Readonly, + newRange: Readonly, ignoreRangesWithMetadata = true ): void { - if (!range) { + if (!newRange) { return; } - if (range[1] < range[0]) { + if (newRange[1] < newRange[0]) { throw new Error( - `invalid range: range[1] (value = ${range[1]}) must be >= range[0] (value = ${range[0]})` + `invalid range: range[1] (value = ${newRange[1]}) must be >= range[0] (value = ${newRange[0]})` ); } // add the new range to the buffer - this.buffers.push(range); + this.buffers.push(newRange); // sort the buffers based on their start value this.buffers.sort((a, b) => a[0] - b[0]); @@ -106,7 +106,7 @@ export class BufferManager { // if current interval is not overlapping with stack top, // push it to the stack if (!areTwoRangesConsecutive && top[1] < rangesWithoutMetadata[i][0]) { - stack.push(rangesWithoutMetadata[i]); + stack.push([...rangesWithoutMetadata[i]]); } // else if end of current interval is more than the // end of stack top interval, update the stack top @@ -207,7 +207,11 @@ export class BufferManager { * input range: [5, 105] * output: [101-105] */ - public getUnprocessedBufferRange(range: Readonly) { + public getUnprocessedBufferRange(range: Readonly | null) { + if (!range) { + return null; + } + const startContainedInRangeIndex = this.getRangeIndexForFrame(range[0]); if (startContainedInRangeIndex === -1) { From ba40b0e1aeee23a266280b0aaf72ae2b2b1872ff Mon Sep 17 00:00:00 2001 From: brimoor Date: Fri, 27 Sep 2024 15:05:13 -0400 Subject: [PATCH 27/30] don't update dataset doc until field values are populated --- fiftyone/migrations/revisions/v1_0_0.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fiftyone/migrations/revisions/v1_0_0.py b/fiftyone/migrations/revisions/v1_0_0.py index e8a5b9b96d..3ea99b9c64 100644 --- a/fiftyone/migrations/revisions/v1_0_0.py +++ b/fiftyone/migrations/revisions/v1_0_0.py @@ -36,8 +36,6 @@ def up(db, dataset_name): added_last_modified_at_frames, ) = _up_fields(dataset_name, frame_fields) - db.datasets.replace_one(match_d, dataset_dict) - # Populate `Sample.created_at` values sample_collection_name = dataset_dict.get("sample_collection_name", None) if sample_collection_name: @@ -62,6 +60,8 @@ def up(db, dataset_name): now, ) + db.datasets.replace_one(match_d, dataset_dict) + def down(db, dataset_name): pass From 3e1a9b1c916ed9f4d2ab6fbe3416fce5e844365a Mon Sep 17 00:00:00 2001 From: imanjra Date: Fri, 27 Sep 2024 16:30:58 -0400 Subject: [PATCH 28/30] increase zIndex of modal sidebar to fix resize bug (#4860) --- app/packages/core/src/components/Sidebar/Sidebar.tsx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/packages/core/src/components/Sidebar/Sidebar.tsx b/app/packages/core/src/components/Sidebar/Sidebar.tsx index bbcecdefef..39a7b178d6 100644 --- a/app/packages/core/src/components/Sidebar/Sidebar.tsx +++ b/app/packages/core/src/components/Sidebar/Sidebar.tsx @@ -11,6 +11,7 @@ import SchemaSettings from "../Schema/SchemaSettings"; import { Filter } from "./Entries"; import style from "./Sidebar.module.css"; import ViewSelection from "./ViewSelection"; +import { useTheme as useMUITheme } from "@mui/material"; const MARGIN = 3; @@ -693,6 +694,7 @@ const InteractiveSidebar = ({ () => new ResizeObserver(placeItems) ); const theme = useTheme(); + const muiTheme = useMUITheme(); return shown ? ( {!modal && ( From f5cdc8888f77843e679a160e896ba3f86b3e8836 Mon Sep 17 00:00:00 2001 From: Sashank Aryal <66688606+sashankaryal@users.noreply.github.com> Date: Fri, 27 Sep 2024 18:20:53 -0500 Subject: [PATCH 29/30] Fix dynamic groups carousel bug (#4863) * fix flashlight rerendering bug in dynamic groups * remove log --- .../carousel/DynamicGroupCarousel.tsx | 40 ++++++++++++++++--- .../DynamicGroupsFlashlightWrapper.tsx | 6 +-- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx index 1583ad26c1..b4dac56971 100644 --- a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx +++ b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx @@ -2,13 +2,13 @@ import { useTheme } from "@fiftyone/components"; import * as fos from "@fiftyone/state"; import { useBrowserStorage } from "@fiftyone/state"; import { Resizable } from "re-resizable"; -import React from "react"; -import { useRecoilValue } from "recoil"; +import React, { useEffect, useState } from "react"; +import { useRecoilCallback, useRecoilValue } from "recoil"; import { DynamicGroupsFlashlightWrapper } from "./DynamicGroupsFlashlightWrapper"; const MAX_CAROUSEL_HEIGHT = 600; -export const DynamicGroupCarousel = () => { +export const DynamicGroupCarousel = React.memo(() => { const [height, setHeight] = useBrowserStorage( "dynamic-group-carousel-height", 150 @@ -17,6 +17,36 @@ export const DynamicGroupCarousel = () => { const theme = useTheme(); const isMainVisible = useRecoilValue(fos.groupMediaIsMainVisibleSetting); + /** + * BIG HACK: TODO: FIX ME + * + * Problem = flashlight is not re-rendering when group by field changes. + * Solution was to key it by groupByValue, but when the component + * subscribes to the groupByFieldValue using useRecoilValue(fos.groupByFieldValue), + * while it solves the problem, it causes flashlight to behave weirdly. + * (try scrolling carousel and selecting samples, flashlight will reset to the front) + * + */ + const getGroupByFieldValue = useRecoilCallback(({ snapshot }) => () => { + const groupByField = snapshot.getLoadable(fos.groupByFieldValue).getValue(); + return groupByField; + }); + + const [groupByValue, setGroupByValue] = useState(getGroupByFieldValue()); + const groupByValueRef = React.useRef(groupByValue); + groupByValueRef.current = groupByValue; + + useEffect(() => { + const intervalId = window.setInterval(() => { + const groupByFieldValue = getGroupByFieldValue(); + if (groupByFieldValue !== groupByValueRef.current) { + setGroupByValue(groupByFieldValue); + } + }, 50); + + return () => window.clearInterval(intervalId); + }, []); + return ( { }} data-cy={"group-carousel"} > - + ); -}; +}); diff --git a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx index 69950952c0..a27625d224 100644 --- a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx +++ b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx @@ -3,7 +3,7 @@ import { Sample, freeVideos } from "@fiftyone/looker"; import * as fos from "@fiftyone/state"; import { selectedSamples } from "@fiftyone/state"; import { get } from "lodash"; -import { +import React, { useCallback, useEffect, useId, @@ -46,7 +46,7 @@ const pageParams = selector({ }, }); -export const DynamicGroupsFlashlightWrapper = () => { +export const DynamicGroupsFlashlightWrapper = React.memo(() => { const id = useId(); const store = fos.useLookerStore(); @@ -175,4 +175,4 @@ export const DynamicGroupsFlashlightWrapper = () => { id={id} >
); -}; +}); From a38596dfe13510a684a13de77962969ba70e6641 Mon Sep 17 00:00:00 2001 From: minhtuevo Date: Fri, 27 Sep 2024 16:56:59 -0700 Subject: [PATCH 30/30] Small bug fix for Numerical Field in Summary Field Operator --- fiftyone/operators/builtin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fiftyone/operators/builtin.py b/fiftyone/operators/builtin.py index d754ee5019..bffe9d6565 100644 --- a/fiftyone/operators/builtin.py +++ b/fiftyone/operators/builtin.py @@ -1255,7 +1255,7 @@ def _create_summary_field_inputs(ctx, inputs): group_by_keys = sorted(p for p in schema if p.startswith(group_prefix)) group_by_selector = types.AutocompleteView() for group in group_by_keys: - group_by_selector.add_choice(group.name, label=group.name) + group_by_selector.add_choice(group, label=group) inputs.enum( "group_by",