feat: using culling render #22

nonzzz · 2025-11-11T14:12:22Z

TL;DR

It's WIP now.

CheckList

Refactor the current highlighting implementation
Using Quadtree improve speed
Text Layout

nonzzz · 2025-11-11T15:44:02Z

Copilot · 2025-11-11T15:44:15Z

@nonzzz I've opened a new pull request, #23, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot

Pull Request Overview

This PR implements viewport culling optimization using a Quadtree spatial data structure to improve rendering performance. The implementation adds a generic widget system to graph objects and modifies the rendering pipeline to skip drawing elements outside the visible viewport.

Key changes:

Quadtree-based spatial indexing for efficient viewport culling
Generic widget support added to graph classes (Text, RoundRect, Graph) for attaching custom data
Modified rendering to check visibility before drawing elements

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
`src/etoile/schedule/index.ts`	Implements QuadTree class and culling logic in the rendering pipeline; adds `collectBoundingGraphics` helper
`src/etoile/native/matrix.ts`	Adds `transformPoint` method for coordinate transformations
`src/etoile/graph/display.ts`	Adds generic widget support and optional ID parameter to Display constructors
`src/etoile/graph/text.ts`	Updates Text class with generic widget parameter and modified clone method
`src/etoile/graph/rect.ts`	Updates RoundRect class with generic widget parameter and modified clone method
`src/etoile/graph/box.ts`	Adds optional ID parameter to Box constructors and updates clone method
`src/etoile/graph/types.ts`	Adds generic type parameter to `isText` type guard
`src/shared/index.ts`	Adds generic widget parameters to helper functions
`src/plugins/drag.ts`	Updates drag logic to handle widget data and element cloning
`src/component.ts`	Fixes method name typo (drawBroundRect → drawRoundRect) and attaches widget data to created elements

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-11T15:48:19Z

src/etoile/schedule/index.ts

+    const p1 = matrix.transformPoint(x, y)
+    const p2 = matrix.transformPoint(x + width, y + height)
+
+    const bbox = <BBox> {
+      x: Math.min(p1.x, p2.x),
+      y: Math.min(p1.y, p2.y),
+      width: Math.abs(p2.x - p1.x),
+      height: Math.abs(p2.y - p1.y)


The bounding box calculation only considers two corner points (top-left and bottom-right), which is insufficient when transformations involving rotation or skew are applied. A rotated or skewed rectangle's axis-aligned bounding box requires checking all four corners to find the correct min/max values.

For accurate bounding boxes with arbitrary transformations, transform all four corners:

const corners = [ matrix.transformPoint(x, y), matrix.transformPoint(x + width, y), matrix.transformPoint(x, y + height), matrix.transformPoint(x + width, y + height) ] const bbox = { x: Math.min(...corners.map(c => c.x)), y: Math.min(...corners.map(c => c.y)), width: Math.max(...corners.map(c => c.x)) - Math.min(...corners.map(c => c.x)), height: Math.max(...corners.map(c => c.y)) - Math.min(...corners.map(c => c.y)) }

Suggested change

const p1 = matrix.transformPoint(x, y)

const p2 = matrix.transformPoint(x + width, y + height)

const bbox = <BBox> {

x: Math.min(p1.x, p2.x),

y: Math.min(p1.y, p2.y),

width: Math.abs(p2.x - p1.x),

height: Math.abs(p2.y - p1.y)

const corners = [

matrix.transformPoint(x, y),

matrix.transformPoint(x + width, y),

matrix.transformPoint(x, y + height),

matrix.transformPoint(x + width, y + height)

]

const minX = Math.min(...corners.map(c => c.x))

const maxX = Math.max(...corners.map(c => c.x))

const minY = Math.min(...corners.map(c => c.y))

const maxY = Math.max(...corners.map(c => c.y))

const bbox = <BBox> {

x: minX,

y: minY,

width: maxX - minX,

height: maxY - minY

Copilot · 2025-11-11T15:48:19Z

src/shared/index.ts

 }

-export function createTitleText(text: string, x: number, y: number, font: string, color: string) {
+export function createTitleText<T extends Any = Any>(text: string, x: number, y: number, font: string, color: string, widget?: T) {


The generic type parameter uses Any which is not a standard TypeScript type. This should be any (lowercase). The same issue exists in the function signature on line 25.

Copilot · 2025-11-11T15:48:20Z

src/etoile/graph/display.ts

 // For performance. we need impl AABB Check for render.

-export abstract class Graph extends S {
+export abstract class Graph<T extends Any = Any> extends S {


The generic type parameter uses Any which is not a standard TypeScript type. This should be any (lowercase).

Suggested change

export abstract class Graph<T extends Any = Any> extends S {

export abstract class Graph<T = any> extends S {

Copilot · 2025-11-11T15:48:20Z

src/etoile/graph/text.ts


  clone() {
-    return new Text({ ...this.style, ...this.__options__ })
+    return new Text({ ...this.style, ...this.__options__, __id__: this.id }, this.__widget__)


The clone method now preserves the original ID by passing __id__: this.id. This means cloned objects will have the same ID as their originals, which could break assumptions about ID uniqueness. If the culling system relies on unique IDs to track individual elements, having duplicate IDs after cloning could cause rendering issues or incorrect culling behavior.

If IDs should be unique, remove __id__: this.id from the clone options. If clones should intentionally share IDs with their source (for the culling system to treat them as the same logical entity), this should be clearly documented.

Suggested change

return new Text({ ...this.style, ...this.__options__, __id__: this.id }, this.__widget__)

return new Text({ ...this.style, ...this.__options__ }, this.__widget__)

Copilot · 2025-11-11T15:48:20Z

src/component.ts

 export function getTextLayout(c: CanvasRenderingContext2D, text: string, width: number, height: number): TextLayoutResult {
-  const textWidth = c.measureText(text).width
  const metrics = c.measureText(text)
+  const textWidth = metrics.width


[nitpick] The variable assignment order has been swapped for no apparent reason. The original code declared textWidth first, then metrics. The new code declares metrics first, then textWidth. While this still works, the change seems unnecessary and makes the diff less clear. Consider keeping the original order unless there's a specific reason for the change.

Suggested change

const metrics = c.measureText(text)

const textWidth = metrics.width

const textWidth = c.measureText(text).width

const metrics = c.measureText(text)

Copilot · 2025-11-11T15:48:22Z

src/etoile/graph/rect.ts

 export type RoundRectOptions = RectOptions & { style: Partial<RoundRectStyleOptions> }

-export class RoundRect extends Graph {
+export class RoundRect<T extends Any = Any> extends Graph {


The generic type parameter uses Any which is not a standard TypeScript type. This should be any (lowercase).

Suggested change

export class RoundRect<T extends Any = Any> extends Graph {

export class RoundRect<T = any> extends Graph {

Copilot · 2025-11-11T15:48:23Z

src/etoile/schedule/index.ts

+    let visibleSet = undefined
+    if (this.quadTree) {
+      const visible = this.quadTree.query(viewport)
+      visibleSet = new Set(visible.map((g) => g.id))
+    }


The visibleSet variable is initialized as undefined but the condition if (this.quadTree) will always be true immediately after the quadTree is assigned. This makes the visibleSet = undefined assignment redundant. Consider initializing visibleSet directly with the Set creation or restructuring this logic for clarity.

const visibleSet = this.quadTree ? new Set(this.quadTree.query(viewport).map((g) => g.id)) : undefined

Suggested change

let visibleSet = undefined

if (this.quadTree) {

const visible = this.quadTree.query(viewport)

visibleSet = new Set(visible.map((g) => g.id))

}

const visibleSet = new Set(this.quadTree.query(viewport).map((g) => g.id))

Copilot · 2025-11-11T15:48:23Z

src/etoile/graph/text.ts

 }

-export class Text extends Graph {
+export class Text<T extends Any = Any> extends Graph {


The generic type parameter uses Any which is not a standard TypeScript type. This should be any (lowercase).

Suggested change

export class Text<T extends Any = Any> extends Graph {

export class Text<T = any> extends Graph {

Copilot · 2025-11-11T15:48:23Z

src/etoile/graph/box.ts


  clone() {
-    const box = new Box()
+    const box = new Box(this.id)


The clone methods now pass this.id to preserve the ID when cloning. However, this means the cloned object will have the same ID as the original, which could cause issues if IDs are expected to be unique identifiers in the system.

If IDs should be unique per instance, the clone should get a new ID. If IDs represent logical identity and clones should share the same ID, this is correct but should be clearly documented. Consider whether this behavior aligns with the intended semantics of cloning in your system.

Suggested change

const box = new Box(this.id)

const box = new Box()

src/etoile/graph/rect.ts

nonzzz added 3 commits November 10, 2025 00:24

chore: fix typo

6ad6a6e

feat: add quard tree

06f5742

feat: perf render

c41d27b

nonzzz force-pushed the culling-render branch from 08f8541 to c41d27b Compare November 11, 2025 15:40

nonzzz requested a review from Copilot November 11, 2025 15:43

Copilot started reviewing on behalf of nonzzz November 11, 2025 15:43 View session

Copilot AI mentioned this pull request Nov 11, 2025

feat: implement viewport culling with quadtree for render optimization #23

Closed

3 tasks

Copilot finished reviewing on behalf of nonzzz November 11, 2025 15:45

Copilot AI reviewed Nov 11, 2025

View reviewed changes

nonzzz merged commit 6808056 into master Nov 16, 2025
6 checks passed

-    const p1 = matrix.transformPoint(x, y)
-    const p2 = matrix.transformPoint(x + width, y + height)
-    const bbox = <BBox> {
-      x: Math.min(p1.x, p2.x),
-      y: Math.min(p1.y, p2.y),
-      width: Math.abs(p2.x - p1.x),
-      height: Math.abs(p2.y - p1.y)
+    const corners = [
+      matrix.transformPoint(x, y),
+      matrix.transformPoint(x + width, y),
+      matrix.transformPoint(x, y + height),
+      matrix.transformPoint(x + width, y + height)
+    ]
+    const minX = Math.min(...corners.map(c => c.x))
+    const maxX = Math.max(...corners.map(c => c.x))
+    const minY = Math.min(...corners.map(c => c.y))
+    const maxY = Math.max(...corners.map(c => c.y))
+    const bbox = <BBox> {
+      x: minX,
+      y: minY,
+      width: maxX - minX,
+      height: maxY - minY

	export abstract class Graph<T extends Any = Any> extends S {
	export abstract class Graph<T = any> extends S {

	return new Text({ ...this.style, ...this.__options__, __id__: this.id }, this.__widget__)
	return new Text({ ...this.style, ...this.__options__ }, this.__widget__)

	export class RoundRect<T extends Any = Any> extends Graph {
	export class RoundRect<T = any> extends Graph {

	export class Text<T extends Any = Any> extends Graph {
	export class Text<T = any> extends Graph {

feat: using culling render #22

feat: using culling render #22

Uh oh!

Conversation

nonzzz commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

CheckList

Uh oh!

nonzzz commented Nov 11, 2025

Uh oh!

Copilot AI commented Nov 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nonzzz commented Nov 11, 2025 •

edited

Loading