feat: Improved performance by optimizing the HitTest service and its interaction with the scheduler #101
Conversation
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
Reviewer's GuideThis PR overhauls the hit testing workflow by integrating a centralized scheduler and reactive Preact signals, refactors components and APIs to leverage the new reactive usableRect, optimizes path rendering via chunking and caching, and streamlines event handling across graph and block layers. Sequence diagram for event capture and release during drag operationssequenceDiagram
actor User
participant GraphComponent
participant GraphLayer
participant EventedComponent
User->>GraphComponent: drag start
GraphComponent->>GraphLayer: captureEvents(this)
Note right of GraphLayer: Sets capturedTargetComponent
User->>GraphComponent: drag end
GraphComponent->>GraphLayer: releaseCapture()
Note right of GraphLayer: Unsets capturedTargetComponent
Class diagram for the refactored HitTest service and related classesclassDiagram
class HitTest {
+signal usableRect
+load(items: HitBox[])
+update(item: HitBox, bbox: HitBoxData)
+add(item: HitBox)
+remove(item: HitBox)
+clear()
+destroy()
+testPoint(point: IPoint, pixelRatio: number)
+testHitBox(item: HitBoxData)
}
class HitBox {
+updateRect(rect: HitBoxData)
+update(minX, minY, maxX, maxY, force?)
+getRect()
}
HitTest "1" -- "*" HitBox : manages
class ESchedulerPriority {
HIGHEST
HIGH
MEDIUM
LOW
LOWEST
}
class GlobalScheduler {
+addScheduler(scheduler, index)
+removeScheduler(scheduler, index)
}
class schedule {
<<function>>
}
schedule ..> ESchedulerPriority : uses
HitTest ..> schedule : uses
GlobalScheduler ..> ESchedulerPriority : uses
schedule ..> GlobalScheduler : uses
class signal {
<<Preact signal>>
}
HitTest --> signal : exposes usableRect
Class diagram for optimized BatchPath2DRenderer and path chunkingclassDiagram
class BatchPath2DRenderer {
+add(item: Path2DRenderInstance, ...)
+delete(item: Path2DRenderInstance)
+markDirty(item: Path2DRenderInstance)
+reset()
+render(ctx)
}
class Path2DGroup {
+add(item: Path2DRenderInstance)
+delete(item: Path2DRenderInstance)
+resetItem(item: Path2DRenderInstance)
+render(ctx)
}
class Path2DChunk {
+add(item: Path2DRenderInstance)
+delete(item: Path2DRenderInstance)
+reset()
+render(ctx)
+size
}
BatchPath2DRenderer --> Path2DGroup : manages
Path2DGroup --> Path2DChunk : contains
Path2DChunk --> Path2DRenderInstance : contains
Class diagram for Background and MiniMapLayer usableRect refactorclassDiagram
class Background {
-extendedUsableRect: IRect
+state: TBackgroundState
+subscribe()
+setupExtendedUsableRect(usableRect: TRect)
+updateChildren()
}
class MiniMapLayer {
+didRender()
-unSubscribeUsableRectLoaded
}
Background ..> signal : subscribes to usableRect
MiniMapLayer ..> signal : subscribes to usableRect
Class diagram for Graph, GraphLayer, and event capture changesclassDiagram
class Graph {
+scheduleTask(cb: () => void)
+getGraphLayer()
+zoomTo(target, config?)
}
class GraphLayer {
+captureEvents(component: EventedComponent)
+releaseCapture()
+handleEvent(e: Event)
-capturedTargetComponent
}
Graph --> GraphLayer : manages
GraphLayer --> EventedComponent : captures events for
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Preview is ready. |
…interaction with the scheduler.
draedful
force-pushed
the
usable_rect_perf
branch
from
2e5f057 to
0f33ee5
Compare
fix(ReactBlock): fix performance issue fix(Layer): use current devicePixelRation in Layer fix(Camera): fix tracking trackpad on pan event with browser zoom
draedful
force-pushed
the
usable_rect_perf
branch
from
1a1ccef to
d4953e1
Compare
feat: add isPathVisible method to BlockConnection and ConnectionArrow, improve path visibility handling in BatchPath2D
draedful
force-pushed
the
usable_rect_perf
branch
from
d4953e1 to
0da1dc7
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @draedful - I've reviewed your changes - here's some feedback:
- In Background.setupExtendedUsableRect, you’re calling setState up to four times per update—batch those rectangle changes into a single setState call to avoid extra renders.
- The HitTest scheduler always defers updates by your frameInterval; consider exposing a flush or synchronous update method so initial usableRect values aren’t delayed.
- The hard-coded CHUNK_SIZE in your Path2DGroup could be made configurable (or extracted to a constant) so you can tune performance without touching the class internals.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In Background.setupExtendedUsableRect, you’re calling setState up to four times per update—batch those rectangle changes into a single setState call to avoid extra renders.
- The HitTest scheduler always defers updates by your frameInterval; consider exposing a flush or synchronous update method so initial usableRect values aren’t delayed.
- The hard-coded CHUNK_SIZE in your Path2DGroup could be made configurable (or extracted to a constant) so you can tune performance without touching the class internals.
## Individual Comments
### Comment 1
<location> `src/services/HitTest.ts:115` </location>
<code_context>
- if (force) {
- this.scheduleLoad.flush();
- }
+ public add(item: HitBox) {
+ this.scheduledItems.set(item, item);
}
</code_context>
<issue_to_address>
Adding item to scheduledItems with itself as bbox may cause issues.
scheduledItems should use a HitBox as the key and a HitBoxData as the value. Passing item as both may cause type or logic errors. Please ensure the correct value type is used.
</issue_to_address>
### Comment 2
<location> `src/services/LayersService.ts:17` </location>
<code_context>
constructor(public $root?: HTMLDivElement) {
super();
+ window.addEventListener("resize", this.handleRootResize);
}
</code_context>
<issue_to_address>
Potential for multiple event listeners if constructor is called multiple times.
Multiple Layers instances will each add a resize event listener to the window. Consider preventing duplicate listeners or clarifying usage expectations.
Suggested implementation:
```typescript
private _resizeListenerAdded = false;
constructor(public $root?: HTMLDivElement) {
super();
this.handleRootResize = this.handleRootResize.bind(this);
if (!this._resizeListenerAdded) {
window.addEventListener("resize", this.handleRootResize);
this._resizeListenerAdded = true;
}
}
```
```typescript
public unmount() {
this.detach(true);
this.destroy();
if (this._resizeListenerAdded) {
window.removeEventListener("resize", this.handleRootResize);
this._resizeListenerAdded = false;
}
```
</issue_to_address>
### Comment 3
<location> `src/services/HitTest.ts:44` </location>
<code_context>
+ public usableRect = signal<TRect>({ x: 0, y: 0, width: 0, height: 0 });
+
+ protected scheduledItems = new Map<HitBox, HitBoxData>();
+ protected scheduledRemoveItems = new Set<HitBox>();
+
</code_context>
<issue_to_address>
Consider replacing multiple state-tracking structures and custom scheduling logic with a single queue and one debounced job to handle all add, update, and remove operations.
Here’s one way to collapse all of those flags/sets/maps and two flows (add/update vs remove vs emit) into a single queue + one scheduled job. You keep exactly the same semantics—items “null”‐marked get removed, non‐null get updated—then you do your tree updates, recompute usableRect if needed, emit once, and clear the queue:
```ts
// 1) Replace both maps/sets + needEmit flag
protected queue = new Map<HitBox, HitBoxData|null>();
// 2) One scheduled job (debounce or your scheduler)
protected processQueue = debounce(() => {
let needUpdRect = false;
for (const [item, bbox] of this.queue) {
if (bbox === null) {
this.tree.remove(item);
} else {
this.tree.remove(item);
item.updateRect(bbox);
this.tree.load([item]);
needUpdRect = true;
}
}
this.queue.clear();
if (needUpdRect) this.updateUsableRect();
this.emit("update", this);
}, 50);
// 3) This replaces add/update/remove/emitUpdate
public add(item: HitBox) {
this.queue.set(item, item);
this.processQueue();
}
public update(item: HitBox, bbox: HitBoxData) {
this.queue.set(item, bbox);
this.processQueue();
}
public remove(item: HitBox) {
this.queue.set(item, null);
this.processQueue();
}
```
That single `debounce` removes all the manual frame counting, `needEmitUpdate`, `scheduledRemoveItems`, `removeSchedulersFns` plumbing, and still:
- coalesces rapid calls
- batches adds/updates/removes
- updates `usableRect` when any bbox changed
- emits `"update"` exactly once per batch
This should collapse ~100 lines of custom scheduler into <30, dramatically improving readability while preserving functionality.
</issue_to_address>
### Comment 4
<location> `src/components/canvas/connections/BatchPath2D/index.tsx:22` </location>
<code_context>
+class Path2DChunk {
protected items: Set<Path2DRenderInstance> = new Set();
+ protected visibleItems = cache(() => {
+ return Array.from(this.items).filter((item) => item.isPathVisible?.() ?? true);
+ });
</code_context>
<issue_to_address>
Consider centralizing visibility filtering into a single cached list to eliminate repeated inline visibility checks in rendering and styling methods.
Here’s a small-focused refactor of `Path2DChunk` that removes the duplicate visibility checks and centralizes all filtering into one `visibleItems` cache. You can apply the same pattern to your `render`/`applyStyles` and drop the inlined `.isPathVisible` checks in the `reduce`:
```ts
class Path2DChunk {
protected items = new Set<Path2DRenderInstance>();
// 1) One cache → only-visible items
protected visibleItems = cache(() =>
Array.from(this.items).filter(i => i.isPathVisible?.() ?? true)
);
// 2) One cache → combined Path2D of visible items
protected path = cache(() => {
const p = new Path2D();
p.moveTo(0, 0);
for (const item of this.visibleItems.get()) {
const sub = item.getPath();
if (sub) p.addPath(sub);
}
return p;
});
// 3) Style comes from first visible
protected applyStyles(ctx: CanvasRenderingContext2D) {
const first = this.visibleItems.get()[0];
return first?.style(ctx);
}
public add(item: Path2DRenderInstance) {
this.items.add(item);
this.reset();
}
public delete(item: Path2DRenderInstance) {
this.items.delete(item);
this.reset();
}
public reset() {
this.visibleItems.reset();
this.path.reset();
}
public render(ctx: CanvasRenderingContext2D) {
const vis = this.visibleItems.get();
if (!vis.length) return;
ctx.save();
const style = this.applyStyles(ctx);
if (style) {
const p = this.path.get();
if (style.type === "fill" || style.type === "both") {
ctx.fill(p, style.fillRule);
}
if (style.type === "stroke" || style.type === "both") {
ctx.stroke(p);
}
}
ctx.restore();
for (const item of vis) {
item.afterRender?.(ctx);
}
}
}
```
Benefits:
- removes all inline `(item.isPathVisible?..)` checks
- uses a single `visibleItems` cache for both styling and path-building
- avoids the extra `reduce` + manual filter in two places
- keeps exactly the same behavior and cache‐reset logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Antamansid
reviewed
Jul 2, 2025
Antamansid
reviewed
Jul 2, 2025
Antamansid
reviewed
Jul 2, 2025
Antamansid
reviewed
Jul 2, 2025
Collaborator
|
Tested on the project: it's just a class. There are a couple of small ts errors during build, but these are the little things. |
Antamansid
approved these changes
Jul 3, 2025
…ve usability; add PATH2D_CHUNK_SIZE constant for batch rendering; update Background component to use debounced state updates
draedful
force-pushed
the
usable_rect_perf
branch
from
fd68e87 to
a8ea93e
Compare
draedful
changed the title
… viewport threshold and improve hit test stability checks
draedful
force-pushed
the
usable_rect_perf
branch
from
7b5aa53 to
6dfe38f
Compare
…e checks with waitUsableRectUpdate method
draedful
force-pushed
the
usable_rect_perf
branch
from
6dfe38f to
caa65c5
Compare
This was referenced Jul 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by Sourcery
Use a priority scheduler to replace debounce-based throttling across hit testing, graph zooming, and rendering pipelines. Expose the HitTest usable area as a reactive signal, implement chunked Path2D batching, and optimize React components and event handling to reduce redundant updates and improve overall performance.
New Features:
Enhancements: