Fixed frames navigation and GT tracks displaying #8533
Conversation
WalkthroughThe changes introduced in this pull request focus on enhancing navigation and frame management within the annotation application. Key updates include fixes for navigation by keyframes and track display issues, modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Actions
participant Reducer
participant State
User->>UI: Request to change frame
UI->>Actions: Validate frame change
Actions->>Reducer: Update state with new frame
Reducer->>State: Reflect new frame in state
State-->>UI: Update UI with new frame
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
changelog.d/20241011_142625_sekachev.bs_fixed_navigation.md (2)
3-4: Update the PR number in the changelog entry.The changelog entry accurately describes the fix for incorrect navigation by keyframes. However, the PR number is missing and needs to be updated.
Please replace 'XXXX' with the actual PR number (8533) in the URL:
- (<https://github.com/cvat-ai/cvat/pull/XXXX>) + (<https://github.com/cvat-ai/cvat/pull/8533>)
5-6: Update the PR number in the changelog entry.The changelog entry accurately describes the fix for incorrect display of tracks from ground truth jobs. However, the PR number is missing and needs to be updated.
Please replace 'XXXX' with the actual PR number (8533) in the URL:
- (<https://github.com/cvat-ai/cvat/pull/XXXX>) + (<https://github.com/cvat-ai/cvat/pull/8533>)cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-buttons.tsx (1)
214-217: LGTM! Consider adding error handling.The change to
isAbleToChangeFrame(frame)is a good improvement, as it now checks if the specific frame can be changed. This aligns well with the PR objective of fixing frame navigation issues.Consider adding error handling or user feedback when
isAbleToChangeFrame(frame)returns false. For example:private changeFrame(frame: number): void { const { changeFrame } = this.props; if (isAbleToChangeFrame(frame)) { changeFrame(frame); } else { // Consider adding a user-friendly message or logging here console.warn(`Unable to change to frame ${frame}`); } }This would provide better feedback and potentially help with debugging if frame changes are unexpectedly blocked.
cvat-ui/src/components/annotation-page/tag-annotation-workspace/tag-annotation-sidebar/tag-annotation-sidebar.tsx (1)
209-211: Improved frame navigation logic, consider adding error handling.The changes to the
onChangeFramefunction improve the frame navigation logic, especially when dealing with deleted frames. The use ofjobInstance.frames.searchprovides more precise control over frame selection.However, consider the following improvements:
- Add error handling for the
jobInstance.frames.searchcall to gracefully handle any potential issues.- Update any relevant tests to reflect the new asynchronous nature of this function.
- Consider adding a comment explaining the logic behind the frame search, particularly the use of the
showDeletedFramesflag.Here's a suggested implementation with error handling:
const onChangeFrame = async (): Promise<void> => { try { // Search for the next frame, skipping deleted frames if showDeletedFrames is false const frame = await jobInstance.frames.search( { notDeleted: !showDeletedFrames }, frameNumber + 1, jobInstance.stopFrame, ); if (frame !== null && isAbleToChangeFrame(frame)) { changeFrame(frame); } } catch (error) { // Handle the error appropriately, e.g., show a notification to the user console.error('Error while changing frame:', error); // You might want to add a user-friendly error message here } };cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/objects-list.tsx (1)
579-580: Improved frame navigation logicThe changes to the
NEXT_KEY_FRAMEandPREV_KEY_FRAMEhandlers enhance the frame navigation by checking if it's possible to change to a specific frame. This improvement aligns with the PR objective of fixing frames navigation.Consider extracting the common logic for both handlers into a separate function to reduce code duplication:
const changeKeyFrame = (frameGetter: () => number | null) => { const state = activatedState(); if (state && state.keyframes) { const frame = frameGetter(); if (frame !== null && isAbleToChangeFrame(frame)) { changeFrame(frame); } } }; // Usage: NEXT_KEY_FRAME: (event: KeyboardEvent | undefined) => { preventDefault(event); changeKeyFrame(() => { const state = activatedState(); return state?.keyframes?.next ?? null; }); }, PREV_KEY_FRAME: (event: KeyboardEvent | undefined) => { preventDefault(event); changeKeyFrame(() => { const state = activatedState(); return state?.keyframes?.prev ?? null; }); },This refactoring would improve code maintainability and reduce the risk of inconsistencies between the two handlers.
Also applies to: 589-590
cvat-ui/src/reducers/index.ts (1)
725-725: LGTM! Consider using optional chaining for null safety.The addition of the
metaproperty to thejobobject is a good enhancement for storing frame metadata. This change aligns well with the PR objectives of improving frame management and navigation.To improve null safety when accessing this property, consider using optional chaining in the code that consumes this state. For example:
const frameMeta = state.annotation.job.meta?.someProperty;This approach will help prevent potential null reference errors in the application.
cvat-core/src/frames.ts (1)
Line range hint
1-1024: Summary of changes and potential impactThe main changes in this file revolve around making the
includedFramesproperty nullable and handling this new possibility in thegetFrameIndexmethod. These changes appear to be consistent and well-implemented.However, it's important to consider the following:
- The reason for allowing
includedFramesto be null is not immediately clear from the code. It would be helpful to add a comment explaining the scenarios where this might occur.- While the
getFrameIndexmethod handles the null case, other parts of the codebase that useincludedFramesmight need to be updated to handle this new possibility.Consider adding a brief comment near the
includedFramesproperty declaration explaining why it can now be null. This will help future developers understand the design decision.Additionally, it might be beneficial to run a thorough test suite that covers various scenarios with both null and non-null
includedFramesto ensure the changes don't introduce any regressions.cvat-ui/src/utils/is-able-to-change-frame.ts (2)
9-9: Update function documentation to include the new parameterSince
isAbleToChangeFramenow accepts an optionalframeparameter, please update the function's documentation or JSDoc comments to reflect this change. This will help other developers understand the purpose and usage of the new parameter.
23-23: Fix typo in commentThere's a typo in the comment on line 23. The word "hovewer" should be corrected to "however" for clarity.
Apply this diff to correct the typo:
- // hovewer includedFrames contains absolute values, so, we need to adjust it with the frameStep + // however includedFrames contains absolute values, so we need to adjust it with the frameStep
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- changelog.d/20241011_142625_sekachev.bs_fixed_navigation.md (1 hunks)
- cvat-core/src/frames.ts (1 hunks)
- cvat-ui/src/actions/annotation-actions.ts (3 hunks)
- cvat-ui/src/components/annotation-page/attribute-annotation-workspace/attribute-annotation-sidebar/attribute-annotation-sidebar.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/tag-annotation-workspace/tag-annotation-sidebar/tag-annotation-sidebar.tsx (1 hunks)
- cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-buttons.tsx (1 hunks)
- cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/objects-list.tsx (2 hunks)
- cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx (3 hunks)
- cvat-ui/src/reducers/annotation-reducer.ts (3 hunks)
- cvat-ui/src/reducers/index.ts (1 hunks)
- cvat-ui/src/utils/filter-annotations.ts (1 hunks)
- cvat-ui/src/utils/is-able-to-change-frame.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (18)
changelog.d/20241011_142625_sekachev.bs_fixed_navigation.md (1)
1-6: Excellent changelog entries, please double-check PR numbers.The changelog entries are well-written, clear, and accurately reflect the fixes implemented in this PR. They align perfectly with the PR objectives of addressing issues related to frame navigation and GT tracks displaying.
Before merging, please ensure that:
- Both PR numbers are correctly updated to 8533.
- The descriptions accurately reflect all the changes made in the PR.
Run the following script to verify the PR number:
cvat-ui/src/utils/filter-annotations.ts (1)
35-39: Improved ground truth track filtering logicThe changes to the conditional logic for ground truth tracks are well-considered and appear to address potential issues with frame numbering. The additional check for
includedFramesand the new calculation for frame inclusion should improve the accuracy of the filtering.However, I have a few suggestions and questions:
- Consider adding a null check for
frameStepto prevent potential runtime errors:if (groundTruthJobFramesMeta.frameStep != null) { return groundTruthJobFramesMeta.includedFrames.includes(frame * groundTruthJobFramesMeta.frameStep); } else { console.warn('frameStep is null or undefined'); return false; }
- Could you please clarify the concept of "absolute numeration" mentioned in the comment? This would help future developers understand the reasoning behind the
frame * groundTruthJobFramesMeta.frameStepcalculation.To verify the usage of
frameStep, let's run the following script:cvat-ui/src/components/annotation-page/attribute-annotation-workspace/attribute-annotation-sidebar/attribute-annotation-sidebar.tsx (3)
319-321: Improved frame change validation for next keyframeThe modification to pass the
frameparameter toisAbleToChangeFrameenhances the accuracy of frame navigation. This change ensures that the specific next keyframe is validated before changing the frame, which aligns with the PR's objective of fixing frame navigation issues.
329-331: Consistent frame change validation for previous keyframeThis change mirrors the improvement made to the next keyframe handler, ensuring consistent behavior for both forward and backward keyframe navigation. By passing the
frameparameter toisAbleToChangeFrame, we maintain symmetry in the validation process for both directions.
Line range hint
319-331: Overall improvement in keyframe navigationThe changes made to both the next and previous keyframe handlers represent a focused and consistent improvement in frame navigation. By passing the specific frame to be validated in
isAbleToChangeFrame, the code now ensures more accurate and reliable keyframe navigation. These modifications directly address the PR's objective of fixing frame navigation issues without introducing extensive changes to the existing codebase.cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/objects-list.tsx (1)
Line range hint
1-624: Summary of changesThe modifications in this file focus on improving the frame navigation logic in the
ObjectsListContainercomponent. The changes are minimal but impactful, addressing the PR objective of fixing frames navigation. The new implementation should prevent navigation to invalid frames, enhancing the overall user experience.The rest of the file remains unchanged, maintaining the existing functionality of the component. The changes are well-integrated and don't introduce any apparent issues or inconsistencies with the existing code.
cvat-core/src/frames.ts (2)
Line range hint
220-234: LGTM: Proper handling of nullincludedFramesThe changes in the
getFrameIndexmethod correctly handle the case whereincludedFramesis null. The fallback calculation usingstartFrameandframeStepis a good practice and aligns well with the type change ofincludedFrames.
47-47: Verify null checks forincludedFramesThe type of
includedFrameshas been changed to allow null values. This change might have implications in other parts of the code whereincludedFramesis used.To ensure this change doesn't introduce any null pointer exceptions, please run the following script to find all usages of
includedFramesand verify that appropriate null checks are in place:After running this script, please review each usage and ensure that null checks are added where necessary to handle cases where
includedFramesmight be null.✅ Verification successful
IncludedFrames nullability is appropriately handled
All usages of
includedFrameshave been reviewed, and necessary null checks are in place to handle cases whereincludedFramesmay be null, ensuring that the change does not introduce any null pointer exceptions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all usages of includedFrames rg "includedFrames" --type tsLength of output: 2357
cvat-ui/src/utils/is-able-to-change-frame.ts (2)
28-28: Confirmcanvas.isAbleToChangeFrame()behaves as expectedEnsure that the method
canvas.isAbleToChangeFrame()returns the correct boolean value under various conditions, and that it does not have side effects that could affect the application state.Consider reviewing the implementation of
canvas.isAbleToChangeFrame()to ensure its reliability.
24-25: Ensuremeta.frameStepis defined and validWhen using
meta.frameStepin the calculation, please verify thatmeta.frameStepis always defined and not zero to prevent potential errors or unexpected behavior.You can run the following script to check for any occurrences where
meta.frameStepmight be undefined or zero:✅ Verification successful
Verification Successful:
meta.frameStepis properly defined and non-zero across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `meta.frameStep` is always defined and not zero. # Test: Search for assignments to `meta.frameStep` and check its usage. # Expected: All assignments should result in a defined, non-zero value. rg --type ts --type js --context 5 'meta\.frameStep\s*=\s*(0|undefined|null)'Length of output: 79
cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx (3)
Line range hint
398-406: EnsureisAbleToChangeFrameacceptsframeparameterSimilarly, in the
onNextFramemethod,isAbleToChangeFrameis called withnewFrameas an argument. Please verify that the function definition supports this parameter.
636-638: EnsureisAbleToChangeFrameacceptsframeparameterIn the
changeFramemethod,isAbleToChangeFrameis now called withframeas an argument. Confirm thatisAbleToChangeFrameis updated to accept this parameter.
Line range hint
370-378: EnsureisAbleToChangeFrameacceptsframeparameterIn the updated code,
isAbleToChangeFrameis called withnewFrameas an argument. Please confirm that theisAbleToChangeFramefunction is defined to accept aframeparameter and handles it appropriately.To verify that
isAbleToChangeFrameaccepts aframeparameter, you can run the following script:✅ Verification successful
Verification Successful: The
isAbleToChangeFramefunction is defined with aframeparameter insrc/utils/is-able-to-change-frame.ts.
- File:
src/utils/is-able-to-change-frame.ts- Definition:
export default function isAbleToChangeFrame(frame?: number): boolean { // function implementation }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `isAbleToChangeFrame` function definition accepts a `frame` parameter. # Expected: The function should be defined with a `frame` parameter. rg --type typescript --multiline --pcre2 '^function isAbleToChangeFrame\(\s*frame' src/utils/is-able-to-change-frame.tsLength of output: 156
Script:
#!/bin/bash # Description: Verify that `isAbleToChangeFrame` function definition accepts a `frame` parameter using corrected ripgrep type. # Expected: The function should be defined with a `frame` parameter. rg --type ts --multiline --pcre2 '^function isAbleToChangeFrame\(\s*frame' src/utils/is-able-to-change-frame.tsLength of output: 190
Script:
#!/bin/bash # Description: Verify that `isAbleToChangeFrame` function definition accepts a `frame` parameter by targeting .ts files directly. # Expected: The function should be defined with a `frame` parameter. rg --multiline --pcre2 '^function isAbleToChangeFrame\(\s*frame' src/utils/is-able-to-change-frame.tsLength of output: 180
Script:
#!/bin/bash # Description: Verify that `isAbleToChangeFrame` function definition accepts a `frame` parameter using ast-grep. # Expected: The function should be defined with a `frame` parameter. ast-grep --lang typescript --pattern 'function isAbleToChangeFrame(frame: $_) { $$$ }' src/utils/is-able-to-change-frame.tsLength of output: 209
Script:
#!/bin/bash # Description: Search the entire codebase for the `isAbleToChangeFrame` function definition accepting a `frame` parameter using ripgrep. # Expected: Output the file(s) and line(s) where the function is defined with a `frame` parameter. rg --multiline --pcre2 '^function\s+isAbleToChangeFrame\(\s*frame\b' .Length of output: 72
Script:
#!/bin/bash # Description: Locate the `isAbleToChangeFrame` function definition with a `frame` parameter using ast-grep. # Expected: Output the function definition details if it exists. ast-grep --lang typescript --pattern 'function isAbleToChangeFrame(frame: $_) { $$$ }' .Length of output: 90
Script:
#!/bin/bash # Description: Search the entire codebase for any occurrences of `isAbleToChangeFrame` using ripgrep. # Expected: Output all file paths and lines where `isAbleToChangeFrame` is mentioned. rg 'isAbleToChangeFrame' .Length of output: 4171
Script:
#!/bin/bash # Description: Locate any definitions or references to `isAbleToChangeFrame` using ast-grep. # Expected: Output details of the function or method definition if it exists. ast-grep --lang typescript --pattern 'isAbleToChangeFrame($_) { $$$ }' .Length of output: 528
cvat-ui/src/reducers/annotation-reducer.ts (3)
70-70: Initializemetaproperty in job stateAdding
meta: nullto the defaultjobstate initializes themetaproperty appropriately.
162-162: IncludejobMetain destructured action payloadDestructuring
jobMetafrom the action payload ensures it is available for updating the state.
210-210: Updatejob.metawithjobMetaAssigning
meta: jobMetaupdates the job state with the new metadata received in the action payload.cvat-ui/src/actions/annotation-actions.ts (2)
963-963: EnsurejobMetais processed correctly in reducers and componentsWith
jobMetaadded to the payload in theGET_JOB_SUCCESSaction, verify that reducers and any components consuming this payload handlejobMetaappropriately, including scenarios where it might benulldue to a fetch error. This will prevent potential runtime errors ifjobMetais undefined or has unexpected values.Run the following script to search for usages of
jobMeta:
654-654: Verify thatisAbleToChangeFrameacceptstoFrameas an argumentThe function
isAbleToChangeFrameis now called withtoFrameas an argument. Please ensure thatisAbleToChangeFramehas been updated to accept this parameter and that all usages of this function have been revised accordingly.Run the following script to verify the function definition:
| @@ -914,6 +914,7 @@ export function getJobAsync({ | |||
| } | |||
| } | |||
|
|
|||
| const jobMeta = await cvat.frames.getMeta('job', job.id); | |||
There was a problem hiding this comment.
Add error handling for cvat.frames.getMeta call
The call to cvat.frames.getMeta('job', job.id) does not include error handling. If this call fails, it could lead to unhandled exceptions or affect the application's stability. Consider wrapping this call in a try-catch block to handle potential errors gracefully.
Apply this diff to add error handling:
+ let jobMeta;
+ try {
+ jobMeta = await cvat.frames.getMeta('job', job.id);
+ } catch (error) {
+ // Handle the error appropriately, e.g., set jobMeta to null or dispatch an error action
+ jobMeta = null;
+ // Optionally log the error
+ console.error('Error fetching job metadata:', error);
+ }📝 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.
| const jobMeta = await cvat.frames.getMeta('job', job.id); | |
| let jobMeta; | |
| try { | |
| jobMeta = await cvat.frames.getMeta('job', job.id); | |
| } catch (error) { | |
| // Handle the error appropriately, e.g., set jobMeta to null or dispatch an error action | |
| jobMeta = null; | |
| // Optionally log the error | |
| console.error('Error fetching job metadata:', error); | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8533 +/- ##
===========================================
- Coverage 74.24% 74.23% -0.02%
===========================================
Files 400 400
Lines 43207 43218 +11
Branches 3905 3909 +4
===========================================
Hits 32081 32081
- Misses 11126 11137 +11
|
| // includedFrames has absolute numeration of frames | ||
| // frame N actually corresponds to startFrame + N * frameStep in absolute numeration | ||
| const { frameStep, startFrame } = groundTruthJobFramesMeta; | ||
| const dataFrameNumber = frame * frameStep + startFrame; |
There was a problem hiding this comment.
It seems like we already have operations with transformation of relative/absolute frames here. Can we re-use it?
Also it seems they are duplicated inside of allocation table. Seems we need to refactor it.
There was a problem hiding this comment.
Probably you meant getDataFrameNumber as getDataStartFrame has another expression
There was a problem hiding this comment.
Yes, but it feels like we dont need multiple expressions for the same thing. The math behind it is the same. Its better to use something like:
const dataFrameNumber = getDataFrameNumber(meta, frame);
instead of writhing the transformation formula each time.
| // frame argument comes in relative job coordinates | ||
| // hovewer includedFrames contains absolute values, so, we need to adjust it with the frameStep and startFrame | ||
| frameInTheJob = meta.includedFrames ? | ||
| meta.includedFrames.includes(frame * meta.frameStep + meta.startFrame) : |
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Improvements