Fix sim-log channel scaling, 1-byte channels, and graph panel UI - #20
Merged
Conversation
Two parsing bugs that only surface on sim-exported ADL logs, both reported in #18. Scaling applied `shift` before `mul`: (raw / scale * 10^-dec + shift) * mul `shift` is an offset already in engineering units, so it belongs after the multiplier. The wrong order offsets a channel by shift * (mul - 1), which is zero whenever mul == 1 — the case in every .ld file in the repo, hence the blind spot. Sim exporters use mul = 2 with a non-zero shift to pack a signed range into int16, so Throttle Pos (shift=50, mul=2, dec_places=3) read 50-150 % instead of 0-100 %, and Ground Speed (shift=641, mul=2) carried a constant +641 km/h. Verified against the reporter's log: ground speed 0-301 km/h, water temp 52-94 C, tyre temps 19.6-219.6 C, brake bias 47.2 %, and wheel rotation speed that now agrees with ground speed for a 0.337 m rolling radius. Separately, `from_codes` had no case for a 1-byte sample width, so Gear and Marker fell through to Unknown and failed to load entirely. For integer channels `dtype_code` is the width in bytes — confirmed by walking the channel list by data_ptr, where the gap divided by n_data is exactly 1.000 for those two channels and 2.000 for every dt=2 channel. Added DataType::Int8, decoded signed since reverse gear logs as -1. Fixes #18 Claude-Session: https://claude.ai/code/session_01PHdY3DhzFpD3QtSJUN1PQv
Three unrelated graph-panel issues found while testing #18. Tiles did not reflow when a graph was added. `ensure_tile_heights` computed available_height / tile_count but passed it to Vec::resize, which only fills newly appended slots — the existing tiles kept their old heights, so the stack overflowed into the scroll area instead of shrinking to make room. Redistribution now happens inside ensure_tile_heights whenever the count changes, scaling rather than resetting so proportions set by dragging survive. That also replaces three ad-hoc tile_heights.clear() calls which only papered over the add-by-drag path and discarded those proportions; the paths that went through add_channel, the channel picker or move_channel_to_new_graph never reset at all. Manage Channels rows could not be clicked. egui's dnd_drag_source layers a bare Sense::drag() over the contents it just added, and because that interaction registers last it sat on top of the row button and swallowed the press. Selection never happened, so the Remove button stayed disabled — and the context menu, attached to the same dead response, could not open either, leaving no way to remove a channel. Replaced with a clickable_drag_source helper that senses click_and_drag on the row itself, matching the pattern channel_browser.rs already uses. Added a per-row remove button so removal does not depend on discovering right-click or select-then-Remove. The main graph scroll area now reserves a permanent gutter (ScrollStyle::solid plus AlwaysVisible) so plot width no longer shifts as tiles are added or removed. egui's default scroll style floats over the content, so visibility alone would not have been enough. Claude-Session: https://claude.ai/code/session_01PHdY3DhzFpD3QtSJUN1PQv
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73c1a0bbb6
ℹ️ 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".
Follow-up from reviewing the previous two commits.
Auditing every (dtype_a, dtype_code) pair across all six .ld files on hand
turned up a third integer family, 0x06, missing from the data-type table.
Four channels per file fell through to Unknown and silently failed to
load — including in VIR_LAP.ld, the checked-in fixture: CP Lotus ESP
System State, Lap Beacon Ticks, Lap GPS Closest Beacon and GPS
Satellites. Decoded as int32 they read 1, ~248-374M ticks, beacon indices
~26000, and 6-10 satellites; as float32 they are all denormal garbage, so
the family is unambiguous. Data-block spacing confirms the 4-byte width.
The snapshot test counts metadata entries, so it was blind to this.
apply_scaling now scales in place instead of collecting into a second
Vec — a channel can hold ~190k samples, so that was a 1.5 MB transient
allocation per read for nothing. Bit-exact: same operations, same order.
fit_tile_heights replaces the scale-then-clamp in ensure_tile_heights.
Clamping after scaling overflows the panel whenever a tile already sits
near the floor, because clamping it back up adds height the others never
gave away: [440, 80, 80] plus a fourth tile came to 632 px in a 600 px
panel. Now tiles that would land under the floor are pinned there and the
rest rescaled into the height that frees up, which fits exactly whenever
the floor fits at all.
Also from review: read_i8 helper so the Int8 arm matches its five
siblings; the row remove button uses the existing RichText("X").small()
idiom rather than a new glyph; clickable_drag_source documented as a fork
of egui's dnd_drag_source with a pointer for re-diffing on upgrade, and
its redundant drag_started payload set dropped to match upstream; dropped
the render-site floor clamps that duplicated the invariant
ensure_tile_heights already guarantees; consolidated the data-type and
reflow assertion tables.
Claude-Session: https://claude.ai/code/session_01PHdY3DhzFpD3QtSJUN1PQv
The graph panel edits left a block that rustfmt reflows, failing the format check. The data-type coverage test asserted the fixture still contained undecodable channels, which stopped being true once the 0x06 integer family was mapped — invert it to assert full coverage instead. Claude-Session: https://claude.ai/code/session_01FV6mm15SerkbAhgKByu2e9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #18, plus three graph-panel issues found while testing it.
Sample scaling applied
shiftbeforemulshiftis an offset already expressed in engineering units, so it belongs after the multiplier. The wrong order offsets a channel byshift * (mul - 1)— zero whenevermul == 1, which is the case for every.ldfile in this repo (VIR_LAP: 199 channels, allmul=1; both S1 examples likewise). That's why it went unnoticed.Sim exporters use
mul = 2with a non-zeroshiftto pack a signed range into int16.Throttle Pos(shift=50, mul=2, scale=1, dec_places=3) stores 0–100 % as raw −25000…25000, which the old order reported as 50–150 % — exactly the reporter's screenshot.Ground Speed(shift=641, mul=2) carried a constant +641 km/h.Verified against the reporter's log:
Wheel rotation speed cross-checks it independently: 301.28 km/h ÷ 248.5 rad/s gives a 0.337 m rolling radius, right for the car.
The reporter also found that setting manual min/max to 0/100 looked "even weirder" — that was this same bug, not the scaling editor. The data genuinely spanned 50–150, so a 0–100 axis pushed the trace off the top.
docs/ld-file-format.mdspecified the wrong formula too; corrected with a note on whymul == 1hides it.GearandMarkerfailed to decodefrom_codeshad no case for a 1-byte sample width, so(0x03, 1)fell through toUnknown,bytes_per_sample()returnedNone, and the channels refused to load.For integer channels
dtype_codeis the width in bytes. Confirmed empirically rather than guessed: walking the channel list sorted bydata_ptrand dividing each gap byn_datagives exactly1.000forGearandMarker,2.000for everydt=2channel.Gearthen decodes to 0–7 — neutral plus seven forward gears, mean 3.19, distributed the way real driving looks. Decoded signed, since reverse logs as-1.Graph tiles didn't reflow when adding a graph
ensure_tile_heightscomputedavailable_height / tile_countbut handed it toVec::resize, which only fills newly appended slots. Existing tiles kept their old heights, so 3 → 4 tiles in a 600 px panel gave 200+200+200+150 = 750 px and the stack overflowed into the scroll area instead of shrinking.Redistribution now happens inside
ensure_tile_heightswhen the count changes, scaling rather than resetting so proportions you dragged survive: 300/150/150 plus a fourth becomes 240/120/120/120 — still 2:1:1, still exactly 600 px. Removal scales back up symmetrically. A resize drag is zero-sum, so the reflow is a no-op when the count is unchanged and can't fight an in-progress drag.This also removes three ad-hoc
tile_heights.clear()calls that were papering over it. They only covered the drag-drop path (and discarded proportions while doing so);add_channel, the channel picker, andmove_channel_to_new_graphnever reset at all, which is why the bug looked inconsistent.Manage Channels rows were inert
egui's
dnd_drag_sourcelayers a bareSense::drag()over the contents it just added (ui.rs:2674-2681). Registered last, it sits on top of the row button and swallows the press. The code checked the inner button'sclicked(), which never fired — sopending_selectionnever got set, theRemovebutton stayed permanently disabled, and the context menu (attached to the same dead response) couldn't open either. There was no way to remove a channel at all.Replaced with a
clickable_drag_sourcehelper sensingclick_and_dragon the row itself — the patternchannel_browser.rsalready uses. Drag-to-reorder is unaffected. Added a per-row remove button so removal doesn't depend on discovering right-click or select-then-Remove.Consistent scrollbar gutter
The main graph stack now reserves a permanent gutter (
ScrollStyle::solid+AlwaysVisible), so plot width no longer shifts as tiles are added or removed. egui's default scroll style floats over content, so always-visible alone wouldn't have been enough. Pairs well with the existingScrollSource::SCROLL_BARon that view, where the wheel is reserved for plot zoom and the bar is the only way to scroll.Dialog lists keep the default floating bars — permanent gutters there would just eat width.
GPS Satellitesand three others never decoded — including in our own fixtureAuditing every
(dtype_a, dtype_code)pair across all six.ldfiles on hand turned up a third integer family,0x06, missing from the type table. Four channels per file fell through toUnknownand silently failed to load — including inVIR_LAP.ld, the checked-in fixture:GPS SatellitesCP Lotus ESP System StateLap GPS Closest BeaconLap Beacon TicksUnambiguous — as float32 they're all denormal garbage. Data-block spacing confirms the 4-byte width. The
vir_lap_parser_snapshottest counts metadata entries, so it was blind to this; there's now an integration test that actually reads the samples and asserts the satellite count is a plausible fix.Tile reflow overflowed when a tile sat near the floor
Found by strengthening the reflow test. Scaling proportionally and then clamping to the floor overflows the panel, because clamping a tile back up adds height the others never gave away:
[440, 80, 80]plus a fourth tile came to 632 px in a 600 px panel. Realistic — it happens on a single add whenever one tile has been dragged small.fit_tile_heightsnow pins tiles that would land under the floor and rescales the rest into the height that frees up, so the stack fits exactly whenever the floor fits at all.Testing
cargo test --workspace: 145 passed, 0 failed. Clippy clean.The
vir_lap_parser_snapshottest passes unchanged, confirming no regression on existing files. Also ran the GUI against the reporter's log to check each fix interactively.Notes
mul = 1there.Beaconstill reads −32767…17377.dtype_a=0x00withshift=0, mul=1, so neither fix touches it. Possibly correct-as-logged, possibly a third issue..ldfixture added; the reporter's file is 5.4 MB. Worth considering, as nothing in the repo exercisesmul != 1or 1-byte channels.apply_scalingnow scales in place rather than collecting into a secondVec— bit-exact, and saves a 1.5 MB transient allocation per channel read. The bigger cost in that path is the per-samplematchinread_raw_samples(~166 µs vs ~30 µs for scaling on a 190k channel), left alone as pre-existing and out of scope.https://claude.ai/code/session_01PHdY3DhzFpD3QtSJUN1PQv