antvis / antvis/G6

AutoAdaptLabel.isFirstRender is never set to false, so getLabelElementsInView() has been unreachable since #6465

Open
#7,675 0 comments 0 reactions 0 assignees View on GitHub
waiting for maintainer
Dominant language
TypeScript
Stars
12.3k
Forks
1.6k
PR merge metrics
No merged PRs in 30d

Description

Searched open and closed issues, PRs and commits for `isFirstRender` before filing — no existing report found.

## Summary

In `AutoAdaptLabel`, the field `isFirstRender` is initialised to `true` and never assigned again, so the branch that filters label candidates by viewport is unreachable. Before #6465 that filtered path was the only path; the PR added the flag alongside it, so the effect is that viewport filtering has been switched off since then rather than a new option not taking effect.

Observed in `@antv/g6@5.1.1`; the file on the current `v5` default branch is byte-for-byte identical to the published `src` (I diffed them), and the shipped build carries the same code (`lib/behaviors/auto-adapt-label.js:86` and `:99`). Line numbers below refer to `packages/g6/src/behaviors/auto-adapt-label.ts` on `v5`.

## The code

```ts
195: private isFirstRender = true;
209: const labelElementsInView = this.isFirstRender ? this.getLabelElements() : this.getLabelElementsInView();
```

These are the only two occurrences of the field in `packages/g6/src`. Nothing assigns `false`, therefore:

- `getLabelElementsInView()` (`:143`) has exactly one call site — the ternary on `:209` — and is never executed;
- every recompute bound in `bindEvents()` (`:257-264`) — AFTER_RENDER, AFTER_DRAW, AFTER_LAYOUT, AFTER_TRANSFORM — walks the whole `elementMap` through `getLabelElements()` (`:128`), sorts that whole list (`sortLabelElementsInView`, `:169`) and runs the collision pass over it (`detectLabelCollision`, `:111`), regardless of how much of the graph is on screen.

Introduced in 49ba7ff (#6465). The relevant hunk changed an unconditional call into the ternary:

```diff
- const labelElementsInView = this.getLabelElementsInView();
+ const labelElementsInView = this.isFirstRender ? this.getLabelElements() : this.getLabelElementsInView();
```

Two things make it easy to miss: the output is unaffected (`detectLabelCollision` re-checks `viewport.isInViewport(labelBounds, true)` per element at `:118`, so off-screen labels still end up hidden), and only AFTER_TRANSFORM is throttled — the other three channels call through directly.

## A sibling behaviour in the same PR does clear its first-render state

#6465 also added a first-render payload to AFTER_DRAW — `firstRender: this.context.graph.rendered === false` (`packages/g6/src/runtime/element.ts:360`, with `graph.rendered` flipped in `runtime/graph.ts:1212`) — and `fix-element-size.ts`, changed in the same commit, consumes it:

```ts
299: private resetTransform = async (event: IGraphLifeCycleEvent) => {
300: // 首屏渲染时跳过 | Skip when rendering the first screen
301: if (event.data?.firstRender) return;
```

Across `packages/g6/src` that payload has exactly one producer and one consumer. So two behaviours in the same PR express a "first screen" condition two different ways; one reads the event payload, the other uses an instance field that is never cleared. I'm not going to guess which shape was intended here — that's the part I'd like your call on.

## What clearing the flag changes, and what it doesn't

I patched the two private methods on the prototype to count calls (they are TS-`private` but ordinary prototype methods at runtime), and compared stock 5.1.1 against an arm that sets `isFirstRender = false` after the first full scan. 1000 labelled nodes on a 40×25 grid, 900×700 page, `autoFit: 'view'`, sequence `render()` → `zoomTo(k)` → `updateNodeData` + `draw()`, with pauses to drain the throttle window. Headless Chromium 149, two runs, identical numbers.

Stock:

- `getLabelElementsInView` called **0** times across all 8 recomputes;
- candidate set is **1000 every time**, at both the fit zoom (0.338) and at zoom 4;
- `isFirstRender` still `true` at the end.

With the flag cleared after the first scan:

- candidate set becomes **12 at zoom 4** and **2 at zoom 8** — i.e. proportional to what is on screen, not a fixed small number;
- the set of visible labels is **identical** at all three checkpoints — element-wise identical id sets, not merely equal counts (994 and 995 hidden respectively in both arms).

Two limits on reading anything performance-related into this:

- At a fit camera the two paths necessarily agree, since every key bbox intersects the viewport; the run confirms it (1000 candidates either way). So this only differs once you are zoomed in.
- `getLabelElementsInView()` (`:145`) itself calls `getLabelElements()` and filters, so the full `elementMap` walk happens per recompute either way. What shrinks is the work downstream of it — the centrality sort and the per-element `getShape('label').getRenderBounds()` plus `occupiedBounds` intersection tests — at the cost of one key-shape `getRenderBounds()` per element. **Whether that translates into measurable frame time in a real application is something I have not measured and am not claiming.**

Minimal repro (single HTML file, no build)

```html

const { Graph, AutoAdaptLabel, register } = window.G6;
const N = 1000;
const CLEAR_FLAG = false; // flip to true for the second arm

const scans = [];
const origAll = AutoAdaptLabel.prototype.getLabelElements;
AutoAdaptLabel.prototype.getLabelElements = function (...a) {
const r = origAll.apply(this, a);
scans.push({ path: 'all', size: r.length });
if (CLEAR_FLAG) this.isFirstRender = false;
return r;
};
const origInView = AutoAdaptLabel.prototype.getLabelElementsInView;
AutoAdaptLabel.prototype.getLabelElementsInView = function (...a) {
const r = origInView.apply(this, a);
scans.push({ path: 'inView', size: r.length });
return r;
};

const cols = 40;
const nodes = Array.from({ length: N }, (_, i) => ({
id: `n${i}`,
style: { x: (i % cols) * 60 + 40, y: Math.floor(i / cols) * 60 + 40 },
}));
const graph = new Graph({
container: 'c', data: { nodes }, animation: false, autoFit: 'view',
node: { style: { size: 12, labelText: (d) => d.id } },
behaviors: ['auto-adapt-label'],
});

(async () => {
await graph.render();
await new Promise((r) => setTimeout(r, 300));
await graph.zoomTo(4);
await new Promise((r) => setTimeout(r, 300));
graph.updateNodeData([{ id: 'n500', style: { x: 120, y: 120 } }]);
await graph.draw();
console.log(JSON.stringify({ CLEAR_FLAG, scans,
inViewCalls: scans.filter((s) => s.path === 'inView').length }, null, 2));
})();

```

The static evidence above stands on its own — two occurrences of the field, no assignment — so this script is only here if you want to see the counts move.

## Two things I could not settle, and would rather leave to you

**Are the two paths meant to be equivalent?** They do not use the same predicate. `isInViewport(target, complete = false, tolerance = 0)` (`runtime/viewport.ts:302`) tests intersection when `complete` is false and containment when true. The filter at `:145` passes the **key** shape's bounds with the default (intersection); the visibility decision at `:118` passes the **label** shape's bounds with `complete = true` (containment). So I cannot argue the two are equivalent by construction. I also could not construct a case where they diverge: sweeping a default-placed node across the viewport edge in 2px steps over 201 positions, the precondition ("key bbox does not intersect the viewport while the label bbox is fully inside it") held **0 times** and observed label visibility differed **0 times** between the arms — with the default placement the label sits against the key, so once the key leaves the edge the label is no longer fully inside either. A positive label offset might open that window; I have not tested it.

**The existing tests may need a look either way.** `__tests__/unit/behaviors/auto-adapt-label.spec.ts` is four snapshot cases with no assertion pinning first-render behaviour, and `zoom-3` goes through `graph.zoomTo` — the path this affects. #6465 also rewrote `snapshots/.../zoom-3.svg` and `padding-60.svg` (many labels flipping `visible` → `hidden`), though that commit touched `runtime/element.ts`, `utils/transform.ts` and `elements/shapes/base-shape.ts` as well, so I can't attribute the snapshot change to this hunk specifically. I mention it only so that "just clear the flag" doesn't look free: whichever way you resolve the semantics, those snapshots are the place it will show up.

Possibly related, though with no root-cause analysis in it: #6658 reports rendering fuzziness with this behaviour after `addData`. I have not verified any connection.

Happy to send a PR once you've decided which of the two shapes is the intended one — reading the payload like `fix-element-size` does, or clearing the field — since that choice also decides what the snapshots should say.

---

Unrelated nit while in this file, say the word and I'll open it separately: the JSDoc at `:63` says `@defaultValue 32` for `throttle`, while `:80` and both documentation pages say `100`, so the JSDoc looks like the odd one out.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with packages/g6/src/behaviors/auto-adapt-label.ts, especially isFirstRender, getLabelElementsInView(), bindEvents(), and the related viewport checks. Review __tests__/unit/behaviors/auto-adapt-label.spec.ts and the zoom-3.svg and padding-60.svg snapshots, then resolve which first-render behavior is intended and update the implementation and tests so the chosen semantics are covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
data-visualization
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.