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 2022/04/11 08:06:24 UTC
[GitHub] [echarts] jiawulin001 opened a new pull request, #16861: fix: markArea display filter correction
jiawulin001 opened a new pull request, #16861:
URL: https://github.com/apache/echarts/pull/16861
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
Correct the markArea filter when both diagonal points are not in coordinate system.
### Fixed issues
- #16853
## Details
### Before: What was the problem?
MarkArea will disappear if both anchor points of it are not in the coordinate system.
<img width="553" alt="before#16853" src="https://user-images.githubusercontent.com/14244944/162692140-ade661a0-caa2-4500-8f19-3b6d86eaef80.png">
### After: How is it fixed in this PR?
MarkArea is correctly displayed.
<img width="455" alt="after#16853" src="https://user-images.githubusercontent.com/14244944/162692215-89ddf604-f313-42d9-858f-fde6e7bb737c.png">
## Misc
<!-- ADD RELATED ISSUE ID WHEN APPLICABLE -->
- [ ] The API has been changed (apache/echarts-doc#xxx).
- [ ] This PR depends on ZRender changes (ecomfe/zrender#xxx).
### Related test cases or examples to use the new APIs
Attached
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] jiawulin001 commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
jiawulin001 commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r850953540
##########
src/component/marker/MarkAreaView.ts:
##########
@@ -123,6 +123,20 @@ function markAreaFilter(coordSys: CoordinateSystem, item: MarkAreaMergedItemOpti
) {
return true;
}
+ /*Another zone filter is applied, which might not be necessary.
+ Directly returning true means displaying markArea component permanently,
Review Comment:
Good point. Comment updated.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] jiawulin001 commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
jiawulin001 commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r849406769
##########
src/coord/cartesian/Cartesian2D.ts:
##########
@@ -105,6 +106,18 @@ class Cartesian2D extends Cartesian<Axis2D> implements CoordinateSystem {
&& this.getAxis('y').containData(data[1]);
}
+ containZone(data1: ScaleDataValue[], data2: ScaleDataValue[]): boolean {
+ const zoneCorner1 = this.dataToPoint(data1);
+ const zoneCorner2 = this.dataToPoint(data2);
+ const area = this.getArea();
+ return rectRectIntersect(
Review Comment:
Thank you for pointing that out! BoundingRect.intersect in use now!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] echarts-bot[bot] commented on pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on PR #16861:
URL: https://github.com/apache/echarts/pull/16861#issuecomment-1099807059
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] pissang commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
pissang commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r850993180
##########
src/component/marker/MarkAreaView.ts:
##########
@@ -123,6 +123,22 @@ function markAreaFilter(coordSys: CoordinateSystem, item: MarkAreaMergedItemOpti
) {
return true;
}
+ //Directly returning true may also do the work,
+ //because markArea will not be shown automatically
+ //when it's not included in coordinate system.
+ //But filtering ahead can avoid keeping rendering markArea
+ //when there are too many of them.
+ return markerHelper.zoneFilter(coordSys, {
Review Comment:
Another suggestion. There are two places creating same object
```ts
{
coord: fromCoord,
x: item.x0,
y: item.y0
}
```
We can assign it to a temporary variable ahead to avoid redundant codes.
```ts
const item1 = { coord, x, y }
const item2 = { coord, x, y }
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] jiawulin001 commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
jiawulin001 commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r850045643
##########
src/coord/cartesian/Cartesian2D.ts:
##########
@@ -105,6 +105,17 @@ class Cartesian2D extends Cartesian<Axis2D> implements CoordinateSystem {
&& this.getAxis('y').containData(data[1]);
}
+ containZone(data1: ScaleDataValue[], data2: ScaleDataValue[]): boolean {
+ const zoneDiag1 = this.dataToPoint(data1);
+ const zoneDiag2 = this.dataToPoint(data2);
+ const area = this.getArea();
+ const zoneX = Math.min(zoneDiag1[0], zoneDiag2[0]);
Review Comment:
Thank you for pointing this out! Zrender does have some handful functions I ignored.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] pissang commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
pissang commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r850080410
##########
src/component/marker/MarkAreaView.ts:
##########
@@ -123,6 +123,20 @@ function markAreaFilter(coordSys: CoordinateSystem, item: MarkAreaMergedItemOpti
) {
return true;
}
+ /*Another zone filter is applied, which might not be necessary.
+ Directly returning true means displaying markArea component permanently,
Review Comment:
Got it. Thanks for the detailed explanation. Perhaps we can tweak the description a bit to make it more clear. For example:
Returning true directly may also do the work. But filtering ahead will avoid creating too much unnecessary elements when there are huge amount of elements.
##########
src/component/marker/MarkAreaView.ts:
##########
@@ -123,6 +123,20 @@ function markAreaFilter(coordSys: CoordinateSystem, item: MarkAreaMergedItemOpti
) {
return true;
}
+ /*Another zone filter is applied, which might not be necessary.
+ Directly returning true means displaying markArea component permanently,
Review Comment:
Got it. Thanks for the detailed explanation. Perhaps we can tweak the description a bit to make it more clear. For example:
> Returning true directly may also do the work. But filtering ahead will avoid creating too much unnecessary elements when there are huge amount of elements.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] jiawulin001 commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
jiawulin001 commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r850039671
##########
src/component/marker/MarkAreaView.ts:
##########
@@ -123,6 +123,20 @@ function markAreaFilter(coordSys: CoordinateSystem, item: MarkAreaMergedItemOpti
) {
return true;
}
+ /*Another zone filter is applied, which might not be necessary.
+ Directly returning true means displaying markArea component permanently,
Review Comment:
For this one, I mean we are using a function `zonefilter` here, but actually returning true can achieve almost the same effect as using this function. The reason is that, when returning true, `markArea` is always shown and will not be clipped however the grid changes, which solves the problem. When the coordinates does not include the `markArea`, it's automatically not shown. So in my opinion a function to check if the coordinates intersect with `markArea` is unnecessary. Of course, keeping the `markArea` rendered may be problematic when there are huge amount of them, but in most cases one or two `markArea`s would hardly affect performance.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] pissang commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
pissang commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r849142993
##########
src/coord/cartesian/Cartesian2D.ts:
##########
@@ -105,6 +106,18 @@ class Cartesian2D extends Cartesian<Axis2D> implements CoordinateSystem {
&& this.getAxis('y').containData(data[1]);
}
+ containZone(data1: ScaleDataValue[], data2: ScaleDataValue[]): boolean {
+ const zoneCorner1 = this.dataToPoint(data1);
+ const zoneCorner2 = this.dataToPoint(data2);
+ const area = this.getArea();
+ return rectRectIntersect(
Review Comment:
We can use the exist `BoundingRect#intersect` method to check if two rect overlaps.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] pissang commented on pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
pissang commented on PR #16861:
URL: https://github.com/apache/echarts/pull/16861#issuecomment-1098731941
Added recorded user interaction for visual regression testing
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] pissang commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
pissang commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r850028935
##########
src/component/marker/MarkAreaView.ts:
##########
@@ -123,6 +123,20 @@ function markAreaFilter(coordSys: CoordinateSystem, item: MarkAreaMergedItemOpti
) {
return true;
}
+ /*Another zone filter is applied, which might not be necessary.
+ Directly returning true means displaying markArea component permanently,
Review Comment:
We prefer using inline comment // here.
Also this comment seems to be outdated after new commits?
##########
src/coord/cartesian/Cartesian2D.ts:
##########
@@ -105,6 +105,17 @@ class Cartesian2D extends Cartesian<Axis2D> implements CoordinateSystem {
&& this.getAxis('y').containData(data[1]);
}
+ containZone(data1: ScaleDataValue[], data2: ScaleDataValue[]): boolean {
+ const zoneDiag1 = this.dataToPoint(data1);
+ const zoneDiag2 = this.dataToPoint(data2);
+ const area = this.getArea();
+ const zoneX = Math.min(zoneDiag1[0], zoneDiag2[0]);
Review Comment:
We can use the `zoneDiag1` and `zoneDiag2` directly when creating `BoundingRect`. It will handle min, max logic in the constructor:
https://github.com/ecomfe/zrender/blob/master/src/core/BoundingRect.ts#L27
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] pissang commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
pissang commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r850080410
##########
src/component/marker/MarkAreaView.ts:
##########
@@ -123,6 +123,20 @@ function markAreaFilter(coordSys: CoordinateSystem, item: MarkAreaMergedItemOpti
) {
return true;
}
+ /*Another zone filter is applied, which might not be necessary.
+ Directly returning true means displaying markArea component permanently,
Review Comment:
Got it. Thanks for the detailed explanation. Perhaps we can tweak the description a bit to make it more clear. For example:
> Directly returning true may also do the work. But filtering ahead will avoid creating too much unnecessary elements when there are huge amount of elements.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] jiawulin001 commented on a diff in pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
jiawulin001 commented on code in PR #16861:
URL: https://github.com/apache/echarts/pull/16861#discussion_r851003501
##########
src/component/marker/MarkAreaView.ts:
##########
@@ -123,6 +123,22 @@ function markAreaFilter(coordSys: CoordinateSystem, item: MarkAreaMergedItemOpti
) {
return true;
}
+ //Directly returning true may also do the work,
+ //because markArea will not be shown automatically
+ //when it's not included in coordinate system.
+ //But filtering ahead can avoid keeping rendering markArea
+ //when there are too many of them.
+ return markerHelper.zoneFilter(coordSys, {
Review Comment:
Right, now it looks neater.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] pissang merged pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
pissang merged PR #16861:
URL: https://github.com/apache/echarts/pull/16861
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
[GitHub] [echarts] echarts-bot[bot] commented on pull request #16861: fix: markArea display filter correction
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on PR #16861:
URL: https://github.com/apache/echarts/pull/16861#issuecomment-1094680585
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/echarts/wiki/How-to-make-a-pull-request).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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