AutoAdaptLabel.isFirstRender is never set to false, so getLabelElementsInView() has been unreachable since #6465
- 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