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.

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

Utils & snippets for manually logged VideoFrameReference #7403

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Sep 11, 2024

What

A bit of a first: I'm referencing the same examples on both the AssetVideo and the VideoFrameReference archetype, seems to make sense :)

screenshot

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide
  • green main build

To run all checks from main, comment on the PR with @rerun-bot full-check.

@Wumpf Wumpf added exclude from changelog PRs with this won't show up in CHANGELOG.md 🎞️ video labels Sep 11, 2024
Base automatically changed from andreas/asset-download-script to main September 12, 2024 07:39
@Wumpf Wumpf force-pushed the andreas/video-reference-snippets-and-utils branch from 148f9fc to 8d4ad66 Compare September 12, 2024 08:10
Copy link

github-actions bot commented Sep 12, 2024

Deployed docs

Commit Link
0140f00 https://landing-dwgu9mquh-rerun.vercel.app/docs

@Wumpf Wumpf mentioned this pull request Sep 12, 2024
@Wumpf
Copy link
Member Author

Wumpf commented Sep 12, 2024

@rerun-bot full-check

Copy link

rr.send_columns(
"video",
times=[rr.TimeSecondsColumn("video_time", times)],
components=[rr.VideoFrameReference.indicator(), rr.components.VideoTimestamp.seconds(times)],
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to also set the entity path for illustration (and testing) purposes, but this makes it pretty complex quickly since all columns need to be the same length and one wouldn't want to log the entity path many times 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I might be misremembering: Didn't we say we would exclude that feature for now, and have the entity path be implicit?

Copy link
Member Author

Choose a reason for hiding this comment

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

that was suggested but at that time I already implemented it because it was fairly trivial to do. It has been suggested to make use of this by adding an example that combines several videos on a single entity

@Wumpf Wumpf marked this pull request as ready for review September 12, 2024 10:21
@Wumpf
Copy link
Member Author

Wumpf commented Sep 12, 2024

main build actually came back red!
snippet builder needs to run the download script https://github.com/rerun-io/rerun/actions/runs/10828719828/job/30045290186

@Wumpf Wumpf added the do-not-merge Do not merge this PR label Sep 12, 2024
@Wumpf
Copy link
Member Author

Wumpf commented Sep 12, 2024

@rerun-bot full-check

@Wumpf Wumpf removed the do-not-merge Do not merge this PR label Sep 12, 2024
Copy link

Copy link
Member

@jprochazk jprochazk left a comment

Choose a reason for hiding this comment

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

LGTM mod typos

@Wumpf Wumpf merged commit ecb20e7 into main Sep 13, 2024
39 checks passed
@Wumpf Wumpf deleted the andreas/video-reference-snippets-and-utils branch September 13, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md 🎞️ video
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants