Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add python panel timeline support #4834

Merged
merged 11 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import React, {
forwardRef,
useCallback,
useEffect,
useMemo,
useRef,
useState,
} from "react";
import { ObjectSchemaType, ViewPropsType } from "../utils/types";
import {
DEFAULT_FRAME_NUMBER,
GLOBAL_TIMELINE_ID,
} from "@fiftyone/playback/src/lib/constants";
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be worth re-exporting at top-level of that package

import { BufferManager, BufferRange } from "@fiftyone/utilities";
import { usePanelEvent } from "@fiftyone/operators";
import {
usePanelId,
usePanelState,
useSetPanelStateById,
} from "@fiftyone/spaces";
import { useTimeline } from "@fiftyone/playback/src/lib/use-timeline";
Copy link
Contributor

Choose a reason for hiding this comment

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

^

import _ from "lodash";
import { useDefaultTimelineName } from "@fiftyone/playback/src/lib/use-default-timeline-name";
Copy link
Contributor

Choose a reason for hiding this comment

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

^


export default function FrameLoaderView(props: ViewPropsType) {
const { schema, path, data } = props;
const { view = {} } = schema;
const { on_load_range, timeline_id, target } = view;
const { properties } = schema as ObjectSchemaType;
const panelId = usePanelId();
const [myLocalFrameNumber, setMyLocalFrameNumber] =
React.useState(DEFAULT_FRAME_NUMBER);
const triggerEvent = usePanelEvent();
const setPanelState = useSetPanelStateById(true);
const localIdRef = React.useRef<string>();
const bufm = useRef(new BufferManager());

useEffect(() => {
localIdRef.current = Math.random().toString(36).substring(7);
// console.log("localIdRef", localIdRef.current);
if (data?.frames)
// console.log("dispatching frames-loaded", localIdRef.current);
dispatchEvent(
new CustomEvent(`frames-loaded`, {
detail: { localId: localIdRef.current },
})
);
}, [data?.signature]); // remove this JSON.strignify

const loadRange = React.useCallback(
async (range: BufferRange) => {
if (on_load_range) {
// if (!bufm.current.containsRange(range)) {
// // only trigger event if the range is not already in the buffer
// await triggerEvent(panelId, {
// params: { range },
// operator: on_load_range,
// });
// }
const unp = bufm.current.getUnprocessedBufferRange(range);
const isProcessed = unp === null;

if (!isProcessed) {
await triggerEvent(panelId, {
params: { range: unp },
operator: on_load_range,
});
}
console.log("loading range", range);
return new Promise<void>((resolve) => {
window.addEventListener(`frames-loaded`, (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool!

// console.log("frames loaded", e, {'current': localIdRef.current, 'detail': e.detail.localId});
if (
e instanceof CustomEvent &&
e.detail.localId === localIdRef.current
) {
// console.log("resolving");
bufm.current.addNewRange(range);
resolve();
}
});
});
}
},
[triggerEvent, on_load_range, localIdRef.current]
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid including mutable ref.current in dependency arrays

Including localIdRef.current in the dependency array of useCallback can cause unexpected behavior. Since ref.current is mutable and does not trigger re-renders when it changes, it shouldn't be included in dependency arrays. Instead, you can exclude it to ensure the callback remains stable unless other dependencies change.

);

const [currentFrame, setCurrentFrame] = useState(DEFAULT_FRAME_NUMBER);

const myRenderFrame = React.useCallback(
(frameNumber: number) => {
setMyLocalFrameNumber(frameNumber);
// console.log("rendering frame", frameNumber, props);
setPanelState(panelId, (current) => {
const currentData = current.data ? _.cloneDeep(current.data) : {}; // Clone the object
const currentFrameData = _.get(currentData, path, { frames: [] })
.frames[frameNumber];
let updatedData = { ...currentData };
_.set(updatedData, target, currentFrameData); // Use lodash set to update safely
return { ...current, data: updatedData };
Comment on lines +67 to +72
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize deep cloning and updating of currentData

In your setPanelState callback, you perform a deep clone of current.data, which can be performance-intensive. Additionally, you create a new object updatedData to set the target property. You can optimize this by updating the state immutably without unnecessary cloning.

Apply this diff to optimize state updates:

  setPanelState(panelId, (current) => {
-   const currentData = current.data ? _.cloneDeep(current.data) : {}; // Clone the object
-   const currentFrameData = _.get(currentData, path, { frames: [] }).frames[frameNumber];
-   let updatedData = { ...currentData };
-   _.set(updatedData, target, currentFrameData); // Use lodash set to update safely
-   return { ...current, data: updatedData };
+   return {
+     ...current,
+     data: {
+       ...current.data,
+       [target]: _.get(current.data, [path, 'frames', frameNumber], {}),
+     },
+   };
  });
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const currentData = current.data ? _.cloneDeep(current.data) : {}; // Clone the object
const currentFrameData = _.get(currentData, path, { frames: [] })
.frames[frameNumber];
let updatedData = { ...currentData };
_.set(updatedData, target, currentFrameData); // Use lodash set to update safely
return { ...current, data: updatedData };
return {
...current,
data: {
...current.data,
[target]: _.get(current.data, [path, 'frames', frameNumber], {}),
},
};

});
setCurrentFrame(frameNumber);
},
[data, setPanelState, panelId, target]
);

const { isTimelineInitialized, subscribe } = useTimeline();
const [subscribed, setSubscribed] = useState(false);

React.useEffect(() => {
if (subscribed) return;
if (isTimelineInitialized) {
subscribe({
id: timeline_id || GLOBAL_TIMELINE_ID,
loadRange,
renderFrame: myRenderFrame,
});
setSubscribed(true);
}
}, [isTimelineInitialized, loadRange, myRenderFrame, subscribe]);

return null;
}
1 change: 1 addition & 0 deletions app/packages/core/src/plugins/SchemaIO/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,4 @@ export { default as TagsView } from "./TagsView";
export { default as TextFieldView } from "./TextFieldView";
export { default as TupleView } from "./TupleView";
export { default as UnsupportedView } from "./UnsupportedView";
export { default as FrameLoaderView } from "./FrameLoaderView";
2 changes: 0 additions & 2 deletions app/packages/embeddings/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,3 @@ registerComponent({
priority: BUILT_IN_PANEL_PRIORITY_CONST,
},
});

// registerOperator(new OpenEmbeddingsPanel());
1 change: 1 addition & 0 deletions app/packages/playback/package.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"name": "@fiftyone/playback",
"main": "./index.ts",
"packageManager": "yarn@3.2.1",
"devDependencies": {
"@eslint/compat": "^1.1.1",
Expand Down
6 changes: 3 additions & 3 deletions app/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -13869,13 +13869,13 @@ __metadata:
linkType: hard

"postcss@npm:^8.4.43":
version: 8.4.44
resolution: "postcss@npm:8.4.44"
version: 8.4.45
resolution: "postcss@npm:8.4.45"
dependencies:
nanoid: ^3.3.7
picocolors: ^1.0.1
source-map-js: ^1.2.0
checksum: 64d9ce78253696bb64e608a54b362c9ddb537d3b38b58223ebce8260d6110d4e798ef1b3d57d8c28131417d9809187fd51d5c4263113536363444f8635e11bdb
checksum: 3223cdad4a9392c0b334ee3ee7e4e8041c631cb6160609cef83c18d2b2580e931dd8068ab13cc6000c1a254d57492ac6c38717efc397c5dcc9756d06bc9c44f3
languageName: node
linkType: hard

Expand Down
14 changes: 14 additions & 0 deletions fiftyone/operators/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -2399,6 +2399,20 @@ def to_json(self):
}


class FrameLoaderView(View):
"""Utility for loading frames and animated panels.

Args:
timeline_id (None): the ID of the timeline to load
on_load (None): the operator to execute when the frame is loaded
on_error (None): the operator to execute when the frame fails to load
on_load_range (None): the operator to execute when the frame is loading
"""

def __init__(self, **kwargs):
super().__init__(**kwargs)


class Container(BaseType):
"""Represents a base container for a container types."""

Expand Down
Loading