Repository navigation
Conversation
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
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: REQUEST CHANGES · 8 inline, 0 off the diff
Good
decodeRunreads the first n_points slots and still refuses frames with too few positions; established by readingsrc/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.tspins them;defaultPanelspicks 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 readingangle()/fromColumnand theseriesPathtests. - Hand-checking the quaternion algebra confirms
upinprojectedCanopyAreais 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 readingSymAWEViewer.tsx. - No runtime dependency is added:
package.jsonlistsyamlandviteas 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—panelsstate survives a run change, becausesetRun(null)andsetRun(decoded)batch when thedataprop changes, sobyId.get(id)!yields undefined for an id the new run lacks andvalueRangethrows during render. Swappingdatato a run without, say, steering takes the host's tree down, and so does passing a newrunto the exportedRunPanels.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, sovel_kite/ycan 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 thevalue !== 0filter 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 plainCL [-], with no caveat in the UI.src/TimePanel.tsx:233—RunPanelsbuilds a newquantitiesarray on every render (line 349), so thisuseMemomisses 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, theframe-to-cursor effect and the one-frame guard have no test; no test mountsSymAWEViewer, and panel tests cover onlytimeAtXand static markup, not the pointer handlers. #8 requires a test per feature and §8 one for new behaviour, and the card admits hostframeand one-frame runs were not run in a browser either.README.md:66— The README says a level kite facing east has zero yaw, butkiteAttitudegives east +90° (q = 0,0,0,1), west -90° astest/quantities.test.tspins, 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-levelconsts 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,runTimesandRun.tableare public but the README does not document them (§4).- The
runQuantitiesdocstring carries a why ("A log carries every column…, so…"), which §4 forbids; the same reasoning is also in the README. scripts/export_v3_fixture.jlopens with a three-line comment block and has lines over 92 characters (§3, §10).test/quantities.test.ts:19uses the one-letter nameq(§3).liftatsrc/quantities.ts:243is 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) seven whenrunTimesfell back to frame numbers, so a run with notimecolumn shows frames as seconds. - The card says any other quantity the run holds can be added, but
COLUMNSis an allow-list;spring_forceand the rawaero_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.tsfailure 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.
| {panels.map((panel) => ( | ||
| <TimePanel | ||
| key={panel.join()} | ||
| quantities={panel.map((id) => byId.get(id)!)} |
There was a problem hiding this comment.
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.
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>
|
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. |
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.
Decisions
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
/v3_psm_sim.arrow frames carry 334 positions but its topology has 44 pointstest/topology.test.ts"reads the first n_points slots" red before, green aftertest/quantities.test.tsandtest/panels.test.tsxagainst 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.tsandtest/panels.test.tsx: the review's cases red before (9 failed of 34), green after (34 passed).tscboth configs clean ·pnpm buildclean · GitHub CI: PASS. There is noCHANGELOG.mdin this repo, so the README carries the change.Test timed out in 5000ms, on a test taking 8208 ms intest/parts.test.ts, then one intest/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.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 hostframeof 40 moves the scrubber to 40, and a one-frame run shows the 3D view with no scrubber or panels.scripts/export_v3_fixture.jlafter its constants became plain variables, since the export takes 162 min and the fixture is unchanged.Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Refs #8 · task
SymAWEViewer-8