feat(Canvas): provide correctly named canvas pointer methods. BREAKING: rm `_normalizePointer`, `restorePointerVpt` #9175

ShaMan123 · 2023-08-25T08:43:35Z

Motivation

This PR adds 2 new methods to get a point from a mouse event and tries to remove the confusion about pointer and absolutePointer.

After much discussion we ended up renaming:

-scenePoint (before absolutePointer) a point that is the fabricCanvas plane, the one in which you measure object.top and object.left of an object added in the canvas.

-viewportPoint (before pointer) a point in the viewport coordinate, for which the point 0,0 is the top left corner of the canvas container, while canvas.width, canvas.height is the bottom right corner of the canvas container ( because of retina scaling i avoid on purpose of talking about the HTMLCanvasElement )

canvas.getPointer(event, ignoreVpt) becomes canvas.getPointer(event, fromViewport) and becomes deprecated, will be moved to protected or private next major version.
Developers are directed to use:
canvas.getViewportPoint(event) for canvas.getPointer(event, true)
and
canvas.getScenePoint(event) for canvas.getPointer(event, false)

All the events that were emitted with pointer and absolutePointer still are doing so, those event properties are deprecated and the new scenePoint takes the place of absolutePointer and viewportPoint takes the place of pointer.

restorePointerVpt has been removed and also transformPointRelativeToCanvas in favor of sendPointToPlane

github-actions · 2023-08-25T08:45:53Z

Build Stats

file / KB (diff)	bundled	minified
fabric	910.338 (-1.419)	305.076 (-0.369)

not a single test to account for

…into rm-vpt-methods

ShaMan123

OMG that slipped under the radar
@jiayihu could this be the reason for the very strange outputs in tests?

ShaMan123 · 2023-08-25T09:40:10Z

src/canvas/Canvas.ts

+        pointer: this.getHTMLPointFromEvent(e),
+        absolutePointer: this.getCanvasPointFromEvent(e),


an example of the bug

https://github.com/fabricjs/fabric.js/pull/9175/files#r1306300610

ShaMan123 · 2023-08-25T09:42:41Z

src/canvas/SelectableCanvas.ts

-      // http://www.geog.ubc.ca/courses/klink/gis.notes/ncgia/u32.html
-      // http://idav.ucdavis.edu/~okreylos/TAship/Spring2000/PointInPolygon.html


link are DEAD

ShaMan123 · 2023-08-25T09:44:26Z

src/canvas/SelectableCanvas.ts

+        ? sendPointToPlane(
+            pointer,
+            invertTransform(this.viewportTransform),
+            objToCheck.group.calcTransformMatrix()
+          )
        : pointer;


look at this
sending a pointer in the canvas/object plane if under group or sending a pointer in the HTML plane if not
BUG BUG BUG
I remembered when rewriting group I could not grasp containsPoint, now I do. A complete mess.

How the bug manifest? The fact that the code is not your liking doesn't mean is bugged.

It does.
The method accepts points in different cases, under group with viewportTransform removed from the point and if not under group then the point in the html plane.
If that is not a bug in design I do not now what a bug is.
I do not plan to touch this because #8767 fixed that and I am tired of getting back to things I fixed

It is a bug because I can't use containsPoint, no one can without copying this code because it makes no sense

Update SelectableCanvas.ts Update SelectableCanvas.ts

ShaMan123

@asturur this is critical before v6
I imagine we should add tests

src/canvas/Canvas.ts

src/canvas/SelectableCanvas.ts

jiayihu · 2023-08-25T10:39:02Z

I imagine we should add tests

Yes, tests for something so important should be there. This is the base of many interactions an application can build on top of the canvas.

ShaMan123

the sanpshot show clearly we need to expose a method that prepares the event data for consistency

leanup Update eventData.test.ts

ShaMan123

event data is TESTED
This PR can merge

src/util/dom_misc.ts

jest.setup.ts

asturur · 2023-08-25T23:07:52Z

Please add a description showcase of the bug. What the bug was? how did you find it?
containspoint was doing stuff that you assumed was other stuff, and now we scream BUG BUG BUG MESS MESS MESS the code is not understandable.
The truth is that all the changes that have been done in the name of improving something else changed the meaning of those functions, containsPoint was strictly used for checking targets on mouse.

asturur · 2023-08-25T23:11:30Z

src/canvas/SelectableCanvas.ts

+   * the same plane as the plane {@link FabricObject#getCenterPoint} exists in
+   * @return {Point}
+   */
+  protected getPointer(e: TPointerEvent, inHTMLPlane = false): Point {


please restore getPointer as normal method, we are not adding another migration headache to devs.

you mean remove the protected keyword?

I can do that.
As I see it it is a present for the dev.
TS will warn them about it. Then they can read the comment and decide if to change it or not. It is not a must. But it might help devs when debugging a hard bug.

Apart from saving the devs, having it protected saves fabric devs - I care about that a lot.
But it is not a must if you disapprove

we can let it public and deprecated and later it will be protected with an internal name. we can't remove it now, i think i have hundreds of getPointer usage and so have others.

ShaMan123 · 2023-09-23T09:16:11Z

renamed.
final thing I need to understand - I will add a test for it.
Seems the calculation of the scene point is wrong because in getPointer it skips the last part of the retina normalization whereas in _cacheTransformEventData it does not

ShaMan123

renamed, rm transformPointRelativeToCanvas which is related to this mess

Update eventData.test.ts Update Canvas.ts

ShaMan123

seems ready apart for the useless tests

ShaMan123 · 2023-11-03T14:09:04Z

src/canvas/Canvas.ts

@@ -855,7 +855,7 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
            transform.target,
            originalControl
          );
-      pointer = pointer || this.getPointer(e);
+      pointer = pointer || this.getScenePoint(e);


seems redundant since we cache getScenePoint

ShaMan123 · 2023-11-03T14:10:19Z

src/canvas/Canvas.ts

-    // this is an absolute pointer, the naming is wrong
-    const pointer = this.getPointer(e);
+    const pointer = this.getScenePoint(e);
    this.freeDrawingBrush &&
      this.freeDrawingBrush.onMouseDown(pointer, { e, pointer });


we should rename this as well when doing the absolutePointer rename

ShaMan123 · 2023-11-03T14:11:17Z

src/canvas/Canvas.ts

+    this._pointer = this.getViewportPoint(e);
+    this._absolutePointer = sendPointToPlane(


also these should be renamed and maybe put into an object

src/canvas/Canvas.ts

ShaMan123 · 2023-11-03T14:19:44Z

src/canvas/__tests__/eventData.test.ts

this test does not seem very useful

ShaMan123 · 2023-11-03T14:26:45Z

and now I pushed a regression

ShaMan123

test should pass now

ShaMan123 · 2023-11-03T15:52:47Z

src/canvas/SelectableCanvas.ts

+      obj.containsPoint(
+        sendPointToPlane(pointer, undefined, this.viewportTransform),
+        true


keep this in mind for conflict resolution with #9395

asturur · 2023-11-04T00:26:34Z

ok i m merging.

github-actions · 2023-11-04T00:32:45Z

Coverage after merging rm-vpt-methods into master will be

82.92%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	9
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.76%	85.29%	87.50%	89.58%	134–135, 160–161, 32–33, 41, 50, 78, 89
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.31%	91.89%	90%	93.55%	119, 130, 139, 32, 95
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.06%	82.35%	100%	93.81%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	78.99%	76.67%	83.05%	80.04%	1007, 1007, 1007–1009, 1009, 1009, 1016–1017, 1025–1026, 1026, 1026–1027, 1033, 1035, 1063–1065, 1068–1069, 1073–1074, 1197–1199, 1202–1203, 1276, 1393, 1515, 159, 184, 291–292, 295–299, 304, 327–328, 333–338, 358, 358, 358–359, 359, 359–360, 368, 373–374, 374, 374–375, 377, 386, 392–393, 393, 393, 43, 436, 444, 448, 448, 448–449, 451, 47, 533–534, 534, 534–535, 541, 541, 541–543, 563, 565, 565, 565–566, 566, 566, 569, 569, 569–570, 573, 582–583, 585–586, 588, 588–589, 591–592, 604–605, 605, 605–606, 608–613, 619, 626, 663, 663, 663, 665, 667–672, 678, 684, 684, 684–685, 687, 690, 695, 708, 736, 736, 797–798, 798, 798, 798, 798, 798, 801–802, 805, 805–807, 810–811, 887, 899, 906, 906, 906, 919, 952, 973–974, 990–991, 991, 991–993, 996–997, 997, 997, 999
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	94.34%	91.94%	94.34%	96.21%	1096, 1098, 1100–1101, 300, 470–471, 473–474, 474, 474, 523–524, 585–586, 599, 639–641, 673, 678–679, 706–707, 896, 896–897, 900, 920, 920, 969, 977
StaticCanvas.ts	96.78%	93.09%	100%	98.53%	1031, 1041, 1093–1094, 1097, 1132–1133, 1209, 1218, 1218, 1222, 1222, 1269–1270, 187–188, 204, 571, 583–584, 914–915, 915, 915–916
StaticCanvasOptions.ts	100%	100%	100%	100%
TextEditingManager.ts	84.31%	71.43%	91.67%	88%	17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	97.50%	88.89%	100%	100%	33
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	94.44%	93.10%	91.67%	96.77%	183, 249, 354
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	7.25%	0%	0%	13.51%	103, 108, 120, 120, 120, 120, 120, 122–125, 125, 128, 135, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–84, 84, 86, 89–90, 90, 90, 90, 90, 92, 97
rotate.ts	19.57%	12.50%	50%	21.43%	41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
scale.ts	93.57%	92.94%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	131–132, 163–164, 171, 177, 179
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env

ShaMan123 · 2023-11-04T05:27:06Z

src/EventTypeDefs.ts

+  /**
+   * @deprecated
+   * use viewportPoint instead.
+   * Kept for compatibility
+   */
+  pointer: Point;
+  /**
+   * @deprecated
+   * use scenePoint instead.
+   * Kept for compatibility
+   */
  absolutePointer: Point;
+  scenePoint: Point;
+  viewportPoint: Point;


the order is confusing

should have been

pointer, absolutePointer, viewportPoint, scenePoint

OR

pointer, viewportPoint, absolutePointer, scenePoint

It might seem petty or insignificant but IMO it isn't
A dev reading this code will be confused

ShaMan123

You forgot the brushes

ShaMan123 · 2023-11-04T07:06:32Z

src/canvas/Canvas.ts

+const getEventPoints = (canvas: Canvas, e: TPointerEvent) => {
+  const viewportPoint = canvas.getViewportPoint(e);
+  const scenePoint = canvas.getScenePoint(e);
+  return {
+    viewportPoint,
+    scenePoint,
+    pointer: viewportPoint,
+    absolutePointer: scenePoint,
+  };


there is anotherthing I have been thinking of lately
Since we cache the points an event can do

scenePoint.x += 5

And cause a bug all across fabric so we should return a clone
I will PR

jiayihu · 2023-11-09T10:41:40Z

FWIW I think the final decision to use scene/viewport as names adds confusion because other parts of the codebase will use a different terminology. So at least another PR is needed so that the whole codebase uses a consistent naming for the same concept.

ShaMan123 · 2023-11-09T11:52:03Z

#9483 has some jsdocs fixes including what you pointed out
After searching there are 4 usages of canvas plane that can be turned into scene plane but that seems minor

ShaMan123 and others added 2 commits August 25, 2023 14:10

patch(Canvas): rm _normalizePointer, restorePointerVpt

ccf1e5a

update CHANGELOG.md

23d3ec4

ShaMan123 added 2 commits August 25, 2023 14:30

very weird comment

017beb3

fix dramatic bug

7824e2f

not a single test to account for

ShaMan123 changed the title ~~patch(Canvas): rm _normalizePointer, restorePointerVpt~~ fix(Canvas): getPointer bug, rm _normalizePointer, restorePointerVpt Aug 25, 2023

ShaMan123 requested a review from asturur August 25, 2023 09:34

ShaMan123 changed the title ~~fix(Canvas): getPointer bug, rm _normalizePointer, restorePointerVpt~~ fix(Canvas): getPointer usage bugs, rm _normalizePointer, restorePointerVpt Aug 25, 2023

github-actions bot and others added 3 commits August 25, 2023 09:35

update CHANGELOG.md

7de49ff

revert

2636773

Merge branch 'rm-vpt-methods' of https://github.com/fabricjs/fabric.js …

3731da4

…into rm-vpt-methods

ShaMan123 force-pushed the rm-vpt-methods branch from f2766a6 to 065ad0c Compare August 25, 2023 09:55

ShaMan123 commented Aug 25, 2023

View reviewed changes

comment

d201b34

Update SelectableCanvas.ts Update SelectableCanvas.ts

ShaMan123 force-pushed the rm-vpt-methods branch from 065ad0c to d201b34 Compare August 25, 2023 10:01

ShaMan123 commented Aug 25, 2023

View reviewed changes

ShaMan123 added bug v6 labels Aug 25, 2023

jiayihu reviewed Aug 25, 2023

View reviewed changes

src/canvas/Canvas.ts Outdated Show resolved Hide resolved

jiayihu reviewed Aug 25, 2023

View reviewed changes

src/canvas/SelectableCanvas.ts Show resolved Hide resolved

ShaMan123 added 2 commits August 25, 2023 17:19

cleanup

e567b27

tests

ab38a5d

ShaMan123 commented Aug 25, 2023

View reviewed changes

ShaMan123 force-pushed the rm-vpt-methods branch from bc9971b to e384006 Compare August 25, 2023 15:22

fix mouseup test

0b47e20

leanup Update eventData.test.ts

ShaMan123 force-pushed the rm-vpt-methods branch from e384006 to 0b47e20 Compare August 25, 2023 15:24

ShaMan123 commented Aug 25, 2023

View reviewed changes

src/util/dom_misc.ts Outdated Show resolved Hide resolved

jest.setup.ts Outdated Show resolved Hide resolved

asturur reviewed Aug 25, 2023

View reviewed changes

ShaMan123 commented Sep 23, 2023

View reviewed changes

ShaMan123 added 2 commits September 23, 2023 15:10

add a weird test

be60b9b

Merge branch 'master' into rm-vpt-methods

8c6bdce

ShaMan123 requested a review from asturur September 27, 2023 02:08

ShaMan123 added 3 commits September 27, 2023 07:45

jsdoc

b88a731

rename

d2e54be

Merge branch 'master' into rm-vpt-methods

fdb1f0e

ShaMan123 force-pushed the rm-vpt-methods branch from abfe8f3 to a6c96e0 Compare November 3, 2023 14:18

rename 2

6e3a2b5

Update eventData.test.ts Update Canvas.ts

ShaMan123 force-pushed the rm-vpt-methods branch from a6c96e0 to 6e3a2b5 Compare November 3, 2023 14:22

ShaMan123 commented Nov 3, 2023

View reviewed changes

fix - from merge conflict

251a2de

ShaMan123 commented Nov 3, 2023

View reviewed changes

asturur added 2 commits November 4, 2023 00:33

just the rename and grouping

a2ab0dc

this test is not great

e33548b

Update CHANGELOG.md

10d462f

asturur approved these changes Nov 4, 2023

View reviewed changes

asturur merged commit 7d0e39d into master Nov 4, 2023
9 checks passed

asturur deleted the rm-vpt-methods branch November 4, 2023 00:28

ShaMan123 commented Nov 4, 2023

View reviewed changes

ShaMan123 mentioned this pull request Nov 19, 2023

BREAKING(): Brush onMouseDown onMouseMove onMouseUp signature #9501

Closed

ShaMan123 mentioned this pull request Dec 28, 2023

🧐 Breaking changes for fabric 6.0 #8299

Open

		pointer: this.getHTMLPointFromEvent(e),
		absolutePointer: this.getCanvasPointFromEvent(e),

		// http://www.geog.ubc.ca/courses/klink/gis.notes/ncgia/u32.html
		// http://idav.ucdavis.edu/~okreylos/TAship/Spring2000/PointInPolygon.html

		this._pointer = this.getViewportPoint(e);
		this._absolutePointer = sendPointToPlane(

feat(Canvas): provide correctly named canvas pointer methods. BREAKING: rm _normalizePointer, restorePointerVpt #9175

feat(Canvas): provide correctly named canvas pointer methods. BREAKING: rm _normalizePointer, restorePointerVpt #9175

Conversation

ShaMan123 commented Aug 25, 2023 • edited by asturur Loading

Motivation

github-actions bot commented Aug 25, 2023 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Aug 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

jiayihu commented Aug 25, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

asturur commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Aug 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 commented Sep 23, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

ShaMan123 commented Nov 3, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Nov 4, 2023

github-actions bot commented Nov 4, 2023

ShaMan123 Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiayihu commented Nov 9, 2023

ShaMan123 commented Nov 9, 2023

feat(Canvas): provide correctly named canvas pointer methods. BREAKING: rm `_normalizePointer`, `restorePointerVpt` #9175

feat(Canvas): provide correctly named canvas pointer methods. BREAKING: rm `_normalizePointer`, `restorePointerVpt` #9175

ShaMan123 commented Aug 25, 2023 •

edited by asturur

Loading

github-actions bot commented Aug 25, 2023 •

edited

Loading

ShaMan123 Aug 26, 2023 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 Aug 26, 2023 •

edited

Loading

ShaMan123 Nov 3, 2023 •

edited

Loading

ShaMan123 Nov 4, 2023 •

edited

Loading