rerun-io / rerun

Visualize streams of multimodal data. Fast, easy to use, and simple to integrate. Built in Rust using egui.
https://rerun.io/
Apache License 2.0
6.42k stars 313 forks source link

VideoFrameReference should have an extension constructor for `seconds=` #7822

Open jleibs opened 4 days ago

jleibs commented 4 days ago

The default signature doesn't make it clear:

class VideoFrameReference(
    timestamp: VideoTimestampLike,
    *,
    video_reference: EntityPathLike | None = None
)

Create a new instance of the VideoFrameReference archetype.

Parameters
timestamp
References the closest video frame to this timestamp.

Note that this uses the closest video frame instead of the latest at this timestamp in order to be more forgiving of rounding errors for inprecise timestamp types.

I would recommend we disallow casting from int on VideoTimestampLike and add optional kwargs such as:

VideoFrameReference(seconds=12.4)
VideoFrameReference(nanos=124000000)
Wumpf commented 3 days ago

The constructor of VideoTimestamp already does the right thing:

    def __init__(
        self: Any,
        *,
        nanoseconds: Union[int, None] = None,
        seconds: Union[float, None] = None,
    ):

However, VideoTimestampLike is defined as VideoTimestampLike = Union[VideoTimestamp, int]. I think thius makes sense for VideoTimestampArrayLike because we want integer arrays to be easily convertible to timestamps (think send_columns!). I'd rather not make this asymmetrical. Also, tightening the type is sort-of a breaking change, thus shouldn't go into a patch..?

jleibs commented 3 days ago

I think the issue is that for people working with video seconds is way more common than nanoseconds.

Accidentally just doing VideoFrameReferance(timestamp=1.0) is an annoying footgun.

At a minimum that should produce an error for floats.