Skip to content

Conversation

@vvihorev
Copy link
Contributor

@vvihorev vvihorev commented May 7, 2025

Problem

In DAR-5605 we started properly handling the case when attributes are unset during import, by passing an empty list of attributes in the payload for the unset frame.

In DAR-5903 we made sure unsetting attributes by passing an empty list happened only to keyframes, not to all frames.

The remaining problem is:

  • we have keyframes 1 and 3, 4 where the annotation changes.
  • we have key subframes 2 and 3, where the attributes change.
  • during export we don't differentiate between frames and subframes, and end up getting key frames 1, 2, 3, 4.
  • on re-import we create both keyframes and key subframes 1, 2, 3, 4.
--- Original item
    key frames: 1 _ 3 4
key sub-frames: _ 2 3 _

--- Exported item
    key frames: 1 2 3 4
key sub-frames: 1 2 3 4

--- Imported item (actual)
    key frames: 1 2 3 4
key sub-frames: 1 2 3 4

--- Imported item (expected)
    key frames: 1 2 3 4
key sub-frames: _ 2 3 _

Changes in attributes between subframes are presented correctly, however the user ends up with more key subframes than in the original item: 2, 3 vs 1, 2, 3, 4.

Notice that keyframes are created even for key subframes in the original item: if we change an attribute, and the annotation itself doesn't change, it creates a key subframe, but after export-import we get both a key frame and a key subframe (see 2 in the example).

Solution

When importing annotations we avoid storing attributes in a keyframe (creating a key subframe) if the attributes have not changed since last keyframe.

All attributes in the frame are treated as a set. If the set changes, the whole set is included in the payload for the keyframe, if the set remains the same nothing is included for the keyframe. This way we avoid creating extra key subframes where not neccessary.

Changelog

Fix annotation attributes not populating correctly upon re-import, create key subframes only if the attributes change.

Copilot AI review requested due to automatic review settings May 7, 2025 10:12
@linear
Copy link

linear bot commented May 7, 2025

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where attribute data was being stored on keyframes unnecessarily during import, leading to duplicate key subframes. The changes update both the test expectations and the payload processing so that attributes are only stored if they have changed compared to the previous keyframe.

  • Updated test cases to check that duplicate attribute data are omitted.
  • Modified payload generation in datatypes.py to remove the attributes field when it remains unchanged.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/darwin/importer/importer_test.py Updated test function name and assertions to reflect desired behavior
darwin/datatypes.py Added logic to remove duplicate attribute data in keyframes

vvihorev and others added 2 commits May 7, 2025 12:14
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

@AndriiKlymchuk AndriiKlymchuk left a comment

Choose a reason for hiding this comment

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

I think that the same issue also applies to other sub annotation types like text and directional vector. Would be good to make behavior consistent across all of them.

@vvihorev vvihorev requested a review from AndriiKlymchuk May 9, 2025 07:18
@vvihorev vvihorev merged commit 45de428 into master May 9, 2025
32 checks passed
@vvihorev vvihorev deleted the dar-6533-bug-annotation-attributes-not-populating-correctly-upon-re branch May 9, 2025 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants