You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2020/09/16 04:20:02 UTC
[GitHub] [incubator-echarts] pissang commented on a change in pull request #13210: Feat: multiple value axis alignment
pissang commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r489138320
##########
File path: src/coord/Axis.ts
##########
@@ -106,6 +107,14 @@ class Axis {
);
}
+ setGridExtent(start: number, end: number) {
+ this._gridExtent = [start, end];
+ }
+
+ getGridExtent(): [number, number] {
+ return this._gridExtent.slice() as [number, number];
+ }
+
Review comment:
As described above. The concept `grid` should not be leaked to the very general Axis module.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
AxisBuilder will also be used on polar, parallel. Not only on grid.
Also, I'm still not sure about the difference between the grid extent and axis extent. When will these two extents has different values?
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
+ extent[1] = extent[1] + (maxSplitNumber - splitNumber[index]) * interval;
+ niceScaleExtent(axis.scale, axis.model, extent, maxSplitNumber);
+ }
+ else {
+ (axis.scale as IntervalScale).niceTicks(
+ maxSplitNumber + 1, null, null,
+ ((extent[1] - extent[0]) - ((extent[1] - extent[0]) % maxSplitNumber)) / maxSplitNumber
+ );
Review comment:
`extent[1] - extent[0]` should be rounded here to avoid rounding error.
```js
numberUtil.round(extent[1] - extent[0]);
```
The result interval should also be rounded.
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
Review comment:
`(extent[1] - extent[0]) % interval` is not very robust because of precision issue. For example:
`(0.3 - 0.1) % 0.1` will get `0.09999999999999998`.
Should use a more numeric robust way:
```js
const splitNumber = (extent[1] - extent[0]) / interval;
// Is integer
if (Math.abs(Math.round(splitNumber) - splitNumber)) < 1e-8) {
....
}
```
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
+ extent[1] = extent[1] + (maxSplitNumber - splitNumber[index]) * interval;
Review comment:
Should be `extent[1] += (maxSplitNumber - splitNumber[index]) * interval` here?
Also, I'm wondering if there is a case that all the values are negative. And the extent is [-10, 0]. In this case, developers may prefer `extent[0] -= (maxSplitNumber - splitNumber[index]) * interval`
##########
File path: src/scale/helper.ts
##########
@@ -35,13 +35,14 @@ export function intervalScaleNiceTicks(
extent: [number, number],
splitNumber: number,
minInterval?: number,
- maxInterval?: number
+ maxInterval?: number,
+ interval?: number
): intervalScaleNiceTicksResult {
Review comment:
Perhaps we can change the parameter name to `preferredInterval`. It's more descriptive and we don't need to modify the argument.
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
+ extent[1] = extent[1] + (maxSplitNumber - splitNumber[index]) * interval;
+ niceScaleExtent(axis.scale, axis.model, extent, maxSplitNumber);
+ }
+ else {
+ (axis.scale as IntervalScale).niceTicks(
+ maxSplitNumber + 1, null, null,
+ ((extent[1] - extent[0]) - ((extent[1] - extent[0]) % maxSplitNumber)) / maxSplitNumber
+ );
+ }
+ const finalScaleExtent = axis.scale.getExtent();
+ const finalInterval = (axis.scale as IntervalScale).getInterval();
+ finalSplitNumber.push(Math.ceil((finalScaleExtent[1] - finalScaleExtent[0]) / finalInterval));
+ });
+
+ return finalSplitNumber;
+ }
+
update(ecModel: GlobalModel, api: ExtensionAPI): void {
const axesMap = this._axesMap;
this._updateScale(ecModel, this.model);
- each(axesMap.x, function (xAxis) {
- niceScaleExtent(xAxis.scale, xAxis.model);
- });
- each(axesMap.y, function (yAxis) {
- niceScaleExtent(yAxis.scale, yAxis.model);
- });
+ const niceSplitNumX = this.calAxisNiceSplitNumber(axesMap.x);
+ const niceSplitNumY = this.calAxisNiceSplitNumber(axesMap.y);
+ const finalSplitNumberY = this.resetAxisExtent(axesMap.y, niceSplitNumY, Math.max(...niceSplitNumY));
+ const finalSplitNumberX = this.resetAxisExtent(axesMap.x, niceSplitNumX, Math.max(...niceSplitNumX));
Review comment:
Seems the extent will be modified by force if the other axis is not aligned.
But in some cases, developers may fix the `min`, `max`, or even `interval`. Theses specified values should have higher priority and won't be modified.
##########
File path: src/scale/Interval.ts
##########
@@ -215,7 +215,7 @@ class IntervalScale extends Scale {
/**
* @param splitNumber By default `5`.
*/
- niceTicks(splitNumber?: number, minInterval?: number, maxInterval?: number): void {
+ niceTicks(splitNumber?: number, minInterval?: number, maxInterval?: number, interval?: number): void {
splitNumber = splitNumber || 5;
Review comment:
I will prefer using the name `preferredInterval` here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org