Refine scatter, mixture map, and timeline interactions#9
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the visualization panels and channel discovery logic. The MixtureMapPanel now features density-based alpha blending, improved axis formatting, and interactive time selection by clicking on heatmap cells. The ScatterPanel adds support for padded and locked bounds, along with the ability to set the global cursor by clicking on the nearest data point. The TimelinePanel has been redesigned to display labeled lap blocks instead of a waveform and now updates the cursor time on hover. Additionally, the channel search logic was expanded with token-based matching to improve reliability across different log files. Feedback focuses on optimizing performance by reducing redundant normalizations in the search loop and caching vector allocations in the scatter plot's UI hot path.
| fn find_channel_by_all_tokens(ld: &LdFile, tokens: &[&str]) -> Option<usize> { | ||
| ld.channels.iter().position(|ch| { | ||
| let normalized = normalize_channel_name(&ch.name); | ||
| !normalized.is_empty() | ||
| && tokens.iter().all(|token| { | ||
| let token = normalize_channel_name(token); | ||
| !token.is_empty() && normalized.contains(&token) | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The tokens are re-normalized for every channel iteration in the loop. Normalizing them once before the loop improves performance, especially when searching through a large number of channels.
fn find_channel_by_all_tokens(ld: &LdFile, tokens: &[&str]) -> Option<usize> {
let normalized_tokens: Vec<String> = tokens
.iter()
.map(|t| normalize_channel_name(t))
.filter(|t| !t.is_empty())
.collect();
ld.channels.iter().position(|ch| {
let normalized = normalize_channel_name(&ch.name);
!normalized.is_empty()
&& normalized_tokens
.iter()
.all(|token| normalized.contains(token))
})
}| }); | ||
| } | ||
| let points = &self.cache.as_ref().unwrap().points; | ||
| let plot_points: Vec<[f64; 2]> = points.iter().map(|p| [p.x, p.y]).collect(); |
There was a problem hiding this comment.
This Vec allocation and iteration happens every frame during the UI update. Since the underlying points are already cached in ScatterCache, you should also cache this derived plot_points vector to avoid redundant allocations in the hot path.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68dbb1998d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| scatter.map(|panel| { | ||
| ( | ||
| "Oil Pressure".to_string(), | ||
| DockState::new(vec![PanelTab::Scatter(scatter_panel)]), | ||
| )), | ||
| (None, Some(graph_panel)) => Some(( | ||
| "Oil Pressure".to_string(), | ||
| DockState::new(vec![PanelTab::Graph(graph_panel)]), | ||
| )), | ||
| (Some(scatter_panel), Some(graph_panel)) => { | ||
| let mut dock = DockState::new(vec![PanelTab::Scatter(scatter_panel)]); | ||
| dock.main_surface_mut().split_below( | ||
| NodeIndex::root(), | ||
| 0.82, | ||
| vec![PanelTab::Graph(graph_panel)], | ||
| ); | ||
| Some(("Oil Pressure".to_string(), dock)) | ||
| } | ||
| } | ||
| DockState::new(vec![PanelTab::Scatter(panel)]), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
This return path now drops the entire Oil Pressure worksheet unless both scatter channels are found. In the previous behavior, users still got a graph-only worksheet when lateral-accel matching failed, so sessions that have oil pressure but no suitable X channel now lose this worksheet entirely. Please preserve a non-scatter fallback so oil-pressure data remains accessible in those logs.
Useful? React with 👍 / 👎.
| if self.lock_bounds && let Some(allowed_bounds) = allowed_bounds { | ||
| let clamped = clamp_plot_bounds(plot_ui.plot_bounds(), allowed_bounds); | ||
| plot_ui.set_plot_bounds(clamped); |
There was a problem hiding this comment.
With lock_bounds enabled, this still allows wheel zoom to shrink the visible span because it only clamps panning outside allowed_bounds rather than restoring the full computed bounds each frame. That breaks the new "locked" behavior contract and is user-visible in the Oil Pressure scatter default (lock_bounds = true). Set plot bounds directly to allowed_bounds (or disable zoom/drag) to keep the view truly fixed.
Useful? React with 👍 / 👎.
1 participant
Summary
Testing