Repository navigation
Make the KA-frame change load against the KiteUtils 0.13 that shipped - #56
Merged
Merged
Conversation
KiteUtils 0.13 defines `quat2viewer(attitude)` and nothing else, so the two-argument call did not just mis-draw a KS log: the `@compile_workload` calls `update_system`, and the package failed to precompile. Also drops the commented-out three-line kite enlargement from the same function. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8gC66GuqKbtyojKrcQP9M
It still allowed 0.11.13 and 0.12, against the 0.13 the root project asks for, so the workspace could not resolve even in principle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8gC66GuqKbtyojKrcQP9M
1-Bort-1
commented
Sep 14, 2026
1-Bort-1
left a comment
Member
Author
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 1 inline, 0 off the diff
Good
- The two-line fix matches the released KiteUtils 0.13 API —
src/common.jl:373-374now converts withfromKS2KAand calls the single-argumentquat2viewer, which is what the precompile workload (src/KiteViewers.jl:139, defaultframe=KA) hits, sousing KiteViewerscan precompile again. - The new test pins the actual rendered value: it reads
KiteViewers.quat[], the quaternion the kite mesh is rotated by, not an intermediate, so it goes red on a missing conversion. - Scope is exactly the stated idea (+17/-9, four files); the only extra is deleting a commented-out three-line-kite block inside the same function, which §6 asks for and the card names.
examples/Project.tomlfollowing the root project toKiteUtils = "0.13"is consistent — leaving it at0.11.13, 0.12inside one workspace would be unresolvable either way.- No CHANGELOG entry is the right call and is argued: #50's
Unreleasedentry already states the behaviour, and a line saying the previous commit did not precompile is about the branch, not the release.
Not good
test/runtests.jl:8—test_frames.jlcreates aViewer3Dat top level andtest_parking.jlthen creates a second one in the same session, which CLAUDE.md calls unsupported — the two sharequat,textnodeand the rest of the module globals. The assertions survive only because test_frames reads its quaternion before test_parking starts; a later reorder or a second frame test would break silently.- The test compares the KS path against the KA path; if the KA↔KS transform is an involution (as the card implies) it would also pass with
fromKA2KSinsrc/common.jl:373— numerically equivalent, but the test does not pin the name. frame == KS ? ... : state.orienttreats every non-KSmember ofFrameConventionasKAsilently; harmless if the enum has exactly two members, worth a thought if it ever grows.- One attitude from
demo_stateis the whole coverage forframe=KS— flagged in the card, and acceptable while nothing else resolves. - Neither the new test nor the suite can actually run until KiteModels ships its 0.13 support, so this lands unverified by CI; the card is explicit about it and the PR stays draft.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3NVPfaDhK4ASic8HuBvgx
Closed
Drop Manifest-v1.11.toml.default; bin/release only checks Manifest-v1.12.toml. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…iteUtils 0.13.1 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2 of 4 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The viewer state (quat, textnode, ...) is module-global, so a second Viewer3D in the same session is not supported. Each file creates the viewer only if it does not exist yet, so both still run on their own. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ufechner7
marked this pull request as ready for review
October 11, 2026 14:42
1-Bort-1
marked this pull request as draft
October 11, 2026 14:44
Member
|
@1-Bort-1 I updated the description, you overwrote it. Why? |
Member
Author
|
Not on purpose: the bot rewrites the description from its own card at the end of every turn, and that card was older than your edit. Restored your description from the edit history (14:43:57) as the card, so the next write keeps it; only Verification gains this turn's runs. |
Member
Author
|
Local full suite: PASS (1 min, Julia 1.13.0, one cell of the matrix) |
ufechner7
marked this pull request as ready for review
October 11, 2026 14:59
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.
TL;DR
#50 was written before KiteUtils 0.13 was released and calls a two-argument
quat2viewerthat the release does not define, which stops KiteViewers precompiling at all; the caller now converts aKSattitude itself withfromKS2KA. On top of that, this branch moves to the released dependency stack: KiteUtils 0.13, KiteModels 0.12 (which writesKAorientations toSysState) and Julia 1.12/1.13. CI is green.The call that is not there
KiteUtils v0.13.0 was released today with
quat2viewer(attitude)and no second method —src/transformations.jl:140. AKSorientation is the caller's job to convert, which is what the release notes of OpenSourceAWE/KiteUtils.jl#130 say:quat2viewer(fromKS2KA(q)).This is not a wrong-looking kite on an old log.
update_systemis called from the@compile_workloadinsrc/KiteViewers.jl:139, so the method that does not exist is reached while the package is being precompiled, andusing KiteViewersfails outright:test/test_frames.jlpins what #50 promises and what this restores — that aKSattitude and theKAattitude it converts to are drawn the same way round. It reads backKiteViewers.quat[], the quaternion the kite mesh is actually rotated by, so it fails on the wrong conversion as well as on no conversion, and it does so over five attitudes rather than the demo state's one: the demo attitude, the identity, two with all four components at 0.5, and one more about a single axis. Each is written intostate.orientasKA, drawn, then written again as itsfromKA2KSimage and drawn withframe=KS. Deleting the conversion insrc/common.jlturns all five red; restoring it turns all five green.atol=1e-6rather than the default relative tolerance, which components sitting at zero can never meet.Moving to KiteModels 0.12 and Julia 1.12/1.13
Project.toml,examples/Project.toml:KiteModels = "0.12",julia = "1.12, 1.13",Pkg = "1.12, 1.13". KiteUtils 0.13 and the WinchModels and AtmosphericModels releases that accept it require Julia 1.12.Manifest-v1.11.toml.defaultremoved;Manifest-v1.12.toml.defaultupdated by a minimal resolve: KiteModels 0.12.0, KiteUtils 0.13.1, WinchModels 0.3.12, AtmosphericModels 0.3.12, KitePodModels 0.4.2 (plus two JLLs from Julia 1.12.7). All registered versions.bin/releaseno longer checks the 1.11 manifest; README and docs say Julia 1.12 or 1.13;CHANGELOG.mdhas two newUnreleasedentries.test_frames.jlandtest_parking.jlshare oneViewer3D(created only ifvieweris not defined yet), because the viewer state is module-global and a second viewer in one session is unsupported (review comment ontest/runtests.jl).Before #50 can merge
This PR targets
frame-unification(#50). After it merges there,frame-unificationneedsmainmerged in:bin/releaseconflicts:main(Support Julia 1.12 and 1.13, drop Julia 1.11 #61) checks the 1.12 and 1.13 manifests, this branch only 1.12. Takemain's version.CHANGELOG.mdconflicts: both add anUnreleasedsection; combine them.Manifest-v1.13.toml.defaultresolved for KiteModels 0.12 / KiteUtils 0.13 (mainhas one for the old stack).Then KiteViewers can be released as v0.7.0 (breaking:
update_systemtakesframe=instead ofned), after which OpenSourceAWE/KiteModels.jl can drop its[sources]lines that point at this branch.Found on the way, not fixed here
steering_4p.jl,depower_simple.jl,joystick.jl,reelout_*.jl, the*_bench_video.jlpair) andtest_parking.jl/test_steering.jlall callupdate_systemwith the default frame. That is correct now that KiteModels.jl#312 is released (KiteModels 0.12 writesKA), and these examples need KiteModels 0.12 anyway, so there is nothing to write differently..github/workflows/CI.ymlcarriesfail-fast: false. One matrix cell today, so it costs nothing yet.CHANGELOG.mdentry for thequat2viewerfix: Take the orientation convention as a keyword, notned#50'sUnreleasedentry already describes the behaviour this makes true. The entries this branch adds are for the KiteModels 0.12 and Julia 1.12/1.13 requirements.bin/installwas rewritten in cleanup: bin/install takes -y, -u and -h and touches nothing outside the repo #55 and moved to Julia 1.12 and 1.13 in Support Julia 1.12 and 1.13, drop Julia 1.11 #61, both merged intomain; this branch is stacked on Take the orientation convention as a keyword, notned#50 and picks them up onceframe-unificationis updated frommain(see below).Verification
Failed to precompile KiteViewers,MethodError: no method matching quat2viewer(::FrameQuat, ::FrameConvention)— full output abovetest/test_frames.jlred before, green after, in a KiteUtils 0.13.0 environment (5 passed;0 passed, 5 failedwith the conversion removed)test_frames5/5,KiteViewers.jl3/3; re-run from the tracked manifest with no local overrides (Pkg.resolve()moved nothing; Julia 1.12.7,examplesenv, exit 0) and asPkg.test()on Julia 1.13.0 (PASS, exit 0)update_systemis already ondocs/src/reference.mdtest_parking.jldrives the viewer from a real KPS4 run writing KA, but asserts position and force, not the drawn orientation; that the kite is drawn the right way round is proven only for the five hand-written attitudes intest_frames.jl.Scope
+58 / −2017 across 12 files against
frame-unification; −1984 of that isManifest-v1.11.toml.default. The code change issrc/common.jl(two lines of conversion and the deletion of a commented-out three-line-kite block);test/test_frames.jlis new.Opened elsewhere
Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #54 · task
KiteViewers.jl-54