Pass raw event information to brushes

[arcatdmz]

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]

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

[asturur]

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]

Looking forward to hearing your ideas!

[asturur]

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]

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]

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

[asturur]

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]

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]

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]

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

[arcatdmz]

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]

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]

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

[asturur]

@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]

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

[arcatdmz]

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

[asturur]

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.