|
| 1 | +# Modernization Roadmap |
| 2 | + |
| 3 | +The project hasn't received updates in ~4 years. This document groups the |
| 4 | +proposed improvements so they can be tracked, discussed, and later converted |
| 5 | +into individual GitHub issues. |
| 6 | + |
| 7 | +Status legend: `[ ]` not started · `[~]` in progress · `[x]` done. |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +## 1. Native / Emscripten side |
| 12 | + |
| 13 | +### 1a. Replace `EM_ASM_` in `emscripten/ARimageFsetDisplay.cpp` (lines 180–190) |
| 14 | + |
| 15 | +The block writes `frameIbwpointer`, `frameimgBWsize`, `frameFeaturePoints` |
| 16 | +onto a JS-side `arfset.frameMalloc` object. Modern, more efficient options, |
| 17 | +in order of preference: |
| 18 | + |
| 19 | +- **Best: drop it entirely.** Those same three fields are already returned |
| 20 | + through the embind `nftMarker` value_object (`pointer`, `imgBWsize`, |
| 21 | + `nftFeaturePoints` in `emscripten/bindings.cpp:46-55`). JS can read them |
| 22 | + from the returned struct instead of from a side-channel global — |
| 23 | + eliminates a JS↔wasm crossing and removes hidden global state. |
| 24 | +- **If a JS-visible global is required:** use `emscripten::val` (pure C++, |
| 25 | + type-safe, no string parser): |
| 26 | + `val::global("arfset")["frameMalloc"].set("frameIbwpointer", arc->imgBW);`. |
| 27 | +- **Last resort:** `EM_JS(void, set_frame_malloc, (...), { ... })` — |
| 28 | + declarative, parsed once at build time instead of every call site. |
| 29 | + |
| 30 | +- [ ] Pick approach (recommended: drop the EM_ASM and read from `nftMarker`). |
| 31 | +- [ ] Remove `EM_ASM_` block. |
| 32 | +- [ ] Update JS callers in `js/arfset.api.js` and `src/ARFset.js`. |
| 33 | + |
| 34 | +### 1b. Replace the `writeFP` / `writeFS` JS library (`js/jslibrary.js`) |
| 35 | + |
| 36 | +Current design has two real problems: |
| 37 | + |
| 38 | +- It's called in a per-point loop from `readNFTMarker` |
| 39 | + (`emscripten/ARimageFsetDisplay.cpp:200-206`), so each point pays a |
| 40 | + wasm→JS call. |
| 41 | +- Each call **adds a new `imageEv` event listener** that is never removed. |
| 42 | + Loading a marker with N feature points leaks N listeners; loading a |
| 43 | + second marker doubles drawing work. |
| 44 | + |
| 45 | +- [ ] Populate the already-declared `nftPoints` vector in |
| 46 | + `ARimageFsetDisplay.cpp` (the loop at lines 192–198 is commented out). |
| 47 | +- [ ] Expose feature-set points the same way (new vector field). |
| 48 | +- [ ] Delete `writeFP` / `writeFS` from `js/jslibrary.js` and remove the |
| 49 | + `--js-library` flag if nothing else needs it. |
| 50 | +- [ ] Have JS draw all points in a single pass after `imageEv` fires. |
| 51 | + |
| 52 | +### 1c. Native code bugs / cleanup in `ARimageFsetDisplay.cpp` |
| 53 | + |
| 54 | +- [ ] `loadNFTMarker` logs an error if `ar2ReadSurfaceSet` returns NULL but |
| 55 | + **continues**, then dereferences the NULL pointer (lines 109–128). |
| 56 | +- [ ] `exit(0)` / `exit(-1)` on file errors (lines 148, 154) terminate the |
| 57 | + wasm runtime in the browser — return an error code instead. |
| 58 | +- [ ] `setup()` mallocs `imgBW` with `width*height*4`, but `loadNFTMarker` |
| 59 | + overwrites the pointer with `surfaceSet[...]->imgBW` — the malloc'd |
| 60 | + buffer leaks (and so does the second `malloc` based on the |
| 61 | + uninitialized `num_F_points_NFT`). |
| 62 | +- [ ] `ARLOGi("...%d", arc->imgBW)` and `(int)arc->imgBW` truncate |
| 63 | + pointers — use `%p` / `uintptr_t`. |
| 64 | +- [ ] The `if (surfaceSetCount == PAGES_MAX) exit(-1);` check at line 153 |
| 65 | + happens *after* the array is written (line 109+); reorder and return |
| 66 | + an error. |
| 67 | + |
| 68 | +--- |
| 69 | + |
| 70 | +## 2. JavaScript API |
| 71 | + |
| 72 | +### 2a. `Module._free(debugBuffer)` is incorrect |
| 73 | + |
| 74 | +`src/ARFset.js:144` and `js/arfset.api.js:114` pass a `Uint8ClampedArray` |
| 75 | +view to `Module._free`, which expects an integer pointer. The call is a |
| 76 | +no-op at best and likely corrupts the heap if the cast happens to land on |
| 77 | +a valid pointer. |
| 78 | + |
| 79 | +- [ ] Remove the bogus `_free` call (the buffer is a view onto wasm memory |
| 80 | + managed by the C++ side; JS shouldn't free it). |
| 81 | + |
| 82 | +### 2b. Per-pixel ImageData fill is slow |
| 83 | + |
| 84 | +The `for (i,j) { id.data[j+0..2] = v; id.data[j+3] = 255 }` loop in both |
| 85 | +`src/ARFset.js:131-137` and `js/arfset.api.js:101-107` can be replaced by |
| 86 | +writing a `Uint32Array` view of `id.data` with |
| 87 | +`(0xff000000 | v<<16 | v<<8 | v)` — typically 3–5× faster. |
| 88 | + |
| 89 | +- [ ] Refactor the gray→RGBA conversion loop using a `Uint32Array` view. |
| 90 | + |
| 91 | +### 2c. Replace `axios` with `fetch` |
| 92 | + |
| 93 | +`axios 0.26.x` has unpatched CVEs (CVE-2023-45857 / SSRF). The two |
| 94 | +`Utils.js` methods just do |
| 95 | +`axios.get(url, { responseType: 'arraybuffer' })` — a one-line |
| 96 | +`fetch().then(r => r.arrayBuffer())` removes the dependency entirely. |
| 97 | + |
| 98 | +- [ ] Replace both `fetchRemoteData` and `fetchRemoteDataBlob` with |
| 99 | + `fetch`-based implementations. |
| 100 | +- [ ] Remove `axios` from `package.json` dependencies. |
| 101 | + |
| 102 | +### 2d. Delete `js/arfset.api.js` (and the asm.js builds that depend on it) |
| 103 | + |
| 104 | +The legacy global-`ARfset` API in `js/arfset.api.js` is coupled to the |
| 105 | +asm.js outputs: `tools/makem.js:176` injects it via `--pre-js` into |
| 106 | +`arfset.debug.js` and `arfset.min.js` only — the wasm and ES6-wasm |
| 107 | +builds never include it. Modern browsers cover wasm, so we drop both |
| 108 | +the legacy file and the asm.js targets in the same change. Only |
| 109 | +`example/example.html` currently consumes it; that example is migrated |
| 110 | +to the ES6 build (the existing `example_es6.html` is the template). |
| 111 | + |
| 112 | +- [ ] Delete `js/arfset.api.js`. |
| 113 | +- [ ] Delete `js/jslibrary.js` once 1b removes its `writeFP`/`writeFS` |
| 114 | + callbacks (the `--js-library` flag goes away with it). |
| 115 | +- [ ] Remove `compile_combine` and `compile_combine_min` jobs from |
| 116 | + `tools/makem.js`, plus the `PRE_FLAGS` block. |
| 117 | +- [ ] Migrate `example/example.html` to the ES6 build (or replace it |
| 118 | + with the existing `example_es6.html`). |
| 119 | +- [ ] Delete `build/arfset.min.js` and `build/arfset.debug.js`; stop |
| 120 | + generating them. |
| 121 | +- [ ] Continue to expose a `window.ARFset` global for legacy users via |
| 122 | + the webpack UMD bundle (`dist/ARFset.js` already does this) — |
| 123 | + no separate legacy entry point needed. |
| 124 | +- [ ] Note the removal in the README and bump to a major version since |
| 125 | + this is a breaking change for anyone loading `arfset.min.js`. |
| 126 | + |
| 127 | +### 2e. Magic numbers in `_setup(893, 1117)` |
| 128 | + |
| 129 | +`src/ARFset.js:249-251` hardcodes initial canvas dimensions. Make them |
| 130 | +constructor options with sensible defaults. |
| 131 | + |
| 132 | +- [ ] Accept `{ width, height }` in the `ARFset` constructor. |
| 133 | +- [ ] Update example HTML files accordingly. |
| 134 | + |
| 135 | +### 2f. Public-facing API gaps |
| 136 | + |
| 137 | +- [ ] Add TypeScript declarations (`.d.ts`) — or convert `src/` to |
| 138 | + TypeScript outright. |
| 139 | +- [ ] Add JSDoc on every public method. |
| 140 | +- [ ] Decide on naming: `nftMarkerCount` (field) vs `markerNFTCount` |
| 141 | + (local) are inconsistent. |
| 142 | + |
| 143 | +--- |
| 144 | + |
| 145 | +## 3. Build, dependencies, tests, CI, docs |
| 146 | + |
| 147 | +### 3a. Update toolchain |
| 148 | + |
| 149 | +`package.json` still pins webpack 5.70, Babel 7.17, axios 0.26 — all |
| 150 | +~4 years old. |
| 151 | + |
| 152 | +- [ ] Bump webpack, Babel, or replace webpack with **Vite/Rollup/esbuild** |
| 153 | + for an order-of-magnitude faster build. |
| 154 | +- [ ] Drop `@babel/transform-runtime` if targeting modern browsers. |
| 155 | +- [ ] Refresh `package-lock.json`. |
| 156 | + |
| 157 | +### 3b. Emscripten build flags (`tools/makem.js`) |
| 158 | + |
| 159 | +The asm.js targets are dropped as part of 2d. This item covers the |
| 160 | +remaining cleanup. |
| 161 | + |
| 162 | +- [ ] `TOTAL_MEMORY` is deprecated — switch to `INITIAL_MEMORY`. Comment |
| 163 | + says `64MB` but value is `256MB`; fix one or the other. |
| 164 | +- [ ] `var arguments = process.argv` shadows the reserved `arguments` — |
| 165 | + replace with `process.argv.slice(2)` and `for…of`. |
| 166 | +- [ ] Add a dedicated debug wasm target with `DEBUG_FLAGS` applied |
| 167 | + (currently they only affect the asm.js debug build, which is |
| 168 | + going away). |
| 169 | +- [ ] Bump the `WebARKitLib` submodule (currently pinned 4 years back). |
| 170 | + |
| 171 | +### 3c. Add a test system |
| 172 | + |
| 173 | +No tests exist today. |
| 174 | + |
| 175 | +- [ ] Pick a runner — recommend **Vitest** for unit tests + |
| 176 | + **Playwright** for a browser-level smoke test that loads a known |
| 177 | + marker and asserts the canvas is non-empty. |
| 178 | +- [ ] Add a `npm test` script. |
| 179 | +- [ ] Wire tests into CI (see 3d). |
| 180 | + |
| 181 | +### 3d. CI workflow |
| 182 | + |
| 183 | +- [ ] Add a GitHub Actions workflow that: |
| 184 | + - Checks out with submodules |
| 185 | + - Sets up Emscripten via `mymindstorm/setup-emsdk` |
| 186 | + - Runs `npm run build` and `npm run build-es6` |
| 187 | + - Runs the test suite from 3c |
| 188 | + - Uploads build artifacts on tag pushes |
| 189 | +- [ ] Add badges to README. |
| 190 | + |
| 191 | +### 3e. README + docs |
| 192 | + |
| 193 | +- [ ] Quick-start: install, build, run example. |
| 194 | +- [ ] API reference table (which methods, what they return, what events |
| 195 | + they emit). |
| 196 | +- [ ] Migration notes from the legacy `arfset.api.js` to the ES6 `ARFset` |
| 197 | + class. |
| 198 | +- [ ] Architecture diagram of the C++ ↔ wasm ↔ JS boundary. |
| 199 | +- [ ] Fix `package.json` `contributors` (currently contains a single |
| 200 | + empty string). |
| 201 | + |
| 202 | +--- |
| 203 | + |
| 204 | +## Suggested order of attack |
| 205 | + |
| 206 | +1. **1b** — fix the listener leak first; it affects every other change |
| 207 | + that touches drawing. |
| 208 | +2. **1a** — drop the EM_ASM once JS reads from `nftMarker` directly. |
| 209 | +3. **2a, 2b, 2c** — small, isolated JS fixes; easy wins. |
| 210 | +4. **1c** — native cleanup; pairs naturally with rebuilding wasm anyway. |
| 211 | +5. **2d** — delete the legacy file and asm.js builds; major-version bump. |
| 212 | +6. **3a + 3b** — toolchain refresh; gives us a clean base for tests. |
| 213 | +7. **3c + 3d** — tests and CI so regressions get caught from here on. |
| 214 | +8. **2e, 2f, 3e** — polish and docs. |
0 commit comments