flame icon indicating copy to clipboard operation
flame copied to clipboard

feat: included InheritedComponent

Open alestiago opened this issue 3 years ago • 7 comments

STATUS: In Development

Description

Changes:

  • Creates InheritedComponent abstract class
  • Adds public dependOnInheritedComponentOfExactType method.

Aim: Aims to include the concept of InheritedWidgets into Flame. Looking for O(1) performance for packages such as flame_bloc.

Integrating this concept into the framework will facilitate resolving common issues such as theming. In addition, mixins such as HasGameRef might be made redundant.

Example:

final component = Component();
  _InheritedComponent(
    children: [
      Component(
         children: [
           component,
         ],
      ),
   ],
;

// Retrieves closest _InheritedComponent in O(1).
component.read<_InheritedComponent>();

Checklist

  • [X] The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • [X] I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • [X] 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

  • [ ] Yes, this is a breaking change.
  • [X] No, this is not a breaking change.

Related Issues

Additional Context

  • https://api.flutter.dev/flutter/widgets/InheritedWidget-class.html
  • https://api.flutter.dev/flutter/widgets/BuildContext/dependOnInheritedWidgetOfExactType.html

alestiago avatar Jul 15 '22 22:07 alestiago

(cc: @renancaraujo @felangel @erickzanardo @wolfenrain )

alestiago avatar Jul 15 '22 22:07 alestiago

I'm still not quite sure how this is different from findParent<T>()?

Perhaps it would make sense to clarify the overall purpose? Currently the stated goal is to "mirror the concept of InheritedWidget from Flutter". However, the primary purpose of an InheritedWidget in Flutter is to allow downstream widgets to mark themselves as "dependent" so that they will be automatically rebuilt when the InheritedWidget's state changes. I don't see how this functionality can be mirrored in Flame, since there's no notion of "rebuilding", neither it is clear what the "state" of an InheritedComponent will be.

st-pasha avatar Jul 16 '22 17:07 st-pasha

@st-pasha the purpose of the above is to guarantee O(1) search with a Flutter like API.

I believe, findParent<T> fails to do so, since it relies on ancestors. Which can be a very expensive operation in very deep trees.

Do you know of another way of retrieving an ancestor component in O(1) already in Flame? Is there a way of doing so by composition?

alestiago avatar Jul 16 '22 22:07 alestiago

https://github.com/flame-engine/flame/pull/1799#pullrequestreview-1041056284

Thanks @erickzanardo for joining.

I agree some renaming needs to occur. I've renamed the method to .read<T> I think it is less verbose and descriptive.

About bundling that map on the component class, I also agree that it isn't optimal, we thrive to keep that component smaller as possible to keep performance good. I wonder if we couldn't move that to the flame game instance though, and the ProviderComponent would be a HasGameRef by default

I agree, I would like to have the Component be untouched by these changes. I'm trying to think of a solution that keeps the Component class untouched but still guarantees O(1) reads.

What do we think of the following example API? (cc: @erickzanardo)

class ProviderComponent<C extends Component> extends Component {
  ProviderComponent({
    super.priority,
    required this.child,
  }) : super(children: [child]);

  C child;

  static T? of<T>(Component component) {
    // Magic.
  }
}

extension ProviderRead on Component {
  T? read<T>() {
    return ProviderComponent.of<T>(this);
  }
}

alestiago avatar Jul 17 '22 07:07 alestiago

Hummm, you need to pass a value to the provider component right? Otherwise it isn't providing anything 😅

Other than that, API looks good to me

erickzanardo avatar Jul 17 '22 12:07 erickzanardo

Thanks @erickzanardo!

Haha, funny enough I edited and copied the wrong sample. I will update it as soon as I get back to the office.

Thanks for the feedback once again.

alestiago avatar Jul 17 '22 20:07 alestiago

I'm going on vacation so this PR will have limited activity from me up to around the 10th of August.

alestiago avatar Jul 20 '22 11:07 alestiago

Closing for now since I do not plan to continue the work on this until the majority of the team is onboard.

Will probably move the discussion to an issue.

alestiago avatar Aug 22 '22 05:08 alestiago