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 timeline view #4965

Merged
merged 9 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { usePanelId, useSetPanelStateById } from "@fiftyone/spaces";
import { useTimeline } from "@fiftyone/playback/src/lib/use-timeline";
import _ from "lodash";

const FRAME_LOADED_EVENT = "frames-loaded";

export default function FrameLoaderView(props: ViewPropsType) {
const { schema, path, data } = props;
const { view = {} } = schema;
Expand All @@ -16,15 +18,16 @@ export default function FrameLoaderView(props: ViewPropsType) {
const setPanelState = useSetPanelStateById(true);
const localIdRef = React.useRef<string>();
const bufm = useRef(new BufferManager());
const frameDataRef = useRef<typeof data.frames>(null);

useEffect(() => {
localIdRef.current = Math.random().toString(36).substring(7);
if (data?.frames)
window.dispatchEvent(
new CustomEvent(`frames-loaded`, {
detail: { localId: localIdRef.current },
})
);
if (data?.frames) frameDataRef.current = data.frames;
window.dispatchEvent(
new CustomEvent(FRAME_LOADED_EVENT, {
detail: { localId: localIdRef.current },
})
);
Comment on lines +25 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing dependency and cleanup to useEffect.

The effect should include data.frames in its dependency array and implement a cleanup function.

Apply this diff:

 useEffect(() => {
   localIdRef.current = Math.random().toString(36).substring(7);
   if (data?.frames) frameDataRef.current = data.frames;
   window.dispatchEvent(
     new CustomEvent(FRAME_LOADED_EVENT, {
       detail: { localId: localIdRef.current },
     })
   );
+  return () => {
+    frameDataRef.current = null;
+  };
-}, [data?.signature]);
+}, [data?.signature, data?.frames]);
📝 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
if (data?.frames) frameDataRef.current = data.frames;
window.dispatchEvent(
new CustomEvent(FRAME_LOADED_EVENT, {
detail: { localId: localIdRef.current },
})
);
useEffect(() => {
localIdRef.current = Math.random().toString(36).substring(7);
if (data?.frames) frameDataRef.current = data.frames;
window.dispatchEvent(
new CustomEvent(FRAME_LOADED_EVENT, {
detail: { localId: localIdRef.current },
})
);
return () => {
frameDataRef.current = null;
};
}, [data?.signature, data?.frames]);

}, [data?.signature]);

const loadRange = React.useCallback(
Expand All @@ -41,15 +44,22 @@ export default function FrameLoaderView(props: ViewPropsType) {
}

return new Promise<void>((resolve) => {
window.addEventListener(`frames-loaded`, (e) => {
if (
e instanceof CustomEvent &&
e.detail.localId === localIdRef.current
) {
bufm.current.addNewRange(range);
resolve();
}
});
if (frameDataRef.current) {
bufm.current.addNewRange(range);
resolve();
} else {
const onFramesLoaded = (e) => {
if (
e instanceof CustomEvent &&
e.detail.localId === localIdRef.current
) {
window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded);
bufm.current.addNewRange(range);
resolve();
}
};
window.addEventListener(FRAME_LOADED_EVENT, onFramesLoaded);
}
Comment on lines +47 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve event listener typing and cleanup pattern.

While the event listener cleanup is implemented, there are opportunities for improvement in typing and pattern usage.

Apply this diff for better type safety and a more robust cleanup pattern:

 if (frameDataRef.current) {
   bufm.current.addNewRange(range);
   resolve();
 } else {
-  const onFramesLoaded = (e) => {
+  const onFramesLoaded = (e: CustomEvent<{ localId: string }>) => {
+    if (e.detail.localId !== localIdRef.current) return;
+    
+    window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded as EventListener);
+    bufm.current.addNewRange(range);
+    resolve();
+  };
-    if (
-      e instanceof CustomEvent &&
-      e.detail.localId === localIdRef.current
-    ) {
-      window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded);
-      bufm.current.addNewRange(range);
-      resolve();
-    }
-  };
   window.addEventListener(FRAME_LOADED_EVENT, onFramesLoaded as EventListener);
 }
📝 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
if (frameDataRef.current) {
bufm.current.addNewRange(range);
resolve();
} else {
const onFramesLoaded = (e) => {
if (
e instanceof CustomEvent &&
e.detail.localId === localIdRef.current
) {
window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded);
bufm.current.addNewRange(range);
resolve();
}
};
window.addEventListener(FRAME_LOADED_EVENT, onFramesLoaded);
}
if (frameDataRef.current) {
bufm.current.addNewRange(range);
resolve();
} else {
const onFramesLoaded = (e: CustomEvent<{ localId: string }>) => {
if (e.detail.localId !== localIdRef.current) return;
window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded as EventListener);
bufm.current.addNewRange(range);
resolve();
};
window.addEventListener(FRAME_LOADED_EVENT, onFramesLoaded as EventListener);
}

});
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,6 @@ export default function PlotlyView(props: ViewPropsType) {
return (
<Box
{...getComponentProps(props, "container")}
useResizeHandler
sx={{ height: "100%", width: "100%" }}
>
<HeaderView {...props} nested />
Expand Down
43 changes: 43 additions & 0 deletions app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import React, { useMemo } from "react";
import { Timeline, useCreateTimeline, useTimeline } from "@fiftyone/playback";
import { ViewPropsType } from "../utils/types";

const DEFAULT_CONFIG = { loop: false };

export default function TimelineView(props: ViewPropsType) {
const { schema } = props;
const { view = {} } = schema;
const { timeline_name, loop, total_frames } = view;

const providedConfig = {
loop,
totalFrames: total_frames,
};
ritch marked this conversation as resolved.
Show resolved Hide resolved

const finalConfig = useMemo(
() => ({ ...DEFAULT_CONFIG, ...providedConfig }),
[providedConfig]
);
if (!timeline_name) {
throw new Error("Timeline name is required");
}
if (!finalConfig.totalFrames) {
throw new Error("Total frames is required");
}

return <TimelineCreator timelineName={timeline_name} {...finalConfig} />;
}
ritch marked this conversation as resolved.
Show resolved Hide resolved

export const TimelineCreator = ({ timelineName, totalFrames, loop }) => {
const config = useMemo(() => ({ totalFrames, loop }), [totalFrames, loop]);
const { isTimelineInitialized } = useCreateTimeline({
name: timelineName,
config,
});

if (!isTimelineInitialized) {
return null;
}
Comment on lines +38 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance loading state feedback

Returning null during initialization provides no feedback to users. Consider showing a loading indicator or message.

   if (!isTimelineInitialized) {
-    return null;
+    return <div aria-busy="true" role="status">Initializing timeline...</div>;
   }
📝 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
if (!isTimelineInitialized) {
return null;
}
if (!isTimelineInitialized) {
return <div aria-busy="true" role="status">Initializing timeline...</div>;
}


return <Timeline name={timelineName} />;
};
ritch marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 3 additions & 2 deletions app/packages/core/src/plugins/SchemaIO/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ export { default as ListView } from "./ListView";
export { default as LoadingView } from "./LoadingView";
export { default as MapView } from "./MapView";
export { default as MarkdownView } from "./MarkdownView";
export { default as ModalView } from "./ModalView";
export { default as MediaPlayerView } from "./MediaPlayerView";
export { default as ModalView } from "./ModalView";
export { default as ObjectView } from "./ObjectView";
export { default as OneOfView } from "./OneOfView";
export { default as PillBadgeView } from "./PillBadgeView";
Expand All @@ -48,8 +48,9 @@ export { default as SwitchView } from "./SwitchView";
export { default as TableView } from "./TableView";
export { default as TabsView } from "./TabsView";
export { default as TagsView } from "./TagsView";
export { default as TextView } from "./TextView";
export { default as TextFieldView } from "./TextFieldView";
export { default as TextView } from "./TextView";
export { default as TimelineView } from "./TimelineView";
export { default as ToastView } from "./ToastView";
export { default as TupleView } from "./TupleView";
export { default as UnsupportedView } from "./UnsupportedView";
13 changes: 13 additions & 0 deletions fiftyone/operators/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -2574,6 +2574,19 @@ def __init__(self, **kwargs):
super().__init__(**kwargs)


class TimelineView(View):
"""Represents a timeline for playing animations.

Args:
timeline_name (None): the name of the timeline
total_frames (None): the total number of frames in the timeline
loop (False): whether to loop the timeline
"""

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


ritch marked this conversation as resolved.
Show resolved Hide resolved
class Container(BaseType):
"""Represents a base container for a container types."""

Expand Down
Loading