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.

Sign up for GitHub

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

Jump to bottom

Merge release/v1.0.1 to develop #4894

Merged
merged 14 commits into from
Oct 5, 2024
Merged

Merge release/v1.0.1 to develop #4894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f08b8cc
fix COCO category IDs
brimoor Oct 2, 2024
221d807
fixing add_coco_labels
brimoor Oct 3, 2024
17e8e53
Merge pull request #4884 from voxel51/bugfix/iss-4615-4795
brimoor Oct 4, 2024
e416f13
Merge branch 'release/v1.0.1' of https://github.com/voxel51/fiftyone …
voxel51-bot Oct 4, 2024
69a89e8
Query performance operators (#4875)
benjaminpkane Oct 4, 2024
97ccc02
Add utils for supporting rerun panel plugin (#4876)
sashankaryal Oct 4, 2024
54ce128
Video playback (#4878)
benjaminpkane Oct 4, 2024
ac6f451
Add utils for supporting rerun panel plugin (#4876)
sashankaryal Oct 4, 2024
35c13f7
Video playback (#4878)
benjaminpkane Oct 4, 2024
a2d26f0
wait until buffering is done (#4892)
sashankaryal Oct 4, 2024
10d303c
Merge branch 'merge/release/v1.0.1' of https://github.com/voxel51/fif…
voxel51-bot Oct 4, 2024
e4f1ca4
Merge branch 'release/v1.0.1' of https://github.com/voxel51/fiftyone …
voxel51-bot Oct 4, 2024
46c1929
externalize fiftyone plugins too (#4897)
sashankaryal Oct 5, 2024
44b6594
Merge branch 'release/v1.0.1' of https://github.com/voxel51/fiftyone …
voxel51-bot Oct 5, 2024
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
25 changes: 7 additions & 18 deletions app/packages/core/src/components/Modal/ImaVidLooker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@ import React, {
import { useErrorHandler } from "react-error-boundary";
import { useRecoilValue, useSetRecoilState } from "recoil";
import { v4 as uuid } from "uuid";
import { useInitializeImaVidSubscriptions, useModalContext } from "./hooks";
import { useClearSelectedLabels, useShowOverlays } from "./ModalLooker";
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

Update import path for hooks

useClearSelectedLabels and useShowOverlays should be imported from "./hooks" instead of "./ModalLooker" to reflect their new location.

Apply this diff to fix the import path:

-import { useClearSelectedLabels, useShowOverlays } from "./ModalLooker";
+import { useClearSelectedLabels, useShowOverlays } from "./hooks";
📝 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
import { useClearSelectedLabels, useShowOverlays } from "./ModalLooker";
import { useClearSelectedLabels, useShowOverlays } from "./hooks";

import {
shortcutToHelpItems,
useClearSelectedLabels,
useInitializeImaVidSubscriptions,
useLookerOptionsUpdate,
useShowOverlays,
} from "./ModalLooker";
useModalContext,
} from "./hooks";
import useKeyEvents from "./use-key-events";
import { shortcutToHelpItems } from "./utils";

interface ImaVidLookerReactProps {
sample: fos.ModalSample;
Expand Down Expand Up @@ -132,19 +133,7 @@ export const ImaVidLookerReact = React.memo(

useEventHandler(looker, "clear", useClearSelectedLabels());

const hoveredSample = useRecoilValue(fos.hoveredSample);

useEffect(() => {
const hoveredSampleId = hoveredSample?._id;
looker.updater((state) => ({
...state,
// todo: always setting it to true might not be wise
shouldHandleKeyEvents: true,
options: {
...state.options,
},
}));
}, [hoveredSample, sample, looker]);
useKeyEvents(initialRef, sample._id, looker);

const ref = useRef<HTMLDivElement>(null);
useEffect(() => {
Expand Down
203 changes: 39 additions & 164 deletions app/packages/core/src/components/Modal/ModalLooker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,12 @@
import { useTheme } from "@fiftyone/components";
import { AbstractLooker } from "@fiftyone/looker";
import { BaseState } from "@fiftyone/looker/src/state";
import type { ImageLooker } from "@fiftyone/looker";
import * as fos from "@fiftyone/state";
import { useEventHandler, useOnSelectLabel } from "@fiftyone/state";
import React, { useEffect, useMemo, useRef, useState } from "react";
import { useErrorHandler } from "react-error-boundary";
import React, { useEffect, useMemo } from "react";
import { useRecoilCallback, useRecoilValue, useSetRecoilState } from "recoil";
import { v4 as uuid } from "uuid";
import { useModalContext } from "./hooks";
import { ImaVidLookerReact } from "./ImaVidLooker";

export const useLookerOptionsUpdate = () => {
return useRecoilCallback(
({ snapshot, set }) =>
async (update: object, updater?: (updated: {}) => void) => {
const currentOptions = await snapshot.getPromise(
fos.savedLookerOptions
);

const panels = await snapshot.getPromise(fos.lookerPanels);
const updated = {
...currentOptions,
...update,
showJSON: panels.json.isOpen,
showHelp: panels.help.isOpen,
};
set(fos.savedLookerOptions, updated);
if (updater) updater(updated);
}
);
};
import { VideoLookerReact } from "./VideoLooker";
import { useModalContext } from "./hooks";
import useLooker from "./use-looker";

export const useShowOverlays = () => {
return useRecoilCallback(({ set }) => async (event: CustomEvent) => {
Expand All @@ -47,137 +24,40 @@ export const useClearSelectedLabels = () => {
};

interface LookerProps {
sample?: fos.ModalSample;
onClick?: React.MouseEventHandler<HTMLDivElement>;
sample: fos.ModalSample;
}

const ModalLookerNoTimeline = React.memo(
({ sample: sampleDataWithExtraParams }: LookerProps) => {
const [id] = useState(() => uuid());
const colorScheme = useRecoilValue(fos.colorScheme);

const { sample } = sampleDataWithExtraParams;

const theme = useTheme();
const initialRef = useRef<boolean>(true);
const lookerOptions = fos.useLookerOptions(true);
const [reset, setReset] = useState(false);
const selectedMediaField = useRecoilValue(fos.selectedMediaField(true));
const setModalLooker = useSetRecoilState(fos.modalLooker);

const createLooker = fos.useCreateLooker(true, false, {
...lookerOptions,
});

const { setActiveLookerRef } = useModalContext();

const looker = React.useMemo(
() => createLooker.current(sampleDataWithExtraParams),
[reset, createLooker, selectedMediaField]
) as AbstractLooker<BaseState>;

useEffect(() => {
setModalLooker(looker);
}, [looker]);

useEffect(() => {
if (looker) {
setActiveLookerRef(looker as fos.Lookers);
}
}, [looker]);

useEffect(() => {
!initialRef.current && looker.updateOptions(lookerOptions);
}, [lookerOptions]);

useEffect(() => {
!initialRef.current && looker.updateSample(sample);
}, [sample, colorScheme]);

useEffect(() => {
return () => looker?.destroy();
}, [looker]);

const handleError = useErrorHandler();
const ModalLookerNoTimeline = React.memo((props: LookerProps) => {
const { id, looker, ref } = useLooker<ImageLooker>(props);
const theme = useTheme();
const setModalLooker = useSetRecoilState(fos.modalLooker);

const updateLookerOptions = useLookerOptionsUpdate();
useEventHandler(looker, "options", (e) => updateLookerOptions(e.detail));
useEventHandler(looker, "showOverlays", useShowOverlays());
useEventHandler(looker, "reset", () => {
setReset((c) => !c);
});
const { setActiveLookerRef } = useModalContext();

const jsonPanel = fos.useJSONPanel();
const helpPanel = fos.useHelpPanel();
useEffect(() => {
setModalLooker(looker);
}, [looker, setModalLooker]);

useEventHandler(looker, "select", useOnSelectLabel());
useEventHandler(looker, "error", (event) => handleError(event.detail));
useEventHandler(
looker,
"panels",
async ({ detail: { showJSON, showHelp, SHORTCUTS } }) => {
if (showJSON) {
jsonPanel[showJSON](sample);
}
if (showHelp) {
if (showHelp == "close") {
helpPanel.close();
} else {
helpPanel[showHelp](shortcutToHelpItems(SHORTCUTS));
}
}

updateLookerOptions({}, (updatedOptions) =>
looker.updateOptions(updatedOptions)
);
}
);

useEffect(() => {
initialRef.current = false;
}, []);

useEffect(() => {
looker.attach(id);
}, [looker, id]);

useEventHandler(looker, "clear", useClearSelectedLabels());

const hoveredSample = useRecoilValue(fos.hoveredSample);

useEffect(() => {
const hoveredSampleId = hoveredSample?._id;
looker.updater((state) => ({
...state,
shouldHandleKeyEvents: hoveredSampleId === sample._id,
options: {
...state.options,
},
}));
}, [hoveredSample, sample, looker]);

const ref = useRef<HTMLDivElement>(null);
useEffect(() => {
ref.current?.dispatchEvent(
new CustomEvent(`looker-attached`, { bubbles: true })
);
}, [ref]);

return (
<div
ref={ref}
id={id}
data-cy="modal-looker-container"
style={{
width: "100%",
height: "100%",
background: theme.background.level2,
position: "relative",
}}
/>
);
}
);
useEffect(() => {
if (looker) {
setActiveLookerRef(looker as fos.Lookers);
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

Review type assertion from ImageLooker to fos.Lookers.

The type assertion looker as fos.Lookers suggests a possible mismatch between ImageLooker and fos.Lookers. If ImageLooker is a subtype of fos.Lookers, you can remove the type assertion to enhance type safety.

Apply this diff to remove the unnecessary type assertion:

- setActiveLookerRef(looker as fos.Lookers);
+ setActiveLookerRef(looker);

If the types are incompatible, consider updating the type definitions or ensuring that looker is correctly typed to avoid casting.

📝 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
setActiveLookerRef(looker as fos.Lookers);
setActiveLookerRef(looker);

}
}, [looker, setActiveLookerRef]);

return (
<div
ref={ref}
id={id}
data-cy="modal-looker-container"
style={{
width: "100%",
height: "100%",
background: theme.background.level2,
position: "relative",
}}
/>
);
});

export const ModalLooker = React.memo(
({ sample: propsSampleData }: LookerProps) => {
Expand All @@ -197,21 +77,16 @@ export const ModalLooker = React.memo(
const shouldRenderImavid = useRecoilValue(
fos.shouldRenderImaVidLooker(true)
);
const video = useRecoilValue(fos.isVideoDataset);

if (shouldRenderImavid) {
return <ImaVidLookerReact sample={sample} />;
}

if (video) {
return <VideoLookerReact sample={sample} />;
}

return <ModalLookerNoTimeline sample={sample} />;
}
);

export function shortcutToHelpItems(SHORTCUTS) {
return Object.values(
Object.values(SHORTCUTS).reduce((acc, v) => {
acc[v.shortcut] = v;

return acc;
}, {})
);
}
80 changes: 80 additions & 0 deletions app/packages/core/src/components/Modal/VideoLooker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import { useTheme } from "@fiftyone/components";
import type { VideoLooker } from "@fiftyone/looker";
import { getFrameNumber } from "@fiftyone/looker";
import {
useCreateTimeline,
useDefaultTimelineNameImperative,
useTimeline,
} from "@fiftyone/playback";
import * as fos from "@fiftyone/state";
import React, { useEffect, useMemo, useState } from "react";
import useLooker from "./use-looker";

interface VideoLookerReactProps {
sample: fos.ModalSample;
}

export const VideoLookerReact = (props: VideoLookerReactProps) => {
const theme = useTheme();
const { id, looker, sample } = useLooker<VideoLooker>(props);
const [totalFrames, setTotalFrames] = useState<number>();
const frameRate = useMemo(() => {
return sample.frameRate;
}, [sample]);

useEffect(() => {
const load = () => {
const duration = looker.getVideo().duration;
setTotalFrames(getFrameNumber(duration, duration, frameRate));
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

Possible incorrect parameters in getFrameNumber function call

In line 28, getFrameNumber is called with duration passed twice:

setTotalFrames(getFrameNumber(duration, duration, frameRate));

Please verify whether using duration for both the time and duration parameters is intentional. If getFrameNumber expects different values for time and duration, this might lead to incorrect frame calculations.

looker.removeEventListener("load", load);
};
looker.addEventListener("load", load);
}, [frameRate, looker]);
Comment on lines +25 to +32
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

Ensure proper cleanup of event listeners in useEffect

When adding event listeners in useEffect, it's important to remove them in the cleanup function to prevent memory leaks or unintended behavior. Consider modifying the useEffect to clean up the event listener when the component unmounts.

Here's a suggested change:

 useEffect(() => {
   const load = () => {
     const duration = looker.getVideo().duration;
     setTotalFrames(getFrameNumber(duration, duration, frameRate));
     looker.removeEventListener("load", load);
   };
   looker.addEventListener("load", load);
+  return () => {
+    looker.removeEventListener("load", load);
+  };
 }, [frameRate, looker]);
📝 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
useEffect(() => {
const load = () => {
const duration = looker.getVideo().duration;
setTotalFrames(getFrameNumber(duration, duration, frameRate));
looker.removeEventListener("load", load);
};
looker.addEventListener("load", load);
}, [frameRate, looker]);
useEffect(() => {
const load = () => {
const duration = looker.getVideo().duration;
setTotalFrames(getFrameNumber(duration, duration, frameRate));
looker.removeEventListener("load", load);
};
looker.addEventListener("load", load);
return () => {
looker.removeEventListener("load", load);
};
}, [frameRate, looker]);


return (
<>
<div
id={id}
data-cy="modal-looker-container"
style={{
width: "100%",
height: "100%",
background: theme.background.level2,
position: "relative",
}}
/>
{totalFrames !== undefined && (
<TimelineController looker={looker} totalFrames={totalFrames} />
)}
</>
);
};

const TimelineController = ({
looker,
totalFrames,
}: {
looker: VideoLooker;
totalFrames: number;
}) => {
Comment on lines +53 to +59
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

Define a separate interface for TimelineController props

For improved readability and maintainability, consider defining an explicit interface for the TimelineController component's props instead of using inline typing.

Here's how you could define the props interface:

interface TimelineControllerProps {
  looker: VideoLooker;
  totalFrames: number;
}

const TimelineController = ({ looker, totalFrames }: TimelineControllerProps) => {
  // component logic
};

const { getName } = useDefaultTimelineNameImperative();
const timelineName = React.useMemo(() => getName(), [getName]);

useCreateTimeline({
name: timelineName,
config: totalFrames
? {
totalFrames,
loop: true,
}
: undefined,
optOutOfAnimation: true,
});

const { pause, play } = useTimeline(timelineName);

fos.useEventHandler(looker, "pause", pause);
fos.useEventHandler(looker, "play", play);

return null;
};
21 changes: 21 additions & 0 deletions app/packages/core/src/components/Modal/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,27 @@ export const useLookerHelpers = () => {
};
};

export const useLookerOptionsUpdate = () => {
return useRecoilCallback(
({ snapshot, set }) =>
async (update: object, updater?: (updated: {}) => void) => {
const currentOptions = await snapshot.getPromise(
fos.savedLookerOptions
);

const panels = await snapshot.getPromise(fos.lookerPanels);
const updated = {
...currentOptions,
...update,
showJSON: panels.json.isOpen,
showHelp: panels.help.isOpen,
};
set(fos.savedLookerOptions, updated);
if (updater) updater(updated);
}
);
};

export const useInitializeImaVidSubscriptions = () => {
const subscribeToImaVidStateChanges = useRecoilCallback(
({ set }) =>
Expand Down
Loading
Loading