Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Plots! #7279

Merged
merged 89 commits into from
Jan 13, 2024
Merged

Plots! #7279

merged 89 commits into from
Jan 13, 2024

Conversation

defunctzombie
Copy link
Contributor

@defunctzombie defunctzombie commented Jan 1, 2024

User-Facing Changes
Plot panel bugfixes

Description
Over the past few months there's been a lot of work on the plot panel to improve performance for large datasets. The performance has improved (yet is still not meeting expectations) and the number of bug reports as skyrocketed. Additionally, the code changes introduced thousands of lines of code that no current team member understands and can effectively debug to resolve the outstanding plot bugs.

Rather than bandaid the plot and continue with a complex and fragile architecture, I've taken a different tactic and propose a complete overhaul of the architecture while maintaining the important performance characteristics.

In this PR I do away with the following concepts from the Plot panel code:

  • Processor: the recently added code that is large, complex, fragile, and hard to debug). It is responsible for some form of coordination between downsampling and rendering.
  • PlotChart (and the many hooks): another layer wrapping the TimeBasedChart component
  • TimeBasedChart: the react layer(s) wrapping the chartjs interface (and ultimately the worker that renders the chart data)
  • useDataset (and other hooks): the layers of hooks responsible for figuring out datasets? unclear what it does now with the processor
  • many other support files related to powering the above

To replace the above I've implemented:

  • Plot.tsx: The plot panel component. This sets up interfaces between the "react" layer (message pipeline, DOM elements, mouse events, config, etc) and the chart logic (dataset building, rendering) which does not use react and does away with the many many hooks for dataset fetching.
  • PlotCoordinator.ts: Receives inputs from the "react" layer (DOM events, hover updates, player state, config, tooltips) and dispatches them to the the DatasetsBuilder or the ChartRenderer depending on the update. It gets downsampled datasets from the Builder to the the ChartRenderer. The builder and renderer run in workers so the main thread UX interactions remain fast while downsampling and rendering.
  • OffscreenCanvasRenderer & ChartRenderer.ts: Draws chart datasets onto an offscreen canvas.
  • IDatasetsBuilder.ts: Accumulates raw datasets and provides an interface to query for downsampled chart datasets.
  • BlockTopicCursor: Tracks the processed block location for a topic. Each series has a cursor which identifies the next blocks of data that need sending to the dataset builder. When the cursor detects the underlying blocks changed it resets and we can re-accumulate the updated series data.

Note: The panel does not pause/resume frame

Fixes: #7278
Fixes: FG-5381
Fixes: FG-6190
Fixes: FG-6181
Fixes: FG-6179
Fixes: FG-6171
Fixes: FG-6184
Fixes: FG-6218
Fixes: FG-6192
Fixes: FG-6189
Fixes: FG-6151
Fixes: https://github.com/orgs/foxglove/discussions/549


Showstoppers:

Errata:

  • story preloaded data in binary blocks (issue is same message path different series w/ receiveTime vs headerStamp) use a string key for plot series #7324
  • When using "show value" in the legend - if you remove a series the value of the series that remains disappears
  • two series with the same message path and same timestamp method? (roman: if they use the same timestamp mode they will appear as the same series - we can disambiguate them by their index in the paths list but if one of them is removed then it will appear like the remaining one is a new series (different index) and we will need to re-send the data to the worker.
  • grid.data[:] crashes with the tf grid dataset

TODO perf/cleanup:

  • use partial subscriptions for index mode and current msg path mode
  • one version of getChartValue, readMessagePathItems, isChartValue
  • "split down" on mcnasty-5m causes the UI to lock up for 0.5-3s. if I try to split many panels too quickly it can crash the tab (roman: split current processing from blocks to dispatch render updates between blocks when all data is already preloaded)
  • more testing with data platform
  • avoid dataset re-downsample when playing or hovering if no dataset changes
  • a path like /mcnasty/position. resulted in a subscription with an "" empty field name when really this path is not yet valid and we should not update subscriptions
  • probably don't need the useEffect to set renderer and can add a canvas element directly to the react via jsx and get a ref on that instead of the canvasDiv flow (noted by roman)
  • normalize config before sending downstream (maybe add the computed key and apply global variables as well)
  • deduplicate mock Worker code for unit tests
  • fillInGlobalVariablesInPath is done in each datasets builder - would be nice to do once at top level
  • have renderer request viewport dataset directly instead of hop through main thread
  • void this.#datasetsBuilderRemote.setConfig(this.#seriesConfigs); needs error handling - maybe move the sending of the config before sending the datasets since that is already in an async function
  • "RosPath" stuff needs to be renamed as MessagePath and put in a new package in the repo.
  • incremental downsampling
  • Add key to IndexDatasetsBuilder series and CurrentCustomDatasetsBuilder for lookup of series that changes after global variables change.
  • This code to build subscriptions from series could be moved into a separate function/file and given unit tests
  • Plots! #7279 (comment)
  • Plots! #7279 (comment)

When plotting an enum value the tooltip can show the "text"
image

Calling panels don't need to import all of the plot code to get this one function.
**User-Facing Changes**
<!-- will be used as a changelog entry -->

**Description**
Disambiguates two series with the same path but different header stamps
so they appear as two separate series.

See the "preloaded data in binary blocks" story for an example of the
fix compared to the base branch
expect(cursor.next(blocks)).toEqual(undefined);
});

it("should continue from last location when new blocks arrive", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove "should" from all of these descriptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why's that? Do we not like that pattern? It's pretty typical language use for the describe/it API style. it should. I don't feel strongly about it though.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I've seen both "it should work" and "it works" styles of writing test names – I don't think we're consistent today in our code

dataset: ChartDataset<"scatter", DatumWithReceiveTime[]>;
};

export class CurrentCustomDatasetsBuilder implements IDatasetsBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Current is a little confusing only because it's also used when there a multiple of something over time. I think CurrentFrameCustomDatasetsBuilder would be a little easier to read and search for, but that might be verbose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The names are meant to match the modes. I will add a comment to the class describing what it does.

return;
}

{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the extra closure/bracket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the msgEvent variable is localized since I make another one below and lint complained about variable shadowing

}
}

function lastMatchingTopic(msgEvents: Immutable<MessageEvent[]>, topic: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

consolidate across dataset builders?

packages/studio-base/src/panels/Plot/Plot.tsx Outdated Show resolved Hide resolved
Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

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

📿

Comment on lines +276 to +277
datasetsBuilder instanceof CurrentCustomDatasetsBuilder ||
datasetsBuilder instanceof CustomDatasetsBuilder
Copy link
Member

Choose a reason for hiding this comment

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

maybe Plot.tsx shouldn't have to care too much about specific builder classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably - then all of them would need a setXPath and some would ignore or throw

packages/studio-base/src/panels/Plot/PlotCoordinator.ts Outdated Show resolved Hide resolved
@defunctzombie defunctzombie merged commit c3468ad into main Jan 13, 2024
14 checks passed
@defunctzombie defunctzombie deleted the roman/plot-rework branch January 13, 2024 02:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

The plot component has a problem displaying the chart when adding a curve.
5 participants