Skip to content

New C++ API for timestamp/duration indices #9200

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

Merged
merged 18 commits into from
Mar 6, 2025
Merged

Conversation

emilk
Copy link
Member

@emilk emilk commented Mar 5, 2025

Related

What

Updated the C++ API to distinguish between absolute timestamps and and relative durations.

We've deprecated the following functions in RecordingStream, with the following replacements:

  • set_time_sequence -> set_index_sequence
  • set_time -> set_index_duration or set_index_timestamp
  • set_time_seconds -> set_index_duration_secs or set_index_timestamp_seconds_since_epoch
  • set_time_nanos -> set_index_duration_nanos or set_index_timestamp_nanos_since_epoch

TimeColumn also has deprecated functions.

Questions to reviewer

The names for the functions in TimeColumn are somewhat inconsistent with those of RecordingStream.

Other notes

I also changed/broke the C API.

TODO

  • Full check

Copy link

github-actions bot commented Mar 5, 2025

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
524ddee https://rerun.io/viewer/pr/9200 +nightly +main

Note: This comment is updated whenever you push a commit.

Copy link

github-actions bot commented Mar 5, 2025

Latest documentation preview deployed successfully.

Result Commit Link
524ddee https://landing-boii4cv2w-rerun.vercel.app/docs

Note: This comment is updated whenever you push a commit.

@emilk emilk marked this pull request as ready for review March 5, 2025 15:33
@emilk
Copy link
Member Author

emilk commented Mar 6, 2025

@rerun-bot full-check

Copy link

github-actions bot commented Mar 6, 2025

@Wumpf Wumpf self-requested a review March 6, 2025 10:13
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

Looking great! Feeling somewhat strong about not having Collection::first (see comment), but otherwise all good

The names for the functions in TimeColumn are somewhat inconsistent with those of RecordingStream.

Seems like a good opportunity to make things more similar to Rust? I figure you'll eventually want to move towards IndexColumn? 🤔
Not sure what the exact question to the Reviewer is here 😄. Can we make things less inconsistent?

@emilk
Copy link
Member Author

emilk commented Mar 6, 2025

About the naming: I added a TODO in the parent issue

@emilk
Copy link
Member Author

emilk commented Mar 6, 2025

@rerun-bot full-check

Copy link

github-actions bot commented Mar 6, 2025

Started a full build: https://github.com/rerun-io/rerun/actions/runs/13699399079

@emilk emilk merged commit 515d05c into main Mar 6, 2025
77 of 78 checks passed
@emilk emilk deleted the emilk/cpp-time-refactor branch March 6, 2025 13:39
@emilk emilk mentioned this pull request Mar 7, 2025
11 tasks
@emilk emilk mentioned this pull request Apr 11, 2025
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants