Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BBox 结构讨论 #229

Closed
hustcc opened this issue Oct 24, 2019 · 20 comments
Closed

BBox 结构讨论 #229

hustcc opened this issue Oct 24, 2019 · 20 comments

Comments

@hustcc
Copy link
Member

hustcc commented Oct 24, 2019

背景

https://github.com/antvis/component/pull/76/files/cefc95b9d90064389ef69034e977740d8a6002d4#diff-2f40c9c995828b4407782848dae7e9aa

一些代码中,存在概念不统一,比如上面这个 review 中,getBBox 返回的不是 G 定义的 BBox,而是自己定义的一个 object,所以需要统一成一个结构。

目前 4.0 中 bbox 代码:https://github.com/antvis/g/blob/4.x/packages/g-base/src/bbox.ts

主要讨论点

  1. 使用 class 还是 object?

  2. 属性包含哪些?

  readonly x: number;
  readonly y: number;
  readonly height: number;
  readonly width: number;

  readonly minX: number;
  readonly maxX: number;
  readonly minY: number;
  readonly maxY: number;

  readonly tl: Point;
  readonly tr: Point;
  readonly bl: Point;
  readonly br: Point;
  1. 是否要具备一些方法?
equal
clone
merge
@antvis antvis deleted a comment from antv-bot Oct 24, 2019
@hustcc
Copy link
Member Author

hustcc commented Oct 24, 2019

@dxq613 @dengfuping @simaQ @paleface001 @zqLu @hustcc 都发表看法,无论对错。有新的觉得更好的方案,也可以提出来~

@simaQ
Copy link
Contributor

simaQ commented Oct 24, 2019

当初加入 BBox 类的初衷是什么?

@simaQ
Copy link
Contributor

simaQ commented Oct 24, 2019

BBox 类引入的初衷可以追溯这个 issue: http://gitlab.alipay-inc.com/eva-engine/eva-engine/issues/14

诸岳 @dengfuping 可以先坐下性能测试,看看这个造成的性能损耗如何。

另外你现在的 merge 方法是有问题的,假定我想要获取某个 shape 的 bbox,拿到这个 bbox 进行 merge 操作以获取同另一个 shape bbox merge 后的结果,这个时候这个 shape 的 bbox 就被更改了,但是按照预期这个 bbox 不应该发生变化。

@dxq613
Copy link
Member

dxq613 commented Oct 24, 2019

本来很轻量简单的对象折腾的这么复杂,综合要考虑两个因素:

  • 高频应用的属性和操作有哪些
  • G 底层的性能问题,在 G 层面要对功能进行克制

@hustcc
Copy link
Member Author

hustcc commented Oct 24, 2019

初衷我不知道,去年加入到现在都是一直都有的。

我的观点是必须 class,且属性,方法丰富完整。因为对于 canvas 画布,所有的图形都是绝对定位(没有 css 布局),BBox 代表着图形的位置大小,无论是 g 自己,还是上层基于 g 的任何应用,永远都逃不掉的数据结构(对应的 dom 中的 https://developer.mozilla.org/en-US/docs/Web/API/DOMRect)。

  1. 为什么需要比 DOMRect 更丰富的属性和方法?

因为 html 有 css 布局,G 没有。

  1. 为什么不把 class 里面方法放到 util 中去通用?

因为职责清晰,面向对象不就是归纳对象的属性和方法。class 就 class,要 fp 就 fp;互相掺杂,自己用的不爽,上层也用的不爽。

  1. 为什么不把 BBox 全部都放到 util 中?

那为什么不把 Event、Group、Shape 都抽出去?

  1. BBox 内容太丰富让 G 太重?

100 行的类,能重多少?G 底层模块不仅仅是提供能力,还需要规范上层开发。如果是私有仓库,自己使用,无所谓;但是开源了,作为其他的依赖,就要考虑这些。

  1. BBox 不属于 G 的职责?上层可以做的就让上层做

Animate 也可以上层做,ctx draw 上层都可以做;还得看 G 的职责究竟是什么?

@paleface001
Copy link

class的方法与Three.js的思路是一致的,好处是可以挂载各种方法,不方便的地方类似于飞飞刚才举得例子,mergeBBox()返回一个新值就必需先新建一个bbox。

@dxq613
Copy link
Member

dxq613 commented Oct 24, 2019

如果测试着没有性能问题,可以使用 class,但是属性一定要克制, fromRect 的静态方法,参考

image

@hustcc
Copy link
Member Author

hustcc commented Oct 24, 2019

如果测试着没有性能问题,可以使用 class,但是属性一定要克制, fromRect 的静态方法,参考

image

语法的性能问题不用多考虑,及时 object 高,对整体绘制渲染来讲不值一提。

至于为什么要有比 DOMRect 更丰富的属性和方法?见上面的 QA

@hustcc
Copy link
Member Author

hustcc commented Oct 24, 2019

性能测试

  • 代码
const Benchmark = require('benchmark');
const suite = new Benchmark.Suite;

class BBox {
  constructor(x, y, width, height) {
    this.x = x;
    this.y = y;
    this.width = width;
    this.height = height;
  }
}

// add tests
suite
  .add('class', function() {
    const bbox = new BBox(0, 0, 100, 100);
  })
  .add('object', function() {
    const bbox = {
      x: 0,
      y: 0,
      width: 100,
      height: 100
    };
  })
  // add listeners
  .on('cycle', function(event) {
    console.log(String(event.target));
  })
  .on('complete', function() {
    console.log('Fastest is ' + this.filter('fastest').map('name'));
  })
  // run async
  .run({ 'async': true });
  • 结果
v11.10.1

class x 757,636,783 ops/sec ±0.40% (89 runs sampled)
object x 784,186,929 ops/sec ±0.41% (91 runs sampled)
Fastest is object

v6.8.0
class x 92,006,540 ops/sec ±1.77% (81 runs sampled)
object x 100,521,703 ops/sec ±1.73% (84 runs sampled)
Fastest is object

及时考虑语法性能,这个性能能带来什么差别影响。

这个就像考虑包大小一样,虽然 G 分包之后,G 的包大小是小了,但是上层封装相同的 canvas、svg 能力确需要引入更多的包大小。

@simaQ
Copy link
Contributor

simaQ commented Oct 24, 2019

初衷我不知道,去年加入到现在都是一直都有的。

我的观点是必须 class,且属性,方法丰富完整。因为对于 canvas 画布,所有的图形都是绝对定位(没有 css 布局),BBox 代表着图形的位置大小,无论是 g 自己,还是上层基于 g 的任何应用,永远都逃不掉的数据结构(对应的 dom 中的 https://developer.mozilla.org/en-US/docs/Web/API/DOMRect)。

  1. 为什么需要比 DOMRect 更丰富的属性和方法?

因为 html 有 css 布局,G 没有。

  1. 为什么不把 class 里面方法放到 util 中去通用?

因为职责清晰,面向对象不就是归纳对象的属性和方法。class 就 class,要 fp 就 fp;互相掺杂,自己用的不爽,上层也用的不爽。

  1. 为什么不把 BBox 全部都放到 util 中?

那为什么不把 Event、Group、Shape 都抽出去?

  1. BBox 内容太丰富让 G 太重?

100 行的类,能重多少?G 底层模块不仅仅是提供能力,还需要规范上层开发。如果是私有仓库,自己使用,无所谓;但是开源了,作为其他的依赖,就要考虑这些。

  1. BBox 不属于 G 的职责?上层可以做的就让上层做

Animate 也可以上层做,ctx draw 上层都可以做;还得看 G 的职责究竟是什么?

2,3,4,5 点的理由不充分,Group Shape 和 BBox 不是同一个层级的问题... BBox 类的封装从出发点来说是为了能够统一结构,便于用户使用,你不能用这样的理由来质疑 G 的架构设计

@dengfuping
Copy link
Member

  • 根据 4.x4.x-without-bbox-class 两个分支进行测试: 10000 个图形,渲染 + 一次包围盒计算的耗时:

    • bbox 对象: 50 ~ 60 ms
    • bbox 类: 60 ~ 70 ms
  • 基本上每个图形计算一次包围盒的耗时在 10 ms/10000 = 1us 左右。在简单场景下性能没有太大影响,但在数据量大以及图形属性变化频繁的场景下,这个性能差异会被放大,在上述测试场景下会有 10% 左右的差距。

  • 测试代码:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <meta http-equiv="X-UA-Compatible" content="ie=edge" />
    <meta name="geometry" content="interval" />
    <title>G</title>
  </head>
  <body>
    <div id="c1"></div>
    <script src="../packages/g-canvas/dist/g.min.js"></script>
    <script>
      const width = document.body.clientWidth;
      const height = 600;
      const radius = 4;
      const canvas = new G.Canvas({
        container: 'c1',
        width: width,
        height: height,
      });
      const group = canvas.addGroup();
      const start = performance.now();
      for (let i = 0; i < 100; i++) {
        for (let j = 0; j < 100; j++) {
          const shape = group.addShape('circle', {
            attrs: {
              x: ((i * 100) / 10000) * width,
              y: ((j * 100) / 10000) * height,
              r: radius,
              fill: 'red',
            },
          });
          shape.getBBox();
        }
      }
      const end = performance.now();
      console.log(end - start);
    </script>
  </body>
</html>

@simaQ
Copy link
Contributor

simaQ commented Oct 24, 2019

先不说类还是对象的方案,现在的 merge() 方法将实例本身改变了,是不合适的。

@hustcc
Copy link
Member Author

hustcc commented Oct 24, 2019

@simaQ 上述 QA 都不是我自问自答,我觉得这些问题都是不存在的问题。问为什么不把 BBox 抽到 util 就像问为什么不把 Shape 抽到 util 中是一个意思。

只有第 3 个不够客观,没有正面的去回答问题。在我看来, BBox 和 Group Shape 都是 G 核心概念,不存在彼此彼此。而且深入考虑一下丢到 util 中,真的好吗?上层定义一个组件,还要去 util 中引入一个类型,等等...

@dxq613
Copy link
Member

dxq613 commented Oct 24, 2019

如果抛开性能、大小、灵活性之类,严格的按照规范来做是可以的,但是只要编程语言提供了可能性,我们都要在一些地方做折中,记得以前一个版本的 G 就是因为改了 ucFirst 的实现,去除了几个正则转换,就将性能提升了 20% ,原因就在于有些操作过于高频。
回调 bbox 这个问题上,G 3.x 以及以前的版本中性能问题可能没这么严重,在 G 4.0 版本中因为为了实现局部刷新,在生成的时候要缓存两个 bbox,每次更新都会重新计算,所以我才让诸岳在实际的场景下测试,如果只有 1% 一下的开销我不在意这点, 10% 实在是不能忽视的开销。

@dengfuping
Copy link
Member

dengfuping commented Oct 24, 2019

先不说类还是对象的方案,现在的 merge() 方法将实例本身改变了,是不合适的。

  • 这行代码 的实现有问题,应该根据 this 创建一个新的对象,我先记下。

@simaQ
Copy link
Contributor

simaQ commented Oct 24, 2019

先不说类还是对象的方案,现在的 merge() 方法将实例本身改变了,是不合适的。

[这行代码][https://github.com/antvis/g/blob/edf6d6942a6b5e609fd9673e2b28f20c5158f625/packages/g-base/src/bbox.ts#L62] 的实现有问题,应该根据 this 创建一个新的对象,我先记下。

逍为这么写,是什么场景需要? @hustcc

@dengfuping
Copy link
Member

dengfuping commented Oct 24, 2019

  • @simaQ 是我提的 PR ~,当时没有注意这一点,是个 bug

@dxq613
Copy link
Member

dxq613 commented Oct 24, 2019

这个地方的性能问题,应该是 v8 引擎对 fast object 的优化 和G shape 内部的赋值次数相关。
https://v8.dev/blog/fast-properties
https://www.smashingmagazine.com/2012/11/writing-fast-memory-efficient-javascript/

@dengfuping dengfuping mentioned this issue Oct 24, 2019
13 tasks
@hustcc
Copy link
Member Author

hustcc commented Oct 24, 2019

既然决定了,就关闭了。

@hustcc hustcc closed this as completed Oct 24, 2019
@hustcc
Copy link
Member Author

hustcc commented Oct 24, 2019

另外提醒一点,把所有 class 换成 function 的方式,可以减少 30% 的包大小,性能上不好说,估计
15% 应该是没有问题的。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants