Skip to content

Show a run's flight data as 2D panels on one time cursor with the 3D view, and read real logs - #19

Open
1-Bort-1 wants to merge 5 commits into
mainfrom
agent/8-symawe-com-runs-simulations-on-request-s
Open

1-Bort-1 wants to merge 5 commits into
mainfrom
agent/8-symawe-com-runs-simulations-on-request-s

Conversation

@1-Bort-1

@1-Bort-1 1-Bort-1 commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

The viewer could draw a kite but not say how it flies, so anyone reading forces or angles went back to Makie. A run of more than one frame now has a scrubber and 2D plots under the 3D view. Dragging the scrubber, or clicking or dragging in a plot, moves all of them together. The plots open on winch force, apparent wind, AoA, heading/course, tether length and depower/steering. The run's other named scalar and vector columns can be added from a list, along with roll, pitch and yaw and the aero quantities, and angles are in degrees. Per-point and per-segment columns such as spring force are not in that list; they arrive with inspect and pin in #14.

The viewer also refused every log the simulator writes, because those frames hold panel corners and wing and body origins behind the points. It now reads the points and ignores the rest. That fix is what makes the rest usable.

before: the viewer refuses the V3 run, because its frames hold more slots than points
after: the same V3 run at 1.80 s, with the scrubber and panels on one time cursor

<SymAWEViewer src={url} frame={30} />                               // before: the host moved the frame
<SymAWEViewer src={url} frame={30} onFrameChange={setFrame} />      // after: the cursor also tells the host

Decisions

  • The test fixture is a real 3 s V3 flight rather than made-up values, exported at OpenSourceAWE/V3Kite.jl@fd73944. Its flight columns are exactly as written. Only its structure document is rewritten, from awesIO 1.0.0 to the 0.1.0 this viewer reads, because the released SymbolicAWEModels 0.20.0 still writes 1.0.0, and V3Kite's move to the newer writer, Read a V3 run from the model's state and place it with KiteGeometry's transforms V3Kite.jl#87, is an unfinished draft. Its canopy faces come from the V3 example. SymAWEViewer: export the V3 fixture again through SymbolicAWEModels'... #18 exports it again once that lands.
  • CL and CD need an air density and a reference area, and a log carries neither. I use 1.225 kg/m³ and the canopy's projected area, which is 17.6 m² for V3 from its nine flat faces. I have no checked reference area to compare that with; V3Kite's own geometry file scales aero forces by 12.9% for "the delta in projected area", so the absolute values may be off by that order. The plots therefore label them as estimates. A run with no canopy faces offers neither, because there is no area to divide by. Lift, drag and L/D depend on neither assumption.
  • Lift is everything perpendicular to the apparent wind, so side force in a turn counts as lift and raises CL and L/D. Separating it needs a choice of side axis, which I left out.
  • A log holds every column whether the model filled it or not, so a column of only zeros or NaN is not offered, unless a default panel opens on it. A run flown without steering still shows that line. The cost is that a real all-zero column outside the defaults, such as a winch held still, cannot be added, because it looks the same as an unfilled one.
  • Plots are hand-drawn SVG. A chart library would be a new dependency for lines, an axis and a cursor.
  • The camera now frames the run's first frame and stays put while scrubbing. It used to re-centre on whichever frame was shown, which would make the view jump on every scrub.

Panels someone added belong to the run they added them on, and a new run opens on its own defaults. Keeping them would crash on, or silently drop, any panel the new run has no column for.

I searched the source for ticks, Euler angles, SVG plots, nearest-frame lookup, resize observing and lift, and the dependencies for a chart library, and found nothing to reuse. three.js has an Euler class, but KiteUtils' convention is measured against NED, so its formula is written out.

What it does not cover. The cursor wiring, a new data, and one-frame runs are checked in a browser below, not in the test suite: mounting the viewer needs a DOM test environment and a stubbed canvas, both new dev dependencies, and #17 carries those tests because it replaces these props. #8 is split so each part can be reviewed alone, stacked in this order: spring force colours and arrows (#13), inspect, pin and CSV (#14), steady table and provenance (#15), SweepView (#16), view state (#17). The screenshot #8 asks for, with forces on and a segment selected, belongs to #14.

Who it affects. Any host showing a multi-frame run gets the panels under its 3D view, with no option to turn them off, and the 3D view shrinks to make room. Real logs from the released SymbolicAWEModels are still refused, by name, for their awesIO 1.0.0 document; that needs its next release. #12 is not a base for this: it conflicts with main, which already reads the schema, and it looks ready to close.

Verification

  • Reproduced first: /v3_psm_sim.arrow frames carry 334 positions but its topology has 44 points
  • test/topology.test.ts "reads the first n_points slots" red before, green after
  • New test/quantities.test.ts and test/panels.test.tsx against the V3 fixture. Lift and drag recombine to the logged aero force, and the kite yaws -90° nosing into its west wind. The Euler convention comes from reading KiteUtils' source and is pinned by hand-worked cases, not by a run against KiteUtils itself.
  • test/quantities.test.ts and test/panels.test.tsx: the review's cases red before (9 failed of 34), green after (34 passed).
  • Local full suite on the final commit: 64 passed (exit 0). tsc both configs clean · pnpm build clean · GitHub CI: PASS. There is no CHANGELOG.md in this repo, so the README carries the change.
  • Known flake: 1 of 3 earlier full runs failed with Test timed out in 5000ms, on a test taking 8208 ms in test/parts.test.ts, then one in test/topology.test.ts. Both only decode example runs, this branch changes neither test, and the box's load was about 180 on 20 cores. I left the timeout alone.
  • Real browser (headless Chromium, software WebGL), pre-fix and fixed source, same steps. Swapping to a run without steering: before TypeError: Cannot destructure property 'values', after six panels. Series paths rebuilt by one scrub step: before 8, after 0. Fixed source also: scrubbing reports frame 10, a host frame of 40 moves the scrubber to 40, and a one-frame run shows the 3D view with no scrubber or panels.
  • At 1100×800 and 390×800, from the earlier build of this branch: six panels open, clicking a panel seeks, panels add and remove, no horizontal overflow at phone width.
  • Not run: scripts/export_v3_fixture.jl after its constants became plain variables, since the export takes 162 min and the fixture is unchanged.
  • Benchmark: n/a
  • Risk: CL and CD rest on the assumed density and area above, so read their trend rather than their absolute value.

Opened by 1-Bort-1, an AI agent working for @1-Bart-1.
Refs #8 · task SymAWEViewer-8

1-Bort-1 and others added 3 commits November 3, 2026 00:53
A log keeps panel corners, wing and body origins behind the points in X, Y
and Z, so the viewer refused every log SymbolicAWEModels writes. Read the
first n_points slots and keep the table for the columns beyond positions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds roll, pitch and yaw from the quaternions, lift, drag and L/D from the
aero forces, and CL and CD where a canopy gives an area.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@1-Bort-1 1-Bort-1 added the agent:running Agent task state label Nov 3, 2026
@1-Bort-1 1-Bort-1 added agent:ci Agent task state agent:review Agent task state and removed agent:running Agent task state agent:ci Agent task state labels Nov 3, 2026

@1-Bort-1 1-Bort-1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent review (advisory)

Verdict: REQUEST CHANGES · 8 inline, 0 off the diff

Good

  • decodeRun reads the first n_points slots and still refuses frames with too few positions; established by reading src/topology.ts, and a new test pins it on the V3 fixture (61 frames, 44 points).
  • The default panels match #8's set and order, and a test in test/quantities.test.ts pins them; defaultPanels picks them from the quantities present, so no kite is hard-wired.
  • Angles are scaled to degrees through ColumnSpec, and periodic ones break at the wrap rather than drawing across the plot; established by reading angle()/fromColumn and the seriesPath tests.
  • Hand-checking the quaternion algebra confirms up in projectedCanopyArea is the rotation's third column, and that identity gives -90° yaw and a +y rotation gives nose-up pitch.
  • The scrubber and panels share one cursor, and the one-frame guard hides them; established by reading SymAWEViewer.tsx.
  • No runtime dependency is added: package.json lists yaml and vite as dev dependencies only, and the plots are hand-drawn SVG as the card says.
  • The split into #13–#17 is stated on the card, and this diff stays inside the panels part of #8 plus the log-reading fix that part needs.

Not good

  • src/TimePanel.tsx:349 — panels state survives a run change, because setRun(null) and setRun(decoded) batch when the data prop changes, so byId.get(id)! yields undefined for an id the new run lacks and valueRange throws during render. Swapping data to a run without, say, steering takes the host's tree down, and so does passing a new run to the exported RunPanels.
  • src/quantities.ts:271 — This filter drops any all-zero series, so a run flown without steering loses a line of #8's default depower/steering panel, and real zeros cannot be added from the list. It also works per element, not per column as its docstring says, so vel_kite/y can vanish alone; #8 asks for panels chosen from the columns present.
  • src/quantities.ts:249 — With zero aero forces, 0/0 makes L/D all NaN, and the value !== 0 filter at line 271 keeps NaN. A run that never filled its aero forces then offers an empty L/D panel, against #8's "where the aero forces are present" and README line 61.
  • src/quantities.ts:252 — #8 offers CL and CD wherever aero forces are present, but this drops them for any run without canopy faces, and elsewhere divides by an assumed density and first-frame projected area. The plot then labels a value the card itself says to read only for trend as plain CL [-], with no caveat in the UI.
  • src/TimePanel.tsx:233 — RunPanels builds a new quantities array on every render (line 349), so this useMemo misses each time the cursor moves and every panel rebuilds its SVG paths on each scrub step. The memo does nothing during scrubbing, and the cost grows with frame count on long runs.
  • src/SymAWEViewer.tsx:198 — seek, onFrameChange, the frame-to-cursor effect and the one-frame guard have no test; no test mounts SymAWEViewer, and panel tests cover only timeAtX and static markup, not the pointer handlers. #8 requires a test per feature and §8 one for new behaviour, and the card admits host frame and one-frame runs were not run in a browser either.
  • README.md:66 — The README says a level kite facing east has zero yaw, but kiteAttitude gives east +90° (q = 0,0,0,1), west -90° as test/quantities.test.ts pins, and zero for north. Anyone reading yaw from this sentence gets its reference direction wrong by 90°.
  • scripts/export_v3_fixture.jl:10 — Lines 10-15 declare top-level consts in a script, which §10 forbids outside modules. Plain locals would meet the rule.
  • The new README text is hard-wrapped (e.g. lines 54-61), against §6's one-paragraph-one-line rule.
  • The card does not say what was searched for before adding helpers, which §2 requires.
  • defaultPanels, kiteAttitude, nearestFrame, runTimes and Run.table are public but the README does not document them (§4).
  • The runQuantities docstring carries a why ("A log carries every column…, so…"), which §4 forbids; the same reasoning is also in the README.
  • scripts/export_v3_fixture.jl opens with a three-line comment block and has lines over 92 characters (§3, §10).
  • test/quantities.test.ts:19 uses the one-letter name q (§3).
  • lift at src/quantities.ts:243 is the whole force perpendicular to the apparent wind, so side force in a turn counts as lift and inflates CL and L/D; neither the card nor the README says so.
  • The scrubber readout prints times[frame].toFixed(2) s even when runTimes fell back to frame numbers, so a run with no time column shows frames as seconds.
  • The card says any other quantity the run holds can be added, but COLUMNS is an allow-list; spring_force and the raw aero_force_* columns are not offered.
  • The card's 19.75 m² area is from memory, so the stated 12% CL/CD error rests on an unchecked figure.
  • The test/parts.test.ts failure on the card was never explained; the timeout cause is a guess and the output was not kept.

claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.

Comment thread src/TimePanel.tsx Outdated
{panels.map((panel) => (
<TimePanel
key={panel.join()}
quantities={panel.map((id) => byId.get(id)!)}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

MAJOR: panels state survives a run change, because setRun(null) and setRun(decoded) batch when the data prop changes, so byId.get(id)! yields undefined for an id the new run lacks and valueRange throws during render. Swapping data to a run without, say, steering takes the host's tree down, and so does passing a new run to the exported RunPanels.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in f41ca76: panels now belong to the run they were chosen on. Swapping to a run without steering threw before and shows six panels after, in Chromium; the choice logic is unit-tested, the mounted swap goes to #17.

Comment thread src/quantities.ts Resolved
Comment thread src/quantities.ts Resolved
Comment thread src/quantities.ts Resolved
Comment thread src/TimePanel.tsx Resolved
Comment thread src/SymAWEViewer.tsx Resolved
Comment thread README.md Resolved
Comment thread scripts/export_v3_fixture.jl Resolved
@1-Bort-1 1-Bort-1 added agent:queued Agent task state and removed agent:review Agent task state labels Nov 3, 2026
@1-Bort-1 1-Bort-1 added agent:running Agent task state and removed agent:queued Agent task state labels Nov 3, 2026
A host swapping `data` to a run without a panel's column crashed: chosen
panels now belong to the run they were chosen on. Scrubbing no longer
rebuilds every panel's paths. The list filter works per column, keeps
default-panel columns and treats NaN as unfilled. CL and CD are labelled
estimates. A run with no time column reads its cursor in frames.
README: yaw reference corrected, public helpers documented, new section
unwrapped. Export scripts follow the comment and const rules.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@1-Bort-1

1-Bort-1 commented Nov 3, 2026

Copy link
Copy Markdown
Member Author

The findings with no comment id, all in f41ca76 or on the card. README hard-wrap: new section unwrapped; older sections left. Search: on the card. Undocumented exports: README table added. Docstring why: removed, and the README states behaviour only. Script comment block and long lines: fixed, in the .mjs too. q: renamed. Lift includes side force: stated on README and card, formula unchanged since separating it needs a side-axis choice. Scrubber: a run with no time column reads frame n, tested. Allow-list: card corrected, spring_force comes with #14. 19.75 m²: dropped, the card now cites V3Kite's own 12.9% note. parts.test flake: reproduced with output kept, a 5000 ms timeout at load ~180, on the card; timeout left alone.

@1-Bort-1 1-Bort-1 added agent:ci Agent task state and removed agent:running Agent task state labels Nov 3, 2026
@1-Bort-1
1-Bort-1 marked this pull request as ready for review November 3, 2026 01:02
@1-Bort-1 1-Bort-1 added agent:review Agent task state and removed agent:ci Agent task state labels Nov 3, 2026
@1-Bort-1
1-Bort-1 requested a review from 1-Bart-1 November 3, 2026 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:review Agent task state

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant