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

Support dynamic stream metadata #359

Merged
merged 2 commits into from
Jul 9, 2019
Merged

Support dynamic stream metadata #359

merged 2 commits into from
Jul 9, 2019

Conversation

Pessimistress
Copy link
Contributor

Change List

  • Add getStreamMetadata to XVIZLoaderInterface
  • Build local stream metadata if DYNAMIC_STREAM_METADATA config is set (requires Support dynamic stream metadata xviz#493)
  • Switch core components to use getStreamMetadata instead of getMetadata().streams

Copy link
Contributor

@marionleborgne marionleborgne left a comment

Choose a reason for hiding this comment

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

Thank you @Pessimistress 🙏 Just a few suggestions on:

  • disambiguating between streamMetadata VS streamsMetadata
  • updating the streamsMetadata refresh logic in XVIZLoaderInterface#getStreamsMetadata()

And a question in metrics-helper.js about a logical change.

modules/core/src/components/log-viewer/core-3d-viewer.js Outdated Show resolved Hide resolved
modules/core/src/loaders/xviz-loader-interface.js Outdated Show resolved Hide resolved
modules/core/src/loaders/xviz-loader-interface.js Outdated Show resolved Hide resolved
const bufferUpdated = this.streamBuffer.insert(timeslice);
if (bufferUpdated) {
this._bumpDataVersion();
}

if (getXVIZConfig().DYNAMIC_STREAM_METADATA && this.streamBuffer.streamCount > oldStreamCount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the second part of the if statement might be too lax (this.streamBuffer.streamCount > oldStreamCount). We could have the same number of streams, but different stream names. And in this case, we would fail to update streamsMetadata.

E.g:

{
   `/vehicle/pose`: ...,
   `/lidar`: ...
}
{
   `/vehicle/speed`: ...,
   `/lidar`: ...
}

I can see how doing a stream by stream comparison can be expensive one very update, so maybe we want to check on the exact streamNames list only periodically (every 1s?). Just an idea to not check this on every update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

streamCount is an accumulative number since the creation of the buffer, and it only ever increases.

modules/core/src/utils/metrics-helper.js Outdated Show resolved Hide resolved
modules/core/src/components/stream-settings-panel.js Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 39.643% when pulling 5e1f00a on x/no-metadata into 0f0edff on master.

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.

3 participants