Sandpies Claude Opus 5 commited on
Commit
9672605
·
1 Parent(s): a21095e

Add pin_mech: choose the hop-2+ pin mechanism

Browse files

Detection already existed -- _motion_context_cls() resolves the upstream class
and rejects incompatible forks, run() logs the path per hop, the preview shows
it. Only the choice was missing, which made one question unanswerable: does the
AddGuide pixel path ratchet less than Motion-Context? Its VAE round trip is
itself a projection and may scrub the latent structure that accumulates, at the
cost of the join quality Motion-Context was chosen for. Asking that used to mean
uninstalling a pack.

pin_mech is auto / motion_context / addguide, default auto = current behaviour.

The override is applied in BOTH _pin_mech_for and _pin_continue. The predictor
exists to put the mechanism in the per-hop cache key before the pin runs, and
its docstring already says every condition mirrors the executor. If the two
disagree the key stops describing what is on disk -- the failure that made
cache_hops=on worse than off before the latent sidecar landed. Both now carry a
comment saying not to change one without the other.

Forcing removes the predictor's four conditions rather than adding a fifth. A
forced setting that silently degrades answers a question nobody asked and
poisons the A/B it was turned on to run, so motion_context refuses instead:

- pack not installed, and overlap with no matching context_length, are
chain-wide, so both are validated before any sampling beside the duration
checks, naming the accepted frame counts;
- no previous sampler latent is per-hop and raises at the hop it affects,
naming both ways out;
- Motion-Context raising at call time -- the one case the predictor was always
unable to foresee -- re-raises rather than downgrading.

Not added to chain_salt. The mechanism is already in the per-hop key by design,
so switching re-renders hops 2+ and hop 1 hits cache: the ideal A/B, free from
the existing structure. Keying it chain-wide would throw away a byte-identical
hop 1 on every comparison, which is the mistake the comment two lines above it
records about overlap.

The widget is APPENDED, never inserted -- widgets_values is a bare ordered array
indexed against the schema. check_workflows.py caught the other half at once (48
vs 49): both shipped workflows carry their own array and neither knew about the
new dial. Both updated; because the widget is last, no existing index moves.

js/editor/run_panel.js GROUPS lands in this commit, not a later one. A Python
widget without its JS registration falls through to a native dial, which is what
happened to five widgets in 0.4.0.

Forcing addguide sets the Motion-Context class to None internally, which walked
into the existing "Motion-Context not available; install it" line -- false, and
exactly the line a user would read while wondering why forcing did nothing.
Forced now says it was forced.

12/12 checks pass. Browser pass still owed: this needs a ComfyUI restart to load
the new schema.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013orqT1xNDezvdwv4f51MdW

docs/DEVLOG.md CHANGED
@@ -2298,3 +2298,66 @@ conclusion was reached by reasoning about what the code must be doing rather
2298
  than reading where the cost actually lands, and both times the reasoning was
2299
  written down confidently enough that it stopped anyone looking again. A user
2300
  asking "possible?" was what reopened it.
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
2298
  than reading where the cost actually lands, and both times the reasoning was
2299
  written down confidently enough that it stopped anyone looking again. A user
2300
  asking "possible?" was what reopened it.
2301
+
2302
+ ## 47. Choosing the pin, and refusing to pretend (2026-09-04)
2303
+
2304
+ The node has always known which mechanism pinned each hop.
2305
+ `_motion_context_cls()` resolves the upstream class and rejects forks whose
2306
+ `apply()` differs, `run()` logs the path each hop took, and the preview
2307
+ shows it per hop. The only thing missing was the ability to choose, which
2308
+ made a whole class of question unanswerable: does the AddGuide pixel path
2309
+ ratchet less than Motion-Context? Its VAE round trip is itself a projection
2310
+ and might scrub the off-manifold latent structure that accumulates -- at the
2311
+ cost of the join quality Motion-Context was picked for. Today the only way
2312
+ to ask was to uninstall a pack.
2313
+
2314
+ `pin_mech` is `auto` / `motion_context` / `addguide`, defaulting to `auto`,
2315
+ which is exactly what shipped.
2316
+
2317
+ The one way to get this wrong is well documented, by the code itself.
2318
+ `_pin_mech_for` predicts the mechanism so it can go in the per-hop cache key
2319
+ before the pin runs, and its docstring says "Every condition here mirrors
2320
+ _pin_continue". If the predicted mechanism and the executed one disagree,
2321
+ the key stops describing what is on disk -- which is the failure that made
2322
+ `cache_hops=on` measurably worse than off before the latent sidecar landed.
2323
+ So the override is applied in both, and both carry a comment saying not to
2324
+ change one without the other.
2325
+
2326
+ Forcing does not add a fifth condition to the predictor. It removes the
2327
+ four, which is the point. A forced setting that silently degrades to the
2328
+ other mechanism answers a question nobody asked, and worse, poisons the A/B
2329
+ it was turned on to run. So `motion_context` refuses rather than falls back.
2330
+ The two chain-wide preconditions -- pack installed, and an overlap with a
2331
+ matching `context_length` -- are checked before any sampling, next to the
2332
+ duration validation, so a bad combination fails on the queue rather than
2333
+ three hops in and names the accepted frame counts. The per-hop precondition,
2334
+ a previous hop that came from a cache entry written before latents were
2335
+ stored, cannot be known up front and raises at the hop it affects, with the
2336
+ two ways out. And if Motion-Context itself raises at call time -- the one
2337
+ thing the predictor was always unable to foresee -- a forced setting
2338
+ re-raises instead of quietly downgrading.
2339
+
2340
+ The cache work was already done, years of it in one comment. The mechanism
2341
+ is in the per-hop key and deliberately NOT in `chain_salt`, so switching
2342
+ re-renders hops 2+ and hop 1 hits cache. That is the ideal A/B and it fell
2343
+ out of the existing design with nothing added. Adding `pin_mech` to
2344
+ `chain_salt` would have thrown away a byte-identical hop 1 on every
2345
+ comparison, which is the mistake the comment two lines above it records
2346
+ about `overlap`.
2347
+
2348
+ Two things this cost that are worth writing down. The widget is APPENDED,
2349
+ never inserted: `widgets_values` is a bare ordered array indexed against the
2350
+ schema, so a widget added anywhere but the end silently reassigns every
2351
+ later value in every saved workflow. Section 27 said it in one line --
2352
+ adding options to a combo is safe, adding widgets is not. And
2353
+ `check_workflows.py` caught the other half immediately, 48 widgets against
2354
+ 49, because both shipped workflows carry their own `widgets_values` array
2355
+ and neither knew about the new dial. That check existing is the only reason
2356
+ this was a thirty-second fix rather than a bug report about a Starter that
2357
+ loads with its dials one position out.
2358
+
2359
+ Last, a message. Forcing `addguide` sets the Motion-Context class to None
2360
+ internally, which walked straight into the existing "Motion-Context not
2361
+ available; install it for a latent join" line -- false, and precisely the
2362
+ line a user would read while wondering why forcing the setting appeared to
2363
+ do nothing. Forced now says it was forced.
h3_ref_chain.py CHANGED
@@ -902,7 +902,7 @@ def _condition_pin_latent(lat, anchor, mode="off", noise=0.0, seed=0):
902
  return new, anchor
903
 
904
 
905
- def _pin_mech_for(hop_index, overlap_n, prev_sampled):
906
  """Which mechanism `_pin_continue` will pick, without doing the work.
907
 
908
  The hop cache key has to be built *before* the pin runs, and the two
@@ -911,9 +911,29 @@ def _pin_mech_for(hop_index, overlap_n, prev_sampled):
911
  predict is Motion-Context raising at call time, which the caller catches by
912
  comparing this against the mechanism actually used and declining to cache
913
  that hop.
 
 
 
 
 
 
 
 
914
  """
915
  if hop_index == 0:
916
  return "none"
 
 
 
 
 
 
 
 
 
 
 
 
917
  if _motion_context_cls() is None:
918
  return "addguide_pixels"
919
  if str(overlap_n) not in MC_CONTEXT_LENGTHS:
@@ -924,7 +944,8 @@ def _pin_mech_for(hop_index, overlap_n, prev_sampled):
924
 
925
 
926
  def _pin_continue(cond, latent, vae, audio_vae, overlap_n,
927
- prev_sampled, prev_imgs, prev_audio, audio_ctx=24):
 
928
  """Hop 2+ motion pin. Latent Motion-Context when possible; AddGuide otherwise.
929
 
930
  AddGuide re-encodes decoded pixels and anchors audio forwards from frame 0
@@ -938,7 +959,11 @@ def _pin_continue(cond, latent, vae, audio_vae, overlap_n,
938
  pin was available.
939
  """
940
  ctx_label = str(overlap_n)
941
- mc = _motion_context_cls()
 
 
 
 
942
  if mc is not None and ctx_label not in MC_CONTEXT_LENGTHS:
943
  print(
944
  f"[{TAG}] overlap {overlap_n}f has no Motion-Context context_length "
@@ -965,11 +990,27 @@ def _pin_continue(cond, latent, vae, audio_vae, overlap_n,
965
  # exception rather than swallowed -- repr(), because a bare
966
  # TypeError from a renamed upstream kwarg stringifies to nothing
967
  # useful.
 
 
 
 
 
 
 
 
 
968
  print(
969
  f"[{TAG}] Motion-Context pin failed ({e!r}); AddGuide pixel pin",
970
  flush=True,
971
  )
972
- if mc is None:
 
 
 
 
 
 
 
973
  print(
974
  f"[{TAG}] Motion-Context not available; AddGuide pixel pin "
975
  f"({overlap_n}f). Install ComfyUI-H3-Motion-Context for a latent join.",
@@ -1639,6 +1680,28 @@ class HandTieClips:
1639
  "rather than by edge."
1640
  ),
1641
  }),
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
1642
  },
1643
  "hidden": {
1644
  "unique_id": "UNIQUE_ID",
@@ -1705,6 +1768,7 @@ class HandTieClips:
1705
  music_start_s=0.0, music_end_s=0.0, render_from=0,
1706
  reference_video_desc="",
1707
  reference_video_size=None,
 
1708
  unique_id=None):
1709
  # First thing, before a single model is touched: hand the writer's VRAM
1710
  # back. The plan writer stays resident between plans now, which is the
@@ -1868,6 +1932,24 @@ class HandTieClips:
1868
  "join at 5 s.",
1869
  flush=True,
1870
  )
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
1871
  for i, ln in enumerate(lengths):
1872
  if overlap_n >= ln:
1873
  raise ValueError(
@@ -2327,7 +2409,8 @@ class HandTieClips:
2327
  # must show staleness before queuing or it reads as a bug.
2328
  hop_key = None
2329
  cached = None
2330
- pin_mech_pred = _pin_mech_for(i, overlap_n, prev_sampled)
 
2331
  pin_mech_used = pin_mech_pred
2332
  if hop_store is not None:
2333
  hop_key = _store.hop_key(prev_key, {
@@ -2438,7 +2521,7 @@ class HandTieClips:
2438
  cond, pin_mech_used = _pin_continue(
2439
  cond, latent, vae, audio_vae, overlap_n,
2440
  pin_latent, prev_imgs, prev_audio,
2441
- audio_ctx=audio_ctx,
2442
  )
2443
 
2444
  _offload_text_encoder(clip, model)
 
902
  return new, anchor
903
 
904
 
905
+ def _pin_mech_for(hop_index, overlap_n, prev_sampled, mode="auto"):
906
  """Which mechanism `_pin_continue` will pick, without doing the work.
907
 
908
  The hop cache key has to be built *before* the pin runs, and the two
 
911
  predict is Motion-Context raising at call time, which the caller catches by
912
  comparing this against the mechanism actually used and declining to cache
913
  that hop.
914
+
915
+ `mode` is the `pin_mech` widget. "auto" is the shipped behaviour and the
916
+ four conditions below. Forcing does not add a fifth condition -- it removes
917
+ them, which is the point: a forced setting that quietly degrades to the
918
+ other mechanism tells you nothing, and the reason to force one is to
919
+ compare it against the other. The two chain-wide preconditions are
920
+ validated before any sampling starts, so the only one that can still be
921
+ false here is the per-hop latent.
922
  """
923
  if hop_index == 0:
924
  return "none"
925
+ if mode == "addguide":
926
+ return "addguide_pixels"
927
+ if mode == "motion_context":
928
+ if prev_sampled is None:
929
+ raise ValueError(
930
+ f"{TAG}: hop {hop_index + 1}: pin_mech=motion_context needs the "
931
+ "previous hop's sampler latent, and this one came from a cache "
932
+ "entry written before latents were stored. Re-render that hop "
933
+ "(edit it, or turn cache_hops off for one run) or use pin_mech="
934
+ "auto, which falls back to the AddGuide pixel pin here."
935
+ )
936
+ return "motion_context"
937
  if _motion_context_cls() is None:
938
  return "addguide_pixels"
939
  if str(overlap_n) not in MC_CONTEXT_LENGTHS:
 
944
 
945
 
946
  def _pin_continue(cond, latent, vae, audio_vae, overlap_n,
947
+ prev_sampled, prev_imgs, prev_audio, audio_ctx=24,
948
+ mode="auto"):
949
  """Hop 2+ motion pin. Latent Motion-Context when possible; AddGuide otherwise.
950
 
951
  AddGuide re-encodes decoded pixels and anchors audio forwards from frame 0
 
959
  pin was available.
960
  """
961
  ctx_label = str(overlap_n)
962
+ # `mode` mirrors _pin_mech_for. Both have to honour it or the mechanism in
963
+ # the per-hop key stops matching the one on disk, which is the failure that
964
+ # made cache_hops=on measurably worse than off before the latent sidecar
965
+ # landed. Do not change one of these without the other.
966
+ mc = None if mode == "addguide" else _motion_context_cls()
967
  if mc is not None and ctx_label not in MC_CONTEXT_LENGTHS:
968
  print(
969
  f"[{TAG}] overlap {overlap_n}f has no Motion-Context context_length "
 
990
  # exception rather than swallowed -- repr(), because a bare
991
  # TypeError from a renamed upstream kwarg stringifies to nothing
992
  # useful.
993
+ if mode == "motion_context":
994
+ # Asked for explicitly, so falling back would answer a question
995
+ # nobody asked and quietly poison an A/B against the other
996
+ # mechanism.
997
+ raise RuntimeError(
998
+ f"{TAG}: pin_mech=motion_context but Motion-Context raised "
999
+ f"({e!r}). Use pin_mech=auto to fall back to the AddGuide "
1000
+ "pixel pin."
1001
+ ) from e
1002
  print(
1003
  f"[{TAG}] Motion-Context pin failed ({e!r}); AddGuide pixel pin",
1004
  flush=True,
1005
  )
1006
+ if mode == "addguide":
1007
+ # Asked for. Saying "not available" here would be false, and it is the
1008
+ # line a user would read while wondering why forcing it did nothing.
1009
+ print(
1010
+ f"[{TAG}] pin_mech=addguide: AddGuide pixel pin ({overlap_n}f)",
1011
+ flush=True,
1012
+ )
1013
+ elif mc is None:
1014
  print(
1015
  f"[{TAG}] Motion-Context not available; AddGuide pixel pin "
1016
  f"({overlap_n}f). Install ComfyUI-H3-Motion-Context for a latent join.",
 
1680
  "rather than by edge."
1681
  ),
1682
  }),
1683
+ # APPENDED, never inserted. `widgets_values` is a bare ordered
1684
+ # array matched to this schema by index, so a widget added
1685
+ # anywhere but the end silently reassigns every later value in
1686
+ # every saved workflow. Adding options to an existing combo is
1687
+ # safe; adding a widget is not.
1688
+ "pin_mech": (["auto", "motion_context", "addguide"], {
1689
+ "default": "auto",
1690
+ "tooltip": (
1691
+ "Which mechanism pins hops 2+ to the previous hop. "
1692
+ "auto = Motion-Context when the pack is installed, the "
1693
+ "overlap has a matching context_length and the previous "
1694
+ "hop left a sampler latent; AddGuide pixels otherwise. "
1695
+ "Forcing one does not fall back -- it fails with the "
1696
+ "reason, because a lever that silently becomes the "
1697
+ "other setting cannot be compared against it. "
1698
+ "motion_context: latent join, no decode/re-encode. "
1699
+ "addguide: re-encodes decoded pixels, which is itself a "
1700
+ "VAE round trip and may scrub differently. The "
1701
+ "mechanism is in the per-hop cache key, so switching "
1702
+ "re-renders hops 2+ and leaves hop 1 on disk."
1703
+ ),
1704
+ }),
1705
  },
1706
  "hidden": {
1707
  "unique_id": "UNIQUE_ID",
 
1768
  music_start_s=0.0, music_end_s=0.0, render_from=0,
1769
  reference_video_desc="",
1770
  reference_video_size=None,
1771
+ pin_mech="auto",
1772
  unique_id=None):
1773
  # First thing, before a single model is touched: hand the writer's VRAM
1774
  # back. The plan writer stays resident between plans now, which is the
 
1932
  "join at 5 s.",
1933
  flush=True,
1934
  )
1935
+ # pin_mech=motion_context: both chain-wide preconditions checked before
1936
+ # any sampling, so a forced setting fails on the queue rather than three
1937
+ # hops in. The per-hop one (no sampler latent) cannot be known here and
1938
+ # is raised by _pin_mech_for at the hop it affects.
1939
+ if str(pin_mech) == "motion_context":
1940
+ if _motion_context_cls() is None:
1941
+ raise ValueError(
1942
+ f"{TAG}: pin_mech=motion_context but ComfyUI-H3-Motion-Context "
1943
+ "is not installed. Install it, or use pin_mech=auto for the "
1944
+ "AddGuide pixel pin."
1945
+ )
1946
+ if str(overlap_n) not in MC_CONTEXT_LENGTHS:
1947
+ raise ValueError(
1948
+ f"{TAG}: pin_mech=motion_context but overlap {overlap} "
1949
+ f"({overlap_n} frames) has no Motion-Context context_length. "
1950
+ f"It accepts {', '.join(sorted(MC_CONTEXT_LENGTHS, key=int))} "
1951
+ "frames; pick an overlap with one of those, or use pin_mech=auto."
1952
+ )
1953
  for i, ln in enumerate(lengths):
1954
  if overlap_n >= ln:
1955
  raise ValueError(
 
2409
  # must show staleness before queuing or it reads as a bug.
2410
  hop_key = None
2411
  cached = None
2412
+ pin_mech_pred = _pin_mech_for(i, overlap_n, prev_sampled,
2413
+ mode=str(pin_mech))
2414
  pin_mech_used = pin_mech_pred
2415
  if hop_store is not None:
2416
  hop_key = _store.hop_key(prev_key, {
 
2521
  cond, pin_mech_used = _pin_continue(
2522
  cond, latent, vae, audio_vae, overlap_n,
2523
  pin_latent, prev_imgs, prev_audio,
2524
+ audio_ctx=audio_ctx, mode=str(pin_mech),
2525
  )
2526
 
2527
  _offload_text_encoder(clip, model)
js/editor/run_panel.js CHANGED
@@ -41,6 +41,11 @@ const GROUPS = [
41
  "control_after_generate", "seed_per_shot",
42
  "shift_video", "shift_audio"]],
43
  ["join & pin", ["hop_script", "pin_to_qwen", "ref_image_size",
 
 
 
 
 
44
  "audio_pin_frames", "pin_renorm", "pin_noise",
45
  // tone_anchor sits next to the mode it modifies: it is
46
  // read only when tone_compensate is `anchor`, and split
 
41
  "control_after_generate", "seed_per_shot",
42
  "shift_video", "shift_audio"]],
43
  ["join & pin", ["hop_script", "pin_to_qwen", "ref_image_size",
44
+ // pin_mech sits with the other pin dials even though it is
45
+ // the newest widget on the node: the panel's order is
46
+ // display only, and INPUT_TYPES appends it last so saved
47
+ // workflows keep their widget indices.
48
+ "pin_mech",
49
  "audio_pin_frames", "pin_renorm", "pin_noise",
50
  // tone_anchor sits next to the mode it modifies: it is
51
  // read only when tone_compensate is `anchor`, and split
workflows/HandTieClips_Showcase.json CHANGED
@@ -611,7 +611,8 @@
611
  0,
612
  0,
613
  "",
614
- "MAX"
 
615
  ],
616
  "title": "H3 Ref2VA Chain - showcase 6x7s"
617
  },
 
611
  0,
612
  0,
613
  "",
614
+ "MAX",
615
+ "auto"
616
  ],
617
  "title": "H3 Ref2VA Chain - showcase 6x7s"
618
  },
workflows/HandTieClips_Starter.json CHANGED
@@ -614,7 +614,8 @@
614
  0,
615
  0,
616
  "",
617
- "MAX"
 
618
  ],
619
  "title": "H3 Ref2VA Chain - starter"
620
  },
 
614
  0,
615
  0,
616
  "",
617
+ "MAX",
618
+ "auto"
619
  ],
620
  "title": "H3 Ref2VA Chain - starter"
621
  },