-
Notifications
You must be signed in to change notification settings - Fork 10
fix: hittest pending entities update #297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d6875a7
a9e984a
1a31d7b
aa0550f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| import { Graph } from "../graph"; | ||
|
|
||
| import { HitBox, HitTest } from "./HitTest"; | ||
|
|
||
| function makeHitTest(hasBlocks = false): HitTest { | ||
| const mockGraph = { | ||
| rootStore: { | ||
| blocksList: { $blocks: { value: hasBlocks ? [{}] : [] } }, | ||
| connectionsList: { $connections: { value: [] } }, | ||
| }, | ||
| } as unknown as Graph; | ||
| return new HitTest(mockGraph); | ||
| } | ||
|
|
||
| function seedUsableRect(ht: HitTest): void { | ||
| const fakeHitBox = { | ||
| affectsUsableRect: true, | ||
| destroyed: false, | ||
| minX: 0, | ||
| minY: 0, | ||
| maxX: 100, | ||
| maxY: 100, | ||
| x: 0, | ||
| y: 0, | ||
| } as unknown as HitBox; | ||
| (ht as unknown as { usableRectTracker: { add(h: HitBox): void } }).usableRectTracker.add(fakeHitBox); | ||
| (ht as unknown as { updateUsableRect(): void }).updateUsableRect(); | ||
| } | ||
|
|
||
| /** | ||
| * Trigger processQueue by queueing a fake hitbox update and flushing. | ||
| * This mirrors real production code: hitbox updates always precede processQueue. | ||
| */ | ||
| function triggerProcessQueue(ht: HitTest): void { | ||
| const fakeHitBox = { | ||
| affectsUsableRect: true, | ||
| destroyed: false, | ||
| minX: 0, | ||
| minY: 0, | ||
| maxX: 100, | ||
| maxY: 100, | ||
| x: 0, | ||
| y: 0, | ||
| updateRect(_bbox: unknown): void { | ||
| // no-op for test fake | ||
| }, | ||
| } as unknown as HitBox; | ||
| ht.update(fakeHitBox, { minX: 0, minY: 0, maxX: 100, maxY: 100, x: 0, y: 0 }); | ||
| (ht as unknown as { processQueue: { flush(): void } }).processQueue.flush(); | ||
| } | ||
|
|
||
| describe("HitTest.markPendingUpdate", () => { | ||
| it("makes isUnstable true immediately after call", () => { | ||
| const ht = makeHitTest(true); | ||
| // Seed non-zero usableRect so zero-rect heuristic doesn't interfere | ||
| seedUsableRect(ht); | ||
| expect(ht.isUnstable).toBe(false); | ||
| ht.markPendingUpdate(); | ||
| expect(ht.isUnstable).toBe(true); | ||
| }); | ||
|
|
||
| it("isUnstable becomes false after processQueue flushes with a hitbox update", () => { | ||
| const ht = makeHitTest(true); | ||
| seedUsableRect(ht); | ||
| ht.markPendingUpdate(); | ||
| expect(ht.isUnstable).toBe(true); | ||
| // processQueue fires naturally when hitbox updates arrive; simulate that here | ||
| triggerProcessQueue(ht); | ||
| expect(ht.isUnstable).toBe(false); | ||
| }); | ||
|
|
||
| it("waitUsableRectUpdate resolves when flag clears, even if usableRect did not change", () => { | ||
| const ht = makeHitTest(true); | ||
| seedUsableRect(ht); | ||
|
|
||
| ht.markPendingUpdate(); | ||
| expect(ht.isUnstable).toBe(true); | ||
|
|
||
| let called = false; | ||
| ht.waitUsableRectUpdate(() => { | ||
| called = true; | ||
| }); | ||
| expect(called).toBe(false); // still waiting | ||
|
|
||
| // processQueue fires when hitbox updates arrive (simulated here) | ||
| triggerProcessQueue(ht); | ||
| expect(called).toBe(true); // resolved after flag cleared | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,10 @@ export class HitTest extends Emitter { | |
|
|
||
| public readonly $usableRect = signal<TRect>({ x: 0, y: 0, width: 0, height: 0 }); | ||
|
|
||
| // Explicit pending flag set by setEntities; cleared when processQueue completes. | ||
| // Avoids the "zero usableRect = unstable" heuristic for entity replacement. | ||
| private readonly $pendingEntitiesUpdate = signal(false); | ||
|
|
||
| // Single queue replaces all complex state tracking | ||
| protected queue = new Map<HitBox, HitBoxData | null>(); | ||
|
|
||
|
|
@@ -90,10 +94,10 @@ export class HitTest extends Emitter { | |
|
|
||
| // If graph has no elements, it's stable even with zero usableRect | ||
| if (hasZeroUsableRect && !this.hasGraphElements()) { | ||
| return hasProcessingQueue; | ||
| return hasProcessingQueue || this.$pendingEntitiesUpdate.value; | ||
| } | ||
|
|
||
| return hasProcessingQueue || hasZeroUsableRect; | ||
| return hasProcessingQueue || hasZeroUsableRect || this.$pendingEntitiesUpdate.value; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -148,6 +152,7 @@ export class HitTest extends Emitter { | |
| this.queue.clear(); | ||
| this.updateUsableRect(); | ||
| this.emit("update", this); | ||
| this.$pendingEntitiesUpdate.value = false; | ||
| }, | ||
| { | ||
| priority: ESchedulerPriority.LOWEST, | ||
|
|
@@ -176,6 +181,7 @@ export class HitTest extends Emitter { | |
| this.interactiveTree.clear(); | ||
| this.usableRectTracker.clear(); | ||
| this.updateUsableRect(); | ||
| this.$pendingEntitiesUpdate.value = false; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -188,6 +194,20 @@ export class HitTest extends Emitter { | |
| this.updateUsableRect(); | ||
| } | ||
|
|
||
| /** | ||
| * Mark hitTest as pending an entity update (called by setEntities). | ||
| * Makes isUnstable = true until processQueue completes. | ||
| * Does NOT clear usableRectTracker — existing data stays valid. | ||
| * Does NOT eagerly schedule processQueue — it fires naturally when hitbox | ||
| * updates arrive (new components register) or when updateBlock forces a hitbox | ||
| * refresh on existing components. Eager scheduling caused processQueue to run | ||
| * in the same rAF frame as markPendingUpdate (before MEDIUM/component renders), | ||
| * clearing $pendingEntitiesUpdate before new block hitboxes were queued. | ||
| */ | ||
| public markPendingUpdate(): void { | ||
| this.$pendingEntitiesUpdate.value = true; | ||
| } | ||
|
|
||
| /** | ||
| * Add new HitBox item | ||
| * @param item HitBox item to add | ||
|
|
@@ -213,15 +233,25 @@ export class HitTest extends Emitter { | |
| } | ||
|
|
||
| if (this.isUnstable) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (complexity): Consider introducing a single stability tick signal and a local pending flag variable to simplify waitUsableRectUpdate subscriptions and the isUnstable logic. You can keep the new behavior but move the coordination logic out of 1. Centralize the “stability-related changes” into a single signalInstead of dual subscriptions + manual cleanup in // New field
private readonly $stabilityTick = signal(0);
constructor(protected graph: Graph) {
super();
// Any change that could affect stability increments the tick
this.$usableRect.subscribe(() => {
this.$stabilityTick.value++;
});
this.$pendingEntitiesUpdate.subscribe(() => {
this.$stabilityTick.value++;
});
}Then public waitUsableRectUpdate(callback: (rect: TRect) => void): () => void {
// For empty graphs, immediately call callback with current usableRect
if (!this.hasGraphElements()) {
callback(this.$usableRect.value);
return noop;
}
if (this.isUnstable) {
const unsubscribe = this.$stabilityTick.subscribe(() => {
if (!this.isUnstable) {
unsubscribe();
callback(this.$usableRect.value);
}
});
return unsubscribe;
}
callback(this.$usableRect.value);
return noop;
}Behavior stays the same: we still wake up when either rect changes or the pending flag flips, but the complexity is now in one small place (the constructor wiring), and 2. Reduce duplication in
|
||
| const removeListener = this.$usableRect.subscribe(() => { | ||
| let cleaned = false; | ||
| let unsubRect: () => void = noop; | ||
| let unsubPending: () => void = noop; | ||
| const cleanup = () => { | ||
| if (cleaned) return; | ||
| cleaned = true; | ||
| unsubRect(); | ||
| unsubPending(); | ||
| }; | ||
| const check = () => { | ||
| if (!this.isUnstable) { | ||
| removeListener(); | ||
| cleanup(); | ||
| // eslint-disable-next-line callback-return | ||
| callback(this.$usableRect.value); | ||
| return; | ||
| } | ||
| return; | ||
| }); | ||
| return removeListener; | ||
| }; | ||
| unsubRect = this.$usableRect.subscribe(check); | ||
| unsubPending = this.$pendingEntitiesUpdate.subscribe(check); | ||
| return cleanup; | ||
| } | ||
| callback(this.$usableRect.value); | ||
| return noop; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.