Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pressure property to fabric.util.getPointer #5587

Closed
wants to merge 4 commits into from

Conversation

arcatdmz
Copy link
Contributor

@arcatdmz arcatdmz commented Mar 22, 2019

I am interested in adding pressure support to fabric.js, and as its first step, this pull request allows custom BaseBrush subclasses to access pressure data.

To do so, this pull request implements the following three changes.

  1. use PointerEvent instead of MouseEvent or TouchEvent whenever possible so that the pressure value can be retrieved from PointerEvent.pressure
  2. as a fallback, retrieve the pressure value from Touch.force
  3. update the return value of the fabric.util.getPointer method to include the pressure value

With these small changes, I hope I can start implementing a custom brush (as well as a custom path) that shows strokes of changing widths according to the pressure values.

@arcatdmz
Copy link
Contributor Author

I've updated some code to address eslint and Node.js-related issues, and the only remaining issue seems to come from the unit tests that expect MouseEvent but PointerEvent.

I'm not sure whether I should modify existing tests, as opposed to adding new tests as recommended in the contributing guide.
I haven't played with QUnit before, which is another blocking issue.

@asturur
Copy link
Member

asturur commented Mar 23, 2019

Hi @arcatdmz , thanks for starting this.
We can do this, but i would like to see it differently.

I would like to see a PR that adds pointer events, like you did, whitout adding pressure yet.
I do not know pointerEvents very good or i would have started long ago.

Also i would like pointerEvents to not be enable immediately by default but with a boolean that is new Canvas(.., { enablePointerEvents: true }). Otherwise switching from mouse to pointer without changing major version of fabric seems bad for other devs.

Then in another PR i would introduce the pressure in a way that makes sense ( personally i do not like to see it in the getPointer function that should be completely geometric )

We also needs tests here and there, at least a couple, to see that events fire correctly

What is the equivalent for mouseUp in pointers?

@arcatdmz
Copy link
Contributor Author

Hi @asturur , thanks for your quick and warm response.
I agree that a separate PR that enables optional PointerEvent handling is reasonable, and will do that.

PointerEvent extends MouseEvent, so what we need to do is basically just adding pointer* event handlers instead of mouse* event handlers.

The equivalent for mouseup is pointerup.
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/pointerup_event

Once I add another PR, I'll close this PR.

@arcatdmz
Copy link
Contributor Author

I'm closing this PR and moving to #5589.

Meanwhile, for those interested in working code capable of getting pressure values, you could use the master branch of arch-inc/fabric.js for now.

@arcatdmz arcatdmz closed this Mar 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants