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

[idea] Convert HasTappableComponents/HasDraggableComponents into classes #1733

Closed
st-pasha opened this issue Jun 15, 2022 · 14 comments
Closed

Comments

@st-pasha
Copy link
Contributor

What could be improved

Currently the HasTappableComponents and HasDraggableComponents are mixins on the Game class, but we could have them as regular objects added to the class, i.e:

class Game {
  List<Detectors> _interfaces = [
    HasTappableComponents(),
    HasDraggableComponents(),
  ];
}

Then in the GameWidget, instead of checking if (game is MultiTouchTapDetector), we'd have instead if (game.hasInterface(MultiTouchTapDetector)).

Why should this be improved

This would provide the following benefits:

  • The user doesn't have to add the mixins to the Game class - instead the TapCallbacks mixin can add the interface that it needs at runtime;
  • We can extend class HasTappableComponents, but we cannot extend a mixin. This would make extending the events functionality much easier.

Any risks?

We could do this change only to the new HasTappableComponents/HasDraggableComponents mixins which are still in the experimental stage, so that wouldn't be that much breaking.

@spydon
Copy link
Member

spydon commented Jun 15, 2022

Sounds very good! One less step for the user to worry about.

@erickzanardo
Copy link
Member

I am failing to see the practical difference, won't this be the same thing but instead of adding a mixin to the class, users would add an instance?

@spydon
Copy link
Member

spydon commented Jun 15, 2022

I am failing to see the practical difference, won't this be the same thing but instead of adding a mixin to the class, users would add an instance?

Now they have to add two mixins, with this change only the component mixin will be necessary and that mixin will add the interface class automatically to the game when that component is added to the game.

@erickzanardo
Copy link
Member

Alright, but will this work in runtime? Like if a component is added after the game has been mounted/attached?

@st-pasha
Copy link
Contributor Author

Yes, it would work just as with overlays: you add an interface, the game rebuilds, and now you have support for drags/taps/whatever.

An interesting corollary of this would be the possibility of building an example where you can turn on/off various detectors (ScaleDetector, PanDetector, etc) and see how they work together. Though this would probably not be useful for any real-world game.

@erickzanardo
Copy link
Member

Interesting! I am all for that, I definitely think this could improve. I would just like to do some POCs before. What I am concerned about this is with the following scenario:

No tapables components > adds one tapable component > screen flicks because of rebuilt.

I know that this scenario can already happen with overlays, but components are added and removed way more often than overlays, so this this could be worse on this context.

@st-pasha
Copy link
Contributor Author

No tapables components > adds one tapable component > screen flicks because of rebuilt.

I know that this scenario can already happen with overlays, but components are added and removed way more often than overlays, so this this could be worse on this context.

Hmm, that's a good point. My understanding of rendering in Flutter was like this (and perhaps @renancaraujo can fact check me on this one): Flutter renders the entire rendering object tree and saves it in the buffer; on the next frame, it rebuilds those objects that are marked as dirty (and Flame is always dirty), composes the screen buffer out of all the available pieces, and replaces the old screen buffer with the new one. Thus, the only way for a screen flick to occur is if there was at least one frame where we requested to draw a black screen.

Now, I imagine this could potentially happen if you mount a new component that adds many other components but does not wait for them to load -- there may occur a frame when the component has only the black background and no children, which would look like a flicker. I presume it could be something like that with overlays too, if the overlay needs to load/add something, but it doesn't await for the loading to end.

These considerations, though, wouldn't affect the proposed interfaces mechanism, because when a new interface is added there are no visual components changing - it's just an invisible layer inside the stack of widgets inside the GameWidget.

So, I don't think there would be any flicker there -- but of course we ought to have an experiment to see how this would work in practice.

@erickzanardo
Copy link
Member

erickzanardo commented Jun 15, 2022

So, I don't think there would be any flicker there

Me neither!

but of course we ought to have an experiment to see how this would work in practice.

Exactly, I just want to for us to be sure before we move to a full blown implementation

@st-pasha
Copy link
Contributor Author

st-pasha commented Jun 15, 2022

I wonder if there is an opportunity here for new possibilities like this:

  • the game normally uses taps for movement;
  • the user opens the inventory screen, and it temporarily enables drag gestures;
  • the user opens world map, and it enables scale/pan gestures.

Currently this wouldn't be possible because scale gestures interfere with the drag gestures (and to a lesser extent with the tap gestures as well).

@erickzanardo
Copy link
Member

Yes! this is a great improvement of that system IMO. Because it will improve a lot possible conflicts of inputs.

@alestiago
Copy link
Contributor

@st-pasha what is the status of this? Have you started working on a solution?

@st-pasha
Copy link
Contributor Author

@alestiago I haven't, this could probably wait until after the next release (which is in just a few days).

@spydon spydon mentioned this issue Nov 21, 2022
20 tasks
spydon pushed a commit that referenced this issue Feb 5, 2023
This PR is first in a series of refactors that aim to simplify event handling in Flame. The approach is as follows:

    Added class GestureDetectorBuilder, which encapsulates the logic of applyGestureDetectors() in a class. This class resides within the Game and initiates widget rebuild whenever any new gesture detectors are added or removed. Note: 

    [idea] Convert HasTappableComponents/HasDraggableComponents into classes #1733 suggests having a list of interfaces inside the Game class -- this is essentially that list, encapsulated in a class.
    Added the MultiDragDispatcher component, which contains the logic that used to be within the HasDraggableComponents mixin. This component is internal; it mounts to a FlameGame directly, and ensures that it is a singleton.
    Whenever any DragCallbacks component is added to a game, it automatically adds the MultiDragDispatcher component (unless there is already one), which in turn registers a drag gesture detector with GestureDetectorBuilder and rebuilds the game widget.

The end result is that now in order to make a component draggable you only need to add the DragCallbacks mixin to that component, everything else will be handled by the framework.

Consequently, the HasDraggableComponents mixin is now empty and marked as deprecated.
@spydon
Copy link
Member

spydon commented Mar 2, 2023

@st-pasha we should do the same with the Tap events until we release v1.7.0 right?

@spydon
Copy link
Member

spydon commented Sep 11, 2023

I think we can close this since the callback mixins work without the game mixins now.

@spydon spydon closed this as completed Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants