[GitHub] [incubator-echarts] pissang commented on a change in pull request #13210: Feat: multiple value axis alignment

2020-09-22 Thread GitBox


pissang commented on a change in pull request #13210:
URL: 
https://github.com/apache/incubator-echarts/pull/13210#discussion_r492453250



##
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:
   I thought the main reason two axes don't align is that they don't have 
the same split numbers. They should be aligned if  the split numbers are same.

##
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:
   I thought the main reason two axes don't align is that they don't 
have the same split numbers. They should be aligned if  the split numbers are 
same.
   
   I see, axis extent is different from grid extent only when `min`, `max` is 
fixed?

##
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:
   I thought the main reason two axes don't align is that they don't 
have the same split numbers. They should be aligned if  the split numbers are 
same.
   
   I see, so axis extent is different from grid extent only when `min`, `max` 
is fixed?





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: commits-unsubscr...@echarts.apache.org
For additional commands, e-mail: commits-h...@echarts.apache.org



[GitHub] [incubator-echarts] pissang commented on a change in pull request #13210: Feat: multiple value axis alignment

2020-09-21 Thread GitBox


pissang commented on a change in pull request #13210:
URL: 
https://github.com/apache/incubator-echarts/pull/13210#discussion_r492453250



##
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:
   I thought the main reason two axes don't align is that they don't have 
the same split numbers. They should be aligned if  the split numbers are same.

##
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:
   I thought the main reason two axes don't align is that they don't 
have the same split numbers. They should be aligned if  the split numbers are 
same.
   
   I see, axis extent is different from grid extent only when `min`, `max` is 
fixed?

##
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:
   I thought the main reason two axes don't align is that they don't 
have the same split numbers. They should be aligned if  the split numbers are 
same.
   
   I see, so axis extent is different from grid extent only when `min`, `max` 
is fixed?





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: commits-unsubscr...@echarts.apache.org
For additional commands, e-mail: commits-h...@echarts.apache.org



[GitHub] [incubator-echarts] pissang commented on a change in pull request #13210: Feat: multiple value axis alignment

2020-09-15 Thread GitBox


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: