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

Pass raw event information to brushes #5687

Merged
merged 12 commits into from Jun 6, 2019

Conversation

@arcatdmz
Copy link
Contributor

commented May 14, 2019

I'd like to continue working on adding pressure-sensitive brush implementation to fabric.js.

This simple PR would be the simplest modification toward that direction, adding raw event information as the last parameters of BaseBrush.onMouse{Move|Down|Up} methods. When combined with the { enablePointerEvents: true } option introduced in PR #5589, all brush subclasses can retrieve pressure information from the raw events.

I'm open to discussions with @asturur and others to improve the brush API as noted in #5589 (comment).

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

FYI, failing tests seem to have nothing to do with this PR.

@asturur

This comment has been minimized.

Copy link
Member

commented May 14, 2019

So now i m going to sleep, just seen this now.
Tomorrow i'll look at all our brush code and try to pull out what i do not like.
I would like a brush to be easier to define and create, but i need to make my mind.

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Looking forward to hearing your ideas!

@asturur

This comment has been minimized.

Copy link
Member

commented May 19, 2019

It looks like i won't have the time to think on how i want to change the brushes.
Let's pass the raw event as we pass it in other parts of the lib.
In an object with the key e so that we can keep adding stuff without changing the function.

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Sorry to hear that, but I'm okay with that decision of adding e property to the Brush objects.

Would that be something around this line, for instance?
https://github.com/fabricjs/fabric.js/blob/master/src/mixins/canvas_events.mixin.js#L537

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

e.g., adding this.freeDrawingBrush.e = e right before that line.

@asturur

This comment has been minimized.

Copy link
Member

commented May 20, 2019

no i meant something like:
this.freeDrawingBrush.onMouseDown(pointer, { e: e });

So that the more things we get to pass, we do not have to change much code.

Ideally would be onMouseDown({ e: e, pointer: pointer })

but that would be a breaking change, i m not sure we should do the day after the 3.0 release.

I would also be ok to jump straight to 4.0 if the interface proove better.

@asturur

This comment has been minimized.

Copy link
Member

commented May 20, 2019

ok i have an idea:
onMouseDown(pointer, { e: e, pointer: pointer })
Will allow to deprecate pointer argument, and in next major version remove it and let people use only the object argument with all the infos.

What do you think?

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Given that 1. onMouseDown (and onMouseMove) events always deal with pointer locations and 2. pressure information is optional (only available in tablet PCs when enablePointerEvents is set), passing onMouseDown(pointer, { e: e }) looks good enough to me. This design could tell Brush developers that handling the second parameter is optional.

Another way of thinking is, while the current implementation pass pointer to onMouseDown and onMouseMove and no argument to onMouseUp, it would be more beautiful to unify the API design: always pass an object. Then, this goes along with your idea of onMouseDown(pointer, { e: e, pointer: pointer }) for now and onMouseDown({ e: e, pointer: pointer }) in the next major release (also coming with onMouseUp({ e: e })).

I'm okay with either way.

@asturur

This comment has been minimized.

Copy link
Member

commented May 20, 2019

i would like to have onMouseDown(pointer, { e: e, pointer: pointer }) for all the related drawing events.

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

so it seems like you like this direction with a slight difference of onMouseUp also receiving { e: e, pointer: pointer } -- is my understanding correct?

it would be more beautiful to unify the API design: always pass an object.

onMouseDown(pointer, { e: e, pointer: pointer }) for now and onMouseDown({ e: e, pointer: pointer }) in the next major release

@asturur

This comment has been minimized.

Copy link
Member

commented May 20, 2019

yes! mouseUp now takes nothing, but it will start to receive.

Also pointer will soon get outdated if we support multi pointers, but that is another problem.

Start to draft it, i will pull down the branch and check what information are already cached and what we can add to the info to give more options to devs.

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

All clear! Thank you for your time. Please let me know when there's something I can do.

@asturur

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

@arcatdmz any chance you can move on with this PR? i would be happy to merge it. We need only to add that extra arguments to mouse events for brushes

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@asturur oooh, I finally see what you expect -- let me do that tonight.

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@asturur 70f9f76 should start passing the events as discussed here.

@asturur

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

ok this is good!
I will try to see if i can add a simple test just that the test suite expect those parameters to be valorized.

@asturur asturur merged commit dd6bf0c into fabricjs:master Jun 6, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arcatdmz arcatdmz deleted the arch-inc:raw-events-to-brushes branch Jun 6, 2019
@asturur asturur referenced this pull request Jun 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.