chore: sync dev with master - #68
Merged
Merged
Conversation
- emcc version > 3.1.40 cause issue with isnan
… to wasm_simd128 for consistency
…lly, keyframes, and BHC clusters The matcher had three `std::unordered_map` typedefs whose iteration order depended on the STL implementation (libstdc++ on Linux, MSVC STL on Windows, libc++ on macOS / Emscripten). Code paths that iterate these maps and pick a winner-on-tie produced different results on different platforms, causing the matcher to be non-deterministic across builds. Concretely: 1. `HoughSimilarityVoting::hash_t` (vote tally) is consumed by `getMaximumNumberOfVotes`, which iterates and picks the bin with the highest count. Ties between Hough bins are common at borderline matches and were broken inconsistently per platform. 2. `VisualDatabase::keyframe_map_t` is iterated by `query()`. Ties on inlier count between keyframes are broken first-wins, so the winning keyframe at borderline ties depended on which iteration order the platform's STL chose. 3. `BinaryHierarchicalClustering::cluster_map_t` is iterated during BHC tree construction; ordering affects the resulting topology and therefore which features cluster together, which propagates into the eventual inlier set. All three typedefs become `std::map<...>`. `std::map`'s ascending- key iteration is consistent across STL implementations (and matches the BTreeMap fix on the pure-Rust port, webarkit/WebARKitLib-rs issue #170). API surface change: none. `std::map` and `std::unordered_map` share the operations used here (`operator[]`, `find`, `insert`, `erase`, `clear`, `iterator`). Performance: `O(log N)` lookup instead of `O(1) amortized`, but N is small for all three maps (number of keyframes ~1-10, number of Hough bins voted for in a query ~10s, number of BHC clusters per level ~1-100), so the difference is negligible. `VisualDatabaseImpl::point3d_map_t` in `facade/visual_database_facade.cpp` is left as `std::unordered_map` because it is used lookup-only (`map[image_id] = ...`, `return map[image_id]`); changing it has no functional benefit and would be cosmetic only. Motivation + measurements live in webarkit/WebARKitLib-rs issue #170, which has the cross-platform repro from CI.
Integrate the WebARKit OCVT tracker line: dev → master
dev had fallen behind master by 14 commits, including the WebARKitVideoLuma module, the KPM matcher determinism fix and the 1.7.6 version bump. Consumers building against master's layout could not build against dev: WebARKitVideoLuma was moved to include/AR/videoLuma.h on dev but is still expected at WebARKit/WebARKitVideoLuma.cpp. Merges cleanly with no conflicts. dev keeps its own two commits, the actions/checkout bump and the OpenCV 4.12.0 upgrade. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
github-project-automation
Bot
moved this from To do
to Done
in New markerless image tracking
This was referenced Sep 21, 2026
kalwalt
added a commit
that referenced
this pull request
Sep 21, 2026
A branch-sync PR - master back into dev after a release - must be merged with "Create a merge commit", not squashed or rebased. Squashing collapses the incoming commits into one new commit; rebasing replays them under new SHAs. Either way the target branch ends up containing the code without git recognising the two branches as related, so the next sync offers the same commits again and usually conflicts. Only a merge commit makes them genuine ancestors, which is what makes the following sync a no-op. All three merge methods are enabled on the repository and the button remembers the last one used, so this is easy to get wrong. Written down because dev had drifted 14 commits behind master and needed repairing in #68, during which consumers could not build against dev at all. Also corrects the workflow section, which told contributors to avoid `main`; the release branch is `master`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 21, 2026
kalwalt
added a commit
to webarkit/jsartoolkitNFT
that referenced
this pull request
Sep 21, 2026
ce5c280 -> 28735ee, picking up three merged PRs: webarkit/WebARKitLib#68 sync dev with master - dev had fallen 14 commits behind, including the whole WebARKitVideoLuma module, the KPM matcher determinism fix and the 1.7.6 bump webarkit/WebARKitLib#67 kpm: treat matchedId 0 as a valid match, and add ARLOGd tracing to kpmSetRefDataSet/kpmMatching webarkit/WebARKitLib#69 docs: require a merge commit when syncing long-lived branches The matchedId change corrects a real defect - db_id 0 is a legitimate image, so `!= 0` made whichever image occupied that slot unmatchable, while the matcher's own contract uses -1 as the "no match" sentinel and >= 0 as the validity test. It does *not* fix #631: measured before and after, the misattribution is identical. The ARLOGd calls are what make the --debug-logs flag in #632 do anything. They compile to nothing without it. Verified against this pointer: full emscripten build of all ten targets with no errors, build-ts clean, npm test green on seven Karma targets plus the Vitest suite, and examples/node/example_dist.js still reporting 10 NFT marker detections. Refs #631 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kalwalt
added a commit
to webarkit/jsartoolkitNFT
that referenced
this pull request
Sep 21, 2026
* chore: bump WebARKitLib to the merged dev tip ce5c280 -> 28735ee, picking up three merged PRs: webarkit/WebARKitLib#68 sync dev with master - dev had fallen 14 commits behind, including the whole WebARKitVideoLuma module, the KPM matcher determinism fix and the 1.7.6 bump webarkit/WebARKitLib#67 kpm: treat matchedId 0 as a valid match, and add ARLOGd tracing to kpmSetRefDataSet/kpmMatching webarkit/WebARKitLib#69 docs: require a merge commit when syncing long-lived branches The matchedId change corrects a real defect - db_id 0 is a legitimate image, so `!= 0` made whichever image occupied that slot unmatchable, while the matcher's own contract uses -1 as the "no match" sentinel and >= 0 as the validity test. It does *not* fix #631: measured before and after, the misattribution is identical. The ARLOGd calls are what make the --debug-logs flag in #632 do anything. They compile to nothing without it. Verified against this pointer: full emscripten build of all ten targets with no errors, build-ts clean, npm test green on seven Karma targets plus the Vitest suite, and examples/node/example_dist.js still reporting 10 NFT marker detections. Refs #631 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: rebuild artifacts after rebasing onto dev The conflicts were confined to generated files: #632 and this branch each rebuilt build/, dist/ and types/ from their own source state. Resolving them by picking a side would have left artifacts that match neither, so they are regenerated from the rebased tree - dev's sources plus the bumped submodule. Verified: full emscripten build and build-ts with no errors, npm test green on seven Karma targets and 30 Vitest assertions, and examples/node/example_dist.js still reporting 10 detections. No -D DEBUG=1 in any emcc invocation, so no tracing ships. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Brings
devup to date withmaster.The problem
devhas fallen 14 commits behindmaster, while being only 2 ahead:Those 14 are not incidental — they are most of the library's recent work:
1bfc1b2…b2dd75eWebARKitVideoLumamodule, including its SIMD support, smart-pointer refactor and namespacing2c9f630fix(kpm/matcher):std::mapfor deterministic iteration in the vote tally, keyframes and BHC clusters1dbeab5,0110870,9ddc65d656436edev's own two commits are anactions/checkoutbump and the OpenCV 4.12.0 upgrade. Both arepreserved by this merge.
Why it matters
devis not a branch you can build a consumer against.WebARKitVideoLuma.cpplives atWebARKit/WebARKitVideoLuma.cpponmaster, which is wherejsartoolkitNFT's
tools/makem.jsexpects it. Ondevthat file is absent, so the build fails outright:This matters for the contribution flow, because
CONTRIBUTING.mdsays to branch fromdevandopen PRs against
dev. Following that instruction today produces a branch that downstreamcannot build. I hit this opening #67 — that PR targets
devper the guidance, and I could notbuild-test it against jsartoolkitNFT as a result; I verified it against the pinned
mastercommit instead.
The change
A straight merge of
masterintodev. No conflicts — the merge is clean.Verification
With
devsynced, jsartoolkitNFT builds and passes against it:npm run build-tsnpm testexamples/node/example_dist.jsAgainst
devas it stands, the first of those fails immediately.Note
Once this lands, #67 should be rebased onto the updated
dev— it is a one-line change plustracing in
kpmMatching.cpp, so that should be trivial.Worth considering separately whether
devshould be kept in sync automatically after eachrelease merge to
master, since this looks like it drifted after PR #60 rather than byintent.
🤖 Generated with Claude Code