Skip to content

fix: Fix the issue where map labels do not follow the map - #4592

Open
rubbishmaker wants to merge 1 commit into
VisActor:developfrom
rubbishmaker:fix/map-label-scale@20260529
Open

fix: Fix the issue where map labels do not follow the map#4592
rubbishmaker wants to merge 1 commit into
VisActor:developfrom
rubbishmaker:fix/map-label-scale@20260529

Conversation

@rubbishmaker

Copy link
Copy Markdown

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Refactoring
  • Update dependency
  • Code style optimization
  • Test Case
  • Branch merge
  • Release
  • Site / documentation update
  • Demo update
  • Workflow
  • Other (about what?)

🔗 Related issue link

#4585

🔗 Related PR link

🐞 Bugserver case id

💡 Background and solution

问题在于:

  1. renderInner() 仅触发 label 组件的重渲染,但 label 的 x/y 是缓存在编码器中的,且 projection 此时已经被 zoom() 累加了缩放——但 label 渲染并不会重新跑 dataToPosition ,所以 label 仍处于 旧位置 。
  2. 即使 label 编码会重算,path 是通过 postMatrix 在父 group 上做变换(不在 label 所在的 component 节点链上),label 与 path 处于不同的渲染父级, postMatrix 不会传播到 label graphic 。
    最终结果:path 视觉上被缩放/平移了,label 没有同步任何变换 → 错位。
    参考 ScatterSeries 的成熟方案: 给 label graphic 同步施加和 path 一致的 postMatrix 变换 ,并在 onLayoutEnd 中重置(与 GeoCoordinate.onLayoutEnd 重置 path 的 postMatrix 行为一致)。

📝 Changelog

Language Changelog
🇺🇸 English
🇨🇳 Chinese

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

🚀 Summary

copilot:summary

🔍 Walkthrough

copilot:walkthrough

@xuefei1313

Copy link
Copy Markdown
Contributor

🦞 Aime Bot Review

本次改动的核心思路是将地图 roam 过程中施加在 pathGroup 上的 postMatrix 同步应用到 label graphic,并在 onLayoutEnd 时将 label 的 postMatrix 重置后再触发一次 renderInner(),这样可以让地图缩放/平移期间的 label 与区域 path 保持一致,同时避免重新布局后遗留上一次交互变换。整体方向和问题定位与 PR 描述是一致的。

代码层面看,packages/vchart/src/series/map/map.ts 中把原先仅触发 label.renderInner() 的处理改为直接对 label graphic 做 scale/translate,能够覆盖 label 与 path 不在同一渲染父级、父级 postMatrix 无法自然透传的场景;另外新增的单元测试和浏览器用例分别覆盖了 zoom、pan、连续交互以及 onLayoutEnd 重置行为,测试补充也比较完整。当前我没有看到明显的阻塞性问题。若后续还计划支持更多 map label 变体(例如特殊 label component / 自定义 label renderer),请确认它们同样走的是当前这条 graphic 变换链路。

合并建议:从当前改动和测试覆盖来看,我这边倾向于合并。

@Issues-translate-bot

Copy link
Copy Markdown

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


🦞Aime Bot Review

The core idea of this change is to synchronously apply the postMatrix applied to the pathGroup during the map roam process to the label graphic, and reset the label's postMatrix during onLayoutEnd before triggering renderInner() again. This can keep the label consistent with the area path during map scaling/panning, and avoid leaving behind the last interactive transformation after re-layout. The overall direction and issue positioning are consistent with the PR description.

At the code level, packages/vchart/src/series/map/map.ts changed the processing that originally only triggered label.renderInner() to directly do scale/translate on the label graphic, which can cover the scenario where the label and path are not in the same rendering parent, and the parent postMatrix cannot be naturally transparently transmitted; in addition, the newly added unit tests and browser use cases cover zoom, pan, continuous interaction and onLayoutEnd resets the behavior, and the test supplement is also relatively complete. Currently I see no obvious blocking issues. If there are plans to support more map label variants (such as special label component/custom label renderer) in the future, please confirm that they also follow the current graphic transformation link.

Merger suggestion: Judging from the current changes and test coverage, I am inclined to merge.

@rubbishmaker

Copy link
Copy Markdown
Author

@xuefei1313 could you review this pr?

@xile611 xile611 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rubbishmaker 感谢提交修复。我将当前提交 7452d4b9dfa679af0fb27ab789624c033b54d0af 应用到 develop 5d03e35856d093a2d6b0b4cd5bcf0a4a5dd7f9d8,使用项目声明的 VRender 1.1.8 依赖做了对照验证。目前有两处可复现的回归,建议处理后再合并,具体根因和修改建议已分别留在行内评论中。

  1. [P1] 缩放后更新数据,标签被重复变换。 通过 GeoCoordinate.dispatchZoom(1.5, { x: 250, y: 250 }) 缩放后,只更新区域的数值、保持地理数据不变,再调用 updateDataSync()。地图 path 和屏幕包围盒都保持不变,但标签坐标发生偏移:测试中区域中心的预期横坐标约为 0px,标签实际落在 -125px。普通 map 图表和 common 图表中的 map 系列都可复现。仅在 onLayoutEnd() 中重置矩阵无法覆盖这条数据更新路径。
  2. [P2] 多系列共享标签组件时,同一次交互会重复应用变换。 在同一个 geo region 中放置两个 map 系列,并配置文档化的 labelLayout: 'region'。两者的 label mark 指向同一个标签组件,但两个系列的 handleZoom() 各执行一次 scale()。一次 1.5 倍缩放后,地图矩阵是 1.5 倍,共享标签矩阵却是 2.25 倍,实际文本位置也随之偏离。handlePan() 中同类的共享组件处理也请一起检查。

验证结果

检查 结果
此 PR 原有的 6 项单元测试 全部通过
以下 5 项真实交互测试,在未应用 PR 的 develop 上运行 全部通过
以下测试,在同一 develop 应用 PR 后运行 3 项失败:普通 map 更新数据、common map 更新数据、共享标签缩放

现有 PR 测试直接调用 series.handleZoom() / handlePan(),并主要检查矩阵是否相等;它们没有覆盖投影更新后的标签重新编码,也没有覆盖共享组件。建议补充经过实际 zoom 派发、数据更新和共享标签布局的回归测试,断言最终文本位置,而不只断言内部矩阵。

另外,当前 develop 的 ComponentMark.renderInner() 会经过标签 itemEncoder / textAttribute / getAttributesOfState 重新计算位置。因此 PR 描述中“renderInner 不会重新跑 dataToPosition”的前提需要针对最新主干重新核实。麻烦先在当前 develop 上重新复现原始 case,再确定必要的修复范围,避免引入与现有投影更新重复的变换。

下面附上本次实际运行的复现测试,便于直接验证。保存为 packages/vchart/__tests__/unit/series/map-review.test.ts,在 packages/vchart 目录执行:

./node_modules/.bin/jest --runInBand --watch=false --runTestsByPath __tests__/unit/series/map-review.test.ts
展开完整复现测试(5 项)
import { VChart } from '../../../src/vchart-all';
import { createCanvas, removeDom } from '../../util/dom';

const geojson = {
  type: 'FeatureCollection',
  features: [0, 20].map((x, i) => ({
    type: 'Feature', properties: { name: String(i), center: [x + 5, 5] },
    geometry: { type: 'Polygon', coordinates: [[[x, 0], [x, 10], [x + 10, 10], [x + 10, 0], [x, 0]]] }
  }))
};

describe('review map label actual interaction', () => {
  let canvas: HTMLCanvasElement;
  let chart: VChart;
  beforeAll(() => VChart.registerMap('review-map', geojson as any));
  beforeEach(() => { canvas = createCanvas(); canvas.width = 500; canvas.height = 500; });
  afterEach(() => { chart?.release(); removeDom(canvas); });

  const create = async (shared = false, chartType = 'common') => {
    const series = {
      type: 'map', map: 'review-map', nameField: 'name', valueField: 'value', nameProperty: 'name',
      dataId: 'values', label: { visible: true, overlap: false, offset: 0, style: { fontSize: 12 } }
    };
    let spec: any = {
      type: 'common', width: 500, height: 500, padding: 0, animation: false,
      data: [{ id: 'values', values: [{ name: '0', value: 1 }, { name: '1', value: 2 }] }],
      region: [{ coordinate: 'geo', roam: true }],
      labelLayout: shared ? 'region' : 'series',
      series: shared ? [{ ...series, id: 'first' }, { ...series, id: 'second' }] : [series]
    };
    if (chartType === 'map') {
      spec = { ...spec, ...series, type: 'map' };
      delete spec.series;
    }
    chart = new VChart(spec, { renderCanvas: canvas, animation: false });
    await chart.renderAsync();
    const model: any = chart.getChart();
    return { series: model.getAllSeries(), geo: model.getComponentsByKey('geoCoordinate')[0] };
  };
  const label = (series: any): any => series._labelMark.getComponent().getComponent();
  const texts = (g: any): any[] => {
    if (g.type === 'text') { return [g]; }
    return (g.getChildren?.() ?? []).flatMap((child: any) => texts(child));
  };
  const assertPositions = (series: any, geo: any, count = 2) => {
    const actual = texts(label(series));
    expect(actual).toHaveLength(count);
    const origin = series.getRegion().getLayoutStartPoint();
    actual.forEach(text => {
      const i = Number(text.attribute.text);
      const expected = geo.dataToPosition([20 * i + 5, 5]);
      const m = text.globalTransMatrix;
      expect(m.e).toBeCloseTo(expected.x + origin.x, 4);
      expect(m.f).toBeCloseTo(expected.y + origin.y, 4);
    });
  };

  it('real dispatchZoom keeps label anchors at projected geographic positions', async () => {
    const { series, geo } = await create();
    assertPositions(series[0], geo);
    geo.dispatchZoom(1.5, { x: 250, y: 250 });
    assertPositions(series[0], geo);
  });

  it('resize after zoom keeps label anchors at projected geographic positions', async () => {
    const { series, geo } = await create();
    geo.dispatchZoom(1.5, { x: 250, y: 250 });
    chart.resize(600, 400);
    assertPositions(series[0], geo);
  });

  it.each(['map', 'common'])('%s updateData after zoom keeps label anchors at projected geographic positions', async chartType => {
    const { series, geo } = await create(false, chartType);
    geo.dispatchZoom(1.5, { x: 250, y: 250 });
    const area = series[0].getMarkInName('area').getGraphics()[0];
    const path = area.attribute.path;
    const bounds = area.globalAABBBounds.clone();
    chart.updateDataSync('values', [{ name: '0', value: 2 }, { name: '1', value: 3 }]);
    expect(area.attribute.path).toBe(path);
    expect(area.globalAABBBounds).toEqual(bounds);
    assertPositions(series[0], geo);
  });

  it('shared region labels apply the zoom once', async () => {
    const { series, geo } = await create(true);
    expect(label(series[0])).toBe(label(series[1]));
    geo.dispatchZoom(1.5, { x: 250, y: 250 });
    expect(series[0].getRootMark().getProduct().attribute.postMatrix.a).toBeCloseTo(1.5);
    assertPositions(series[0], geo, 4);
  });
});

麻烦修复上述两处问题、补充回归覆盖并重新运行检查,谢谢!

super.onLayoutEnd();

const labelGraphic = this._labelMark?.getComponent()?.getComponent();
if (labelGraphic?.attribute.postMatrix) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] 标签重编码时需要保持坐标与 postMatrix 一致,不能只在布局结束时重置

复现条件:开启 map label 和 roam,先通过 GeoCoordinate.dispatchZoom(1.5, { x: 250, y: 250 }) 缩放,再调用 updateDataSync() 更新数值,地理区域保持不变。普通 map 和 common 中的 map 系列均可复现:地图 path 及其屏幕包围盒不变,标签横坐标却从应在的约 0px 变成 -125px

原因是数据更新触发标签 renderInner() 后,itemEncoder → textAttribute → getAttributesOfState → dataToPosition 会按已经缩放的投影重新计算标签 x/y;而此 PR 留在标签组件上的 postMatrix 仍然存在,又对这组新坐标施加一次缩放。这次更新没有触发布局,因此这里的 reset 不会执行。

建议先重新核实当前主干的标签重编码契约,保证标签位置和父级变换使用一致的坐标空间;如果保留矩阵方案,需要在标签重新编码的公共路径解决重复变换,覆盖数据更新等入口。请补充“真实 zoom → updateDataSync → 标签最终屏幕位置”的回归测试,而不只检查矩阵值。相同用例在未应用 PR 的 develop 上通过,应用此 PR 后失败。

Comment on lines +268 to +270
const labelGraphic = this._ensureLabelGraphicPostMatrix();
if (labelGraphic) {
labelGraphic.scale(scale, scale, scaleCenter);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] 共享 label 组件不能由每个系列分别累加同一次变换

labelLayout: 'region' 是文档化的配置:同一个 region 中的多个系列会共同布局标签,Label._initTextMarkStyle() 会将这些 label mark 关联到同一个 component。因此这里取得的 labelGraphic 不一定由当前 MapSeries 独占。

实测在同一个 geo region 中放置两个开启标签的 map 系列,再通过 dispatchZoom(1.5, { x: 250, y: 250 }) 缩放,两个系列的 handleZoom() 会先后对同一个标签组件执行 scale()。两个地图 path 都是 1.5 倍,共享标签却累积为 1.5 × 1.5 = 2.25 倍,实际文本位置随即偏离对应区域;相同配置在当前未应用 PR 的 develop 上位置检查通过。

建议让共享标签组件在每次交互中只应用一次变换,明确变换应由哪个组件统一负责,避免以“每个系列一个独立标签组件”为前提。下面 handlePan()translate() 也有同样的共享对象处理方式,请一起检查,并补充 labelLayout: 'region' 下多个地图系列的真实交互测试。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants