fix!: Remove pointerId from Draggable callbacks

[spydon]

Description

Since pointerId is already handled by the handle* methods we don't have to expose it in the onDrag* methods, this conforms with the standard set in Tappable.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the
relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
• I have read the Contributor Guide and followed the process outlined for submitting PRs.
• I have updated/added tests for ALL new/updated/fixed functionality.
• I have updated/added relevant documentation in docs and added dartdoc comments with ///.
• I have updated/added relevant examples in examples.

Breaking Change

Does your PR require Flame users to manually update their apps to accommodate your change?

• Yes, this is a breaking change. (Indicate it in the Conventional Commit prefix with a !,
  e.g. feat!:, fix!:).
• No, this is not a breaking change.

[st-pasha]

This is a positive change, it makes the API cleaner. However, several things need to be addressed:

1. This is a big breaking change ("big" meaning that it will definitely break the code for whoever uses Draggable components). Shouldn't we go via a more cautious deprecation route here?

2. The documentation is missing (I know it was missing before, but still). It's hard to figure out why the onDrag* methods return booleans, or what's the difference between onDragEnd vs onDragCancel. I think it's a great opportunity to add the missing doc-strings.

3. With pointerId parameter gone, how can a user access this information if they really need it? Say, could it be stored inside the info parameter?

[spydon]

This is a positive change, it makes the API cleaner. However, several things need to be addressed:

1. This is a big breaking change ("big" meaning that it will definitely break the code for whoever uses Draggable components). Shouldn't we go via a more cautious deprecation route here?

2. The documentation is missing (I know it was missing before, but still). It's hard to figure out why the onDrag* methods return booleans, or what's the difference between onDragEnd vs onDragCancel. I think it's a great opportunity to add the missing doc-strings.

3. With pointerId parameter gone, how can a user access this information if they really need it? Say, could it be stored inside the info parameter?

1. We'll release it together with other breaking changes at a later point.

2. Good idea, will add it!

3. They can override the handle* methods to access the pointerId if they really need it.

[st-pasha]

What's the use case for returning true here? Do you want to be able to drag multiple components with a single finger?

[spydon]

Either that, or just know that there is a drag event happening on top, you don't have to react to by moving the component.

[st-pasha]

Not sure I understand your example -- this is an onDragStart event, which means there can't be any other "drag event happening on top". Unless you mean a drag from another finger, but that sounds like an advanced use case that we defer to handleDragStart?

[st-pasha]

Maybe mention that "within boundaries" means as determined by [containsPoint] method, and remove mention of hitboxes (which may be confusing to someone who doesn't know about hitboxes).

[spydon]

Good idea.

[st-pasha]

If this event is uncommon, then I wonder if we could provide some default implementation that defers to onDragEnd?

[spydon]

I think that should be up to the user to decide, it will after all just be one line for them to do that if they want to.

[st-pasha]

Hmm, now that I took another look at Draggable, I think I understand what the problem is: it doesn't really make a Component draggable, it only enables drag events on it. In particular, the default implementation does not move the component in response to the drag event.

Also, I don't see any mechanism for "capturing" the drag event by the component that started it. In particular, how do we know that the component that had captured onDragStart will also be getting all onDragUpdates and the eventual onDragEnd?

[spydon]

Hmm, now that I took another look at Draggable, I think I understand what the problem is: it doesn't really make a Component draggable, it only enables drag events on it. In particular, the default implementation does not move the component in response to the drag event.

Correct, this isn't a problem a problem though, this is just how it is designed since it isn't a mixin on a PositionComponent but on a Component, and also users might want to react differently to such events on top of their components. We could create another mixin that is more "high level" specifically for the PositionComponent that does what you are describing.

Also, I don't see any mechanism for "capturing" the drag event by the component that started it. In particular, how do we know that the component that had captured onDragStart will also be getting all onDragUpdates and the eventual onDragEnd?

Because it has stored the pointer id, check the handleDrag* methods.

[st-pasha]

this isn't a problem a problem though, this is just how it is designed

Fair; but then it underscores the need for clear documentation, since the name of the class doesn't reflect its actual functionality. We could consider renaming it, or extend its functionality to match the current name, or both.

Because it has stored the pointer id, check the handleDrag* methods.

Ah ok, makes sense. But then what's the point in bool return value in onDragUpdate, onDragEnd and onDragCancel, since those can't be handled by any component other than the one that captured onDragStart?

[spydon]

Fair; but then it underscores the need for clear documentation, since the name of the class doesn't reflect its actual functionality. We could consider renaming it, or extend its functionality to match the current name, or both.

It could of course be clearer, but we do go through it here:
https://docs.flame-engine.org/1.0.0/gesture-input.html?highlight=draggable#draggable-components
I have actually never heard anyone that have thought that it would move the component directly by adding the mixin, which is a bit strange since it does have a bit of a confusing name like you say, or it could be because most come from the Flutter world and know how the detectors work.

Ah ok, makes sense. But then what's the point in bool return value in onDragUpdate, onDragEnd and onDragCancel, since those can't be handled by any component other than the one that captured onDragStart?

It can be "captured" by more than one component.

[st-pasha]

It can be "captured" by more than one component.

Seems to me that dragging multiple components with a single gesture is an even more rare use case than multi-finger drag.

[spydon]

Seems to me that dragging multiple components with a single gesture is an even more rare use case than multi-finger drag.

They are not mutally exclusive, you can do both. The id is only removed from the callbacks, not the handle methods. I don't get your point here I think.

[st-pasha]

The point of this PR is to simplify the API of Draggable by removing the pointerId arguments that are very rarely needed (but the users who need them can still access via the handle* methods).

At the same time, boolean return values are also very rarely needed (perhaps even more rarely), and removing them would provide an even better improvement to the API (improvement because the developer doesn't need to worry whether to return true or false or what does it all mean).

[spydon]

At the same time, boolean return values are also very rarely needed (perhaps even more rarely), and removing them would provide an even better improvement to the API (improvement because the developer doesn't need to worry whether to return true or false or what does it all mean)

These booleans are used all the time, or do you mean that the default should be false and that the events should always only affect the top most component? The feature to let an event propagate until you tell it to stop is not something that is unique to Flame.

The boolean is perhaps used more in other events, like in the Tappable, but we want to have the API consistent.

[st-pasha]

The feature to let an event propagate until you tell it to stop is not something that is unique to Flame.

I know, this paradigm comes all the way from HTML. But, in HTML there is no explicit marking of components as tappable / draggable / etc -- all elements respond to all events; and also they don't support 2 levels of APIs "simple" and "advanced" as we do.

So, yes, if a component is Tappable and is visually on top, then it should receive the tap event. If it is Draggable, it should receive the drag event, etc. At least in the "simple API" case. The cases where you'd need to pass the event through are so rare that I can't imagine a single example of a game where that could be useful. Can you?

[spydon]

I know, this paradigm comes all the way from HTML. But, in HTML there is no explicit marking of components as tappable / draggable / etc -- all elements respond to all events; and also they don't support 2 levels of APIs "simple" and "advanced" as we do.

The second, "advanced" level, is mostly for internal use, but can be overridden if there really is a need for it, almost no use-cases need to do that.

So, yes, if a component is Tappable and is visually on top, then it should receive the tap event. If it is Draggable, it should receive the drag event, etc. At least in the "simple API" case. The cases where you'd need to pass the event through are so rare that I can't imagine a single example of a game where that could be useful. Can you?

Yes, I have seen it used in several games. One game for example was falling fruits where you should tap the correct fruits to gain points and these fruits could overlap, and one tap should give all the plus and minus points from all the fruits that were tapped. We can discuss this more in the chat, because this is not related to this PR.

[wolfenrain]

If we are going to add breaking changes for the pointerId, might it be an idea to expose it on the info event classes and create such classes for the cancel events as well?

Or does the dragCancel not give a pointer?

[spydon]

If we are going to add breaking changes for the pointerId, might it be an idea to expose it on the info event classes and create such classes for the cancel events as well?

It is possible, a little bit of a hassle in the gestures file though.

Or does the dragCancel not give a pointer?

Yes, you can get the pointer in dragCancel, but you can't get the other information that is needed to create an event.

Do you want to try to do it? 😄

[fwh1990]

class HasDraggables still contains int pointerId parameter

[spydon]

class HasDraggables still contains int pointerId parameter

It does, and it should.