Flame specific lint rules

[erickzanardo]

We could create Flame specific lint rules, to help the community avoid bad practices that are specific to the Flame Engine.

I propose that we use this discussion to talk about possible lint rules, and once we agree on a rule, we create an issue out of it.

Thanks so much to @incendial for reaching out to me and suggesting this idea.

[erickzanardo]

I will start by proposing a lint rule to check if the user is instantiating their game directly on the GameWidget, inside a build method. Which would cause the game to "reset" everytime Flutter rebuilds the widget where the GameWidget is in.

Bad:

class MyGamePage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return GameWidget(game: MyFlameGame());
  }
}

Good:

class MyGamePage extends StatefulWidget {
  @override
  State<MyGamePage> createState() => _MyGamePageState();
}

class _MyGamePageState extends State<MyGamePage> {

  late final _game = _MyFlameGame();

  @override
  Widget build(BuildContext context) {
    return GameWidget(game: _game);
  }
}

or

class MyGamePage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return GameWidget.controlled(gameFactory: MyFlameGame.new);
  }
}

[incendial]

Here is the implementation dart-code-checker/dart-code-metrics#1163. Any feedback is welcome!

[erickzanardo]

This looks great @incendial !!

[incendial]

Thank you ☺️

[projectitis]

This is how I am currently doing it. Is it bad?
No state[less] widget involved...

class MyGame extends FlameGame with SingleGameInstance { ... }

void main() {
  final game = MyGame( ... );

  runApp(GameWidget(game: game));
}

[spydon]

This is how I am currently doing it. Is it bad?
No state[less] widget involved...

class MyGame extends FlameGame with SingleGameInstance { ... }

void main() {
  final game = MyGame( ... );

  runApp(GameWidget(game: game));
}

No, that's fine.

[wolfenrain]

A lint rule to check if the onLoad needs to be async or not. If the body does not use await then it shouldn't be async so Flame can directly continue in it's loading cycle.

It is just a minor thing but it can optimize code in the long run (especially for the internals of Flame it would help out if we ever make extra decisions based on that)

✅ Good:

class MyComponent extends Component {
  Future<void> onLoad() async {
    await ...
  }
}

class MyComponent extends Component {
  void onLoad() {
    ...
  }
}

❌ Bad:

class MyComponent extends Component {
  Future<void> onLoad() async {
    ...
  }
}

[st-pasha]

There was a discussion about this on dart-lang: https://github.com/dart-lang/linter/issues/1257

[incendial]

@wolfenrain Am I right that this one https://dcm.dev/docs/individuals/rules/common/avoid-redundant-async/ won't work, coz you need to additionally change the return type from Future<void> to void?

[incendial]

So, waiting for an update on this one before implementing.

[spydon]

you need to additionally change the return type from Future to void?

That is correct

[incendial]

Done dart-code-checker/dart-code-metrics#1168 🎉

[aytunch]

I am not sure if this is always a good practice to call super methods in overrides, but if it is then we can add a lint rule.
Currently, if we don't call super.render(canvas); there is no lint warning. And we can cover all the overrides which require super calls.

@override
  void render(Canvas canvas) {
    // super.render(canvas);

[wolfenrain]

Methods that require you to call super.xxx should have a @mustCallSuper which forces that through static analyzing

[incendial]

So, no rule is needed, right?

[spydon]

Agree, no rule needed for this.

[pastordougdev]

We all probably remember this, but I get forgetful sometimes - do not initialize late final variables in the onMount method.

[spydon]

class MyComponent extends Component {
  late final int x;

  @override
  void onMount() {
    x = 1;
  }
}

[incendial]

Thank you!

[incendial]

But initialising any other type of class field is allowed, right?

[spydon]

Indeed, as long as they are not late final since you can remove a component and add it again and then onMount will run again.

[incendial]

Here is the implementation dart-code-checker/dart-code-metrics#1165

[munsterlander]

Do not create Vector2 in update as well.

[incendial]

@spydon for the second example, I can simply ban all binary operations (like +, -, *, /) when both operands are vectors, will this work or there are some correct cases to have them?

[spydon]

That should work!

[incendial]

Great! And another one dart-code-checker/dart-code-metrics#1166 😁

[incendial]

Should Vector3 and Vector4 be linted as well or Flame allows only to work with Vector2?

[spydon]

I think only Vector2 is fine for now, until we go 3D. 😉

[incendial]

All four rules are now available in prerelease version 5.6.0-dev.1 https://pub.dev/packages/dart_code_metrics/versions/5.6.0-dev.1/changelog! 🚀

Any feedback is very welcome! ☺️

[spydon]

Good job! Will try them out when I get the chance!

[munsterlander]

This is really cool. I will add this package today as well!

[incendial]

Thank you, if you need anything else to be linted - just let me know 😉

[bricekams]

[spydon]

@jotterkain please don't post random stuff without content on several different posts. If you want to say +1, just do a thumbs up reaction instead. Thanks.