Repository navigation
fix(cli): measure ink, one output-folder flag spelling, logo wordmark variant - #6891
Conversation
… logo --variant wordmark measure ink reports the bounds, size and lines of text in a region of one frame, so a rebuild can be sized without a script. measure and snapshot take --out and --output for the same folder. media-use resolve --type logo --variant wordmark returns the brand's wordmark when theSVG has one.
There was a problem hiding this comment.
Requesting changes at 988a0b5d. Two blockers:
1. A --variant resolve leaves its artwork in the default logo's cache slot. --variant skips reuse on its own way in (forced), but run() still saves the fetched wordmark like any other result: appendRecord(projectDir, record) plus cachePut(fullPath, record), with provenance.prompt = intent and entity. None of the readers look at provenance.variant:
findByPromptinlib/manifest.mjsfindByEntitycacheGet/cacheGetByEntityinlib/cache.mjs
Repro: resolve --type logo --intent "Slack logo" --variant wordmark, then resolve --type logo --intent "Slack logo" in the same project, or in any project through the global cache. The second call returns the wordmark as cached (.find gives back the first record that matches). This is the reverse of the swap the PR guards against.
Fix: either keep variant records out of reuse (don't cachePut, and don't key them on the bare intent), or make the four lookups reject a record whose provenance.variant is set and not "default". Add a test that resolves a variant and then the default for the same intent and expects the default mark.
2. The body has no fails without the fix: line. This lane counts it as a fix PR (fix(cli)), and the --out/--output change fixes a bug: each command rejected the other spelling. The body needs a fails without the fix: line naming a test that fails without the fix. Right now there is no test for the aliases at all: no test parses measure frames --output, measure extract --output, or snapshot --out and checks they reach out/output. The test plan says the ink and variant tests fail when their code is broken, but neither appears on a fails without the fix: line.
Smaller, not blocking:
- The body says the aliases are
output(measure) andout(snapshot). The code also addsoutput-dirto all three. Update the body, or dropoutput-dirif it wasn't meant. - The GitHub title is
feat(cli): …, but the lane pins it asfix(cli): … check false positives in a reverse. Thefirst_frame_emptypart was left out (explained in the body). Make the title match what ships.
Looks right otherwise: measureInk bounds and bands (with +1), projectOf moved to media.ts without changing what it does, and the avatar and favicon tiers returning nothing for a variant.
There was a problem hiding this comment.
Approving 1fcd3a00. Both blockers from my last review are fixed.
Variant logo no longer reused for the default logo. A variant resolve now records its prompt as <intent> [<variant>], leaves out entity, and skips cachePut. None of the four lookups can return it for a default request any more. I checked it two ways:
- The new
resolve.test.mjstest, "a logo variant is not reused for the default logo, in the project or the global cache", passes: 63/63. - With
resolve.mjsput back to988a0b5d, the same test fails: 62 passed, 1 failed. So thefails without the fix:line is accurate.
fails without the fix: line. It is present and names measure/outFlags.test.ts and the resolve test. The body now also lists the output-dir alias, and the title matches what ships.
CI run 41246115131 is green.
Nit, not blocking: outFlags.test.ts checks the alias definitions rather than parsing argv through citty. That is fine because citty resolves aliases into the main key, but a parse-level case would also cover the required: true --out on extract.
What
Three small CLI gaps found while rebuilding a 77 s animated film with fast camera moves and light flashes from a reference video:
hyperframes measure ink <video> --at <s> --roi x,y,w,hreports the bounds, size and text lines of the ink in a region of one frame, in the video's own pixels. No numpy script needed to size text.measureandsnapshotboth accept--out,--outputand--output-dir(andsnapshot -o) for the output folder. Before,measure frames --outandsnapshot --outputeach rejected the other spelling.media-use resolve --type logo --variant wordmarkreturns the brand's wordmark from theSVG (alsomono,light,dark, ...). A brand without that variant is a miss, never the mark or an avatar in its place.Why
Sizing text from its ink was the one step in the reverse workflow that needed hand-written code. The two flag spellings were an easy trap, and
resolve --type logoonly ever returned the default mark although theSVG ships a wordmark for many brands.Related work
Partial:
first_frame_emptyfalse positive is not included, see below.How
measure/ink.tsreuses the existing ink definition inmeasure/drawn.ts(inkOf: a pixel is ink when its colour is far from its neighbourhood's median), then takes bounds and row bands.outArgandextract'soutgain the aliasesoutputandoutput-dir; snapshot'soutputgainsoutandoutput-dir(same pattern astts.ts).--variantgoes throughctxtothesvgSearch. A variant bypasses the intent and entity caches on the way in, and is never saved back to them (no global-cache put, no entity, its recorded prompt carries the variant), so a later default request for the same intent or entity cannot return the wordmark. The avatar and favicon tiers return nothing for a variant.first_frame_emptydid not reproduce as reported. On main, a full-frame scene atz-index: 10over a title at 0 s gives nofirst_frame_emptyand nocanvas_coveredwhen the scene holds a div, an svg or text; a scene starting at 3 s gives none at 0 s. It fires only for a bare opaque slab with nothing in it, where frame 0 really is empty andcanvas_coveredalso errors. Left unchanged.Not done, needs a product call:
text_below_floorandcontrast_aa_failurein a reverse, where the original's own tiny text and colours trip them. Proposal: a--match-originalmode that skips those two checks for text present in the reference.Test plan
fails without the fix:
measure/outFlags.test.ts(each command's output flag takesout,output,output-dir; red with the alias removed) andresolve.test.mjs"a logo variant is not reused for the default logo" (resolves the wordmark, then the default in the same project and in another project through the global cache; red with the variant saved back to the cache).measure/ink.test.ts(bounds shifted to the region's place, one band per line, null when empty); breaking the +1 in the width makes it fail.logo-provider.test.mjs(wordmark url, miss when absent, no avatar or favicon for a variant); ignoring the variant makes it fail. 23 pass, 3 runs.Manual on a generated clip:
measure ink --at 1 --roi 40,100,500,160printedbounds [100,140,300,50]for a 300x50 white box;measure frames --outand--outputboth wrote their stills.Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug