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

Disable reading from bag while recording - use direct caching to index for timeline #123

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

emersonknapp
Copy link

@emersonknapp emersonknapp commented Sep 2, 2022

Part of #121
RFC: This covers the Recording functionality for #121. Draft PR for consideration, feeling out opposition on whether the proposed tradeoff is acceptable.

Description

Rosbag2 doesn't actually support reading-while-writing. The current code goes around this limitation by making queries directly into the SQLite database, but MCAP is now growing in usage as a rosbag2 storage format, so we need rqt_bag to support strictly the rosbag2 API.

For now, I believe it is a worthwhile tradeoff to lose the RawView/PlotView on bags currently recording, in order to gain support for all storage implementations. From what I can tell, the PlotView only shows "data-until-now" when opened, and cannot refresh data, so the panel needs to be closed and reopened to see new data. RawView likewise does not update while recording, since the playhead does not advance - though the user can click in.

…x for timeline

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp marked this pull request as ready for review September 8, 2022 19:04
@emersonknapp
Copy link
Author

@ivanpauno do you have any opinion on this?

@ivanpauno
Copy link

Seems like a fair tradeoff to me.
We could also keep the sql queries if the backend is detected to be sql for backwards compatibility, but I don't know if that's worth it. i.e. I don't know if people really use the views while recording, based on the limitations described here I would guess no.

Copy link

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants