Skip to content

feat: support barpadding for histogram#3048

Closed
Runtus wants to merge 3 commits intoVisActor:developfrom
Runtus:feat-barpadding
Closed

feat: support barpadding for histogram#3048
Runtus wants to merge 3 commits intoVisActor:developfrom
Runtus:feat-barpadding

Conversation

@Runtus
Copy link
Copy Markdown

@Runtus Runtus commented Aug 6, 2024

[中文版模板 / 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

close #2629

🔗 Related PR link

🐞 Bugserver case id

💡 Background and solution

  • change bar begin and end position throw 1/2 barPadding.
  • below image show the barPadding: 12px case.
    image
    image

📝 Changelog

feat: support barpadding for histogram.

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

@Runtus Runtus marked this pull request as draft August 6, 2024 07:43
@Runtus Runtus marked this pull request as ready for review August 6, 2024 07:43
Copy link
Copy Markdown
Contributor

@xile611 xile611 left a comment

Choose a reason for hiding this comment

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

另外,新功能都需要补充文档和示例

/**
* 堆叠柱之间的距离
*/
barPadding?: number;
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.

是不是理解有误,需求上是实现“分类轴”上的padding
堆叠柱是数值轴上的

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

这块儿我注释写的有问题,我改改。

Comment thread packages/vchart/src/series/bar/bar.ts Outdated
? {
x: (datum: Datum) => valueInScaleRange(this._dataToPosX(datum), xScale),
x1: (datum: Datum) => valueInScaleRange(this._dataToPosX1(datum), xScale)
x: (datum: Datum) => valueInScaleRange(this._dataToPosX(datum) + barPadding / 2, xScale),
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.

这里从实现上也是有问题的,因为x 和 x1 不知道哪个对应的坐标值更小,想要实现padding也是让小的加上1/2的padding,大的加上1/2的padding
现在这个实现在做了轴转换后就会出问题了
另外最贴近坐标轴的柱子应该也会出问题

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

您好,对于您的评论,我有些疑问就是:

  1. x1我看官方文档给的是柱的右下角的坐标,那么在未翻转的前提下,它不应该始终比x大吗?、
  2. 轴转换指的是x-y轴翻转吗?还是指的坐标系的转换。如果是前者,上述的if( this.direction === Direction.horizontal) 里面已经有实现了,我测试过没问题。如果我理解错您的意思了,麻烦您再详细告知下。
  3. 最贴近坐标轴的柱子 => 现在的预期是,两侧的柱子需要紧贴图表边缘是吗

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.

试试这个case:

{
  type: 'histogram',
  xField: 'from',
  x2Field: 'to',
  yField: 'profit',
  seriesField: 'type',
  barPadding: 10,
  bar: {
    style: {
      stroke: 'white',
      lineWidth: 1
    }
  },
  axes: [
    {
      orient: 'bottom',
      inverse: true,
      nice: false,
      tick: {
        visible: true
      }
    }
  ],
  title: {
    text: 'Profit',
    textStyle: {
      align: 'center',
      height: 50,
      lineWidth: 3,
      fill: '#333',
      fontSize: 25,
      fontFamily: 'Times New Roman'
    }
  },
  tooltip: {
    visible: true,
    mark: {
      title: {
        key: 'title',
        value: 'profit'
      },
      content: [
        {
          key: datum => datum['from'] + '~' + datum['to'],
          value: datum => datum['profit']
        }
      ]
    }
  },
  data: [
    {
      name: 'data1',
      values: [
        {
          from: 0,
          to: 10,
          profit: 2,
          type: 'A'
        },
        {
          from: 10,
          to: 16,
          profit: 3,
          type: 'B'
        },
        {
          from: 16,
          to: 18,
          profit: 15,
          type: 'C'
        },
        {
          from: 18,
          to: 26,
          profit: 12,
          type: 'D'
        },
        {
          from: 26,
          to: 32,
          profit: 22,
          type: 'E'
        },
        {
          from: 32,
          to: 56,
          profit: 7,
          type: 'F'
        },
        {
          from: 56,
          to: 62,
          profit: 17,
          type: 'G'
        }
      ]
    }
  ]
}

x轴配置了 inverse: true

image

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

好的感谢,我思考下怎么去做适配

@xile611
Copy link
Copy Markdown
Contributor

xile611 commented Aug 6, 2024

补充一个badcase,当图表宽度比较小的时候,较窄的“柱子”会被消除掉

const spec = {
  width: 400,
  type: 'histogram',
  xField: 'from',
  x2Field: 'to',
  yField: 'profit',
  seriesField: 'type',
  barPadding: 10,
  bar: {
    style: {
      stroke: 'white',
      lineWidth: 1
    }
  },
  axes: [
    {
      orient: 'bottom',
      nice: false,
      tick: {
        visible: true
      }
    }
  ],
  title: {
    text: 'Profit',
    textStyle: {
      align: 'center',
      height: 50,
      lineWidth: 3,
      fill: '#333',
      fontSize: 25,
      fontFamily: 'Times New Roman'
    }
  },
  tooltip: {
    visible: true,
    mark: {
      title: {
        key: 'title',
        value: 'profit'
      },
      content: [
        {
          key: datum => datum['from'] + '~' + datum['to'],
          value: datum => datum['profit']
        }
      ]
    }
  },
  data: [
    {
      name: 'data1',
      values: [
        {
          from: 0,
          to: 10,
          profit: 2,
          type: 'A'
        },
        {
          from: 10,
          to: 16,
          profit: 3,
          type: 'B'
        },
        {
          from: 16,
          to: 18,
          profit: 15,
          type: 'C'
        },
        {
          from: 18,
          to: 26,
          profit: 12,
          type: 'D'
        },
        {
          from: 26,
          to: 32,
          profit: 22,
          type: 'E'
        },
        {
          from: 32,
          to: 56,
          profit: 7,
          type: 'F'
        },
        {
          from: 56,
          to: 62,
          profit: 17,
          type: 'G'
        }
      ]
    }
  ]
};

@Runtus
Copy link
Copy Markdown
Author

Runtus commented Aug 6, 2024

补充一个badcase,当图表宽度比较小的时候,较窄的“柱子”会被消除掉

const spec = {
  width: 400,
  type: 'histogram',
  xField: 'from',
  x2Field: 'to',
  yField: 'profit',
  seriesField: 'type',
  barPadding: 10,
  bar: {
    style: {
      stroke: 'white',
      lineWidth: 1
    }
  },
  axes: [
    {
      orient: 'bottom',
      nice: false,
      tick: {
        visible: true
      }
    }
  ],
  title: {
    text: 'Profit',
    textStyle: {
      align: 'center',
      height: 50,
      lineWidth: 3,
      fill: '#333',
      fontSize: 25,
      fontFamily: 'Times New Roman'
    }
  },
  tooltip: {
    visible: true,
    mark: {
      title: {
        key: 'title',
        value: 'profit'
      },
      content: [
        {
          key: datum => datum['from'] + '~' + datum['to'],
          value: datum => datum['profit']
        }
      ]
    }
  },
  data: [
    {
      name: 'data1',
      values: [
        {
          from: 0,
          to: 10,
          profit: 2,
          type: 'A'
        },
        {
          from: 10,
          to: 16,
          profit: 3,
          type: 'B'
        },
        {
          from: 16,
          to: 18,
          profit: 15,
          type: 'C'
        },
        {
          from: 18,
          to: 26,
          profit: 12,
          type: 'D'
        },
        {
          from: 26,
          to: 32,
          profit: 22,
          type: 'E'
        },
        {
          from: 32,
          to: 56,
          profit: 7,
          type: 'F'
        },
        {
          from: 56,
          to: 62,
          profit: 17,
          type: 'G'
        }
      ]
    }
  ]
};

那这种情况怎么处理呢?为每个柱设一个最小宽度吗?

@xile611
Copy link
Copy Markdown
Contributor

xile611 commented Aug 6, 2024

补充一个badcase,当图表宽度比较小的时候,较窄的“柱子”会被消除掉

const spec = {
  width: 400,
  type: 'histogram',
  xField: 'from',
  x2Field: 'to',
  yField: 'profit',
  seriesField: 'type',
  barPadding: 10,
  bar: {
    style: {
      stroke: 'white',
      lineWidth: 1
    }
  },
  axes: [
    {
      orient: 'bottom',
      nice: false,
      tick: {
        visible: true
      }
    }
  ],
  title: {
    text: 'Profit',
    textStyle: {
      align: 'center',
      height: 50,
      lineWidth: 3,
      fill: '#333',
      fontSize: 25,
      fontFamily: 'Times New Roman'
    }
  },
  tooltip: {
    visible: true,
    mark: {
      title: {
        key: 'title',
        value: 'profit'
      },
      content: [
        {
          key: datum => datum['from'] + '~' + datum['to'],
          value: datum => datum['profit']
        }
      ]
    }
  },
  data: [
    {
      name: 'data1',
      values: [
        {
          from: 0,
          to: 10,
          profit: 2,
          type: 'A'
        },
        {
          from: 10,
          to: 16,
          profit: 3,
          type: 'B'
        },
        {
          from: 16,
          to: 18,
          profit: 15,
          type: 'C'
        },
        {
          from: 18,
          to: 26,
          profit: 12,
          type: 'D'
        },
        {
          from: 26,
          to: 32,
          profit: 22,
          type: 'E'
        },
        {
          from: 32,
          to: 56,
          profit: 7,
          type: 'F'
        },
        {
          from: 56,
          to: 62,
          profit: 17,
          type: 'G'
        }
      ]
    }
  ]
};

那这种情况怎么处理呢?为每个柱设一个最小宽度吗?

要效果比较好,需要先把padding减去,然后按照剩下的尺寸来计算x,x1,这样就没这个问题
不过按照vchart这个流程,会非常复杂

另一种思路就是要保留一个最小的宽度,至少让用户看到那里是有数据的

@github-actions github-actions Bot added the docs label Aug 7, 2024
* @description 用于计算不同场景下barPadding对应的柱坐标位移值
*/
const barPaddingCompute = (field: 'x' | 'x1' | 'y' | 'y1', inverse: boolean) => {
let acturlPadding = 0;
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.

这个写法还是挺写死的,
如果出现了 x1 对应的值 < x 的情况,还是会出错
也就是说这种写法假设了x1一定要大于x
要代码写的更严谨一点,可以基于vgrammar 的transform机制实现,

在完成所有的视觉通道映射后,来对 x,x1或者 y\y1 进行调整,这样能覆盖更多的场景

可以参考这个:

https://github.com/VisActor/VGrammar/blob/develop/packages/vgrammar-core/src/transforms/mark/mark-overlap.ts

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

好的谢谢,我研究下

@xile611
Copy link
Copy Markdown
Contributor

xile611 commented Aug 30, 2024

@Runtus 有进展吗?

@Runtus
Copy link
Copy Markdown
Author

Runtus commented Aug 31, 2024

@Runtus 有进展吗?

抱歉,最近很忙,一直没时间处理这边。
您之前说的参考vgrammer的transform机制实现我有些没get到意思,去看了下那边代码有点懵,能否再给一些详细的描述

@xile611
Copy link
Copy Markdown
Contributor

xile611 commented Sep 2, 2024

@Runtus 有进展吗?

抱歉,最近很忙,一直没时间处理这边。 您之前说的参考vgrammer的transform机制实现我有些没get到意思,去看了下那边代码有点懵,能否再给一些详细的描述

文档可能写的不清楚 。。。
https://www.visactor.io/vgrammar/guide/guides/marks/base

简单而言就是vchart是基于vgrammar实现的,在计算图元的视觉通道映射前后,都会支持transform变换
就像前面我举的的例子,mark-overlap.ts 就是在计算完视觉通道后,执行的一种变换

@Runtus
Copy link
Copy Markdown
Author

Runtus commented Sep 10, 2024

@Runtus 有进展吗?

抱歉,最近很忙,一直没时间处理这边。 您之前说的参考vgrammer的transform机制实现我有些没get到意思,去看了下那边代码有点懵,能否再给一些详细的描述

文档可能写的不清楚 。。。 https://www.visactor.io/vgrammar/guide/guides/marks/base

简单而言就是vchart是基于vgrammar实现的,在计算图元的视觉通道映射前后,都会支持transform变换 就像前面我举的的例子,mark-overlap.ts 就是在计算完视觉通道后,执行的一种变换

不好意思,最近学校和实习的任务太忙了,没时间做这一块儿工作了,后续可能还要忙一个多月,耽误太长时间了,我就把这pr给close了。

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] histogram support barPadding

2 participants