This occurred to me after a discussion on the [new FCS component
PR](https://github.com/flame-engine/flame/pull/2694#discussion_r1312450113).
As per usual, @spydon has opened my eyes to the ultimate truth:
We should rename loads of files, and it shall affect almost no one.
The idea is to (1) add a "Text" prefix to all text-rendering-related
classes and (2) rename the existing `Text*` to `InlineText*` (which is
what they are).
This PR is a bit big, but the changes should hopefully be simple to
review, and can be broken down into:
* Add a proper base class for the node inheritance chain, call it
TextNode (while working on Flame Markdown I realized the value this will
have to me)
* Rename the old TextNode to InlineTextNode
* Rename DocumentNode to DocumentRoot because it is not a node
* Rename Element to TextElement
* Rename the old TextElement to InlineTextElement
* Rename Style to FlameTextStyle (note: we could consider dropping the
Flame here)
* Rename the old FlameTextStyle to InlineTextStyle
* Update the docs accordingly
* Add some more diagrams and explanations to the docs, following the new
nomenclature
* I also updated our "internal" imports to use the text module to make
life so much easier (this could arguably be done in a separate PR, but I
honestly think it's easier to review together, please lmk if you prefer
me to split).
These are all breaking changes but likely won't actually affect most
users (see below).
While this is breaking, it should hopefully not affect most users,
because these are all infrastructure classes that most people aren't
using directly. If you are using the FCS components, or the renderers
`TextPaint` or `SpriteFontRenderer` directly, this should have zero
effect to you.
If you are using the Nodes, Stlyes or Elements directly, or have a
custom TextRenderer, see below.
Migrating should be a simple matter of renaming your type references:
* from TextNode to InlineTextNode
* from TextElement to InlineTextElement
* from Element to TextElement
* from FlameTextStyle to InlineTextStyle
* from Style to FlameTextStyle
Make sure to do it in the appropriate order not to cause any
double-replace issues.
If you are importing via the module `package:flame/text.dart`, which we
highly encourage, you should not have to change any import statements
whatsoever.
This PR add the ability for users to override the max size of
`TiledAtlas` used by `TiledComponent` for packing all the images of all
the tilesets used by a tilemap. The overrides can be used if one is sure
that their target platform can handle huge textures.
It also adds an assert for cases when the `RectangleBinPacker` runs out
of space. This is done to help users find the problem quickly.
The issue with GitHub killing out test pipeline might be due to the running on the latest ubuntu image, this commit tries to use the older one (which I read could be a solution somewhere...)
Allows direct access to the variables in the VariableStorage class in jenny (via YarnProject.variables.variables).
This enables a user to implement a save game feature to save the variables and later restore them.
This will:
kill the TextRenderer inheritance chain
incorporate the functionality of the base TextRenderer in the base TextFormatter
rename TextFormatter to TextRenderer and appropriate references
That is because both essentially do the same job; encompass the style (or "how") information about how to render text, but using two slightly different interfaces. While that could allow for more flexibility, it is a faux choice that needlessly complicates the pipeline. By having a single interface to comply with, we still allow for custom renders while at the same time making all the code downstream simpler to use and understand.
Extend the contributing guidelines with more usable markdown
suggestions.
In particular suggest the `Rewrap` VS-code extension since the existing
tooling cannot fix laborious markdown line length issues.
This is part of my ongoing effort to simplify the text rendering
pipeline.
My ultimate goal is to:
* get rid of renders
* rename formatters to renderers
* make the interface complies to both
All details are specified here:
https://github.com/flame-engine/flame/pull/2663
As a first step to break down that huge PR, this extracts
TextRendererFactory in preparation of getting rid of TextRenderer
entirely.
This is technically a breaking change but should have no effect for
users, unless you they have a custom Renderer *and* are using the
registry (which I honestly doubt). But if you are:
* just import TextRendererFactory and use the createDefault method from
there instead
This is part of my ongoing effort to simplify the text rendering
pipeline.
My ultimate goal is to:
* get rid of renders
* rename formatters to renderers
* make the interface complies to both
All details are specified here:
https://github.com/flame-engine/flame/pull/2663
As a first step to break down that huge PR, this makes a small change to
TextElements to make them more useful
This PR will:
### rename render -> draw
draw becomes the "internal", underlying impl, raw method, that just
draws the element w/ any custom options
### add a new render method that takes in more options
this does not need to be extended by every impl.
this is for end users and accepts parameters like position and anchor to
be more in line with the renderer interface
This is technically a breaking change but should have no effect for
users, unless you are creating your own custom `TextElement`s. In that
case, to migrate:
* rename your `render` method to `draw`
This PR introduces a new HasVisibility mixin on Component. It prevents the renderTree method from progressing if the isVisible property is false. It therefore prevents the component and all it's children from rendering.
The purpose of this mixin is to allow showing and hiding a component without removing it from the tree.
An important note is that the component (and all it's children) will still be on the tree. They will continue to receive update events, and all other lifecycle events. If the user has implemented input such as tap events, or collision detection, or any other interactivity, this will continue to operate without affect (unless it relies on the render step - such as per-pixel collision detection).
This is expected behaviour. If it is not desired, the user needs to account for it (by checking isVisible in their code) or someone needs to create another mixin for HasEnabled or HasActive which would prevent these other actions 😁
I am very happy to make changes, take suggestions, etc!
Fix a handful of lint issues across the codebase, identified using DCM.
Nothing controversial, I expect; slowly getting these out of the way so
we can focus on discussing bigger things.
Currently, the implementation of the `absoluteAngle` getter in
`PositionComponent` only takes into account `PositionComponent`
ancestors. The implementation simply runs through the whole hierarchy
summing up the relative angles.
This gives a wrong result with Forge2D because `BodyComponent`s have
angles too: in the common situation where a `PositionComponent` (e.g. a
`SpriteComponent`) is the child of a `BodyComponent`, the
`BodyComponent`'s rotation is ignored when computing the
`absoluteAngle`.
The fix simply introduces a new minimal interface `HasAngle` that both
`PositionComponent` and `BodyComponent` implement (`PositionComponent`
implements it automatically via `AngleProvider`), and reimplements
`absoluteAngle` so that it considers all ancestors with an angle and not
just the `PositionComponent` ones.
This is a first step towards enabling DCM for Flame. Though I have tested with all rules, and am working on selecting, assessing, and fixing violations, as a first step, we can merge the infrastructural changes to flame_lint to and GitHub actions.
As a proof of concept, I am enabling two rules for which we have no violations.
I will followup with enabling more rules, adding discussions for controversial changes, and fixing non-controversial violations.
Fix a couple lint issues across the codebase, identified by
https://github.com/flame-engine/flame/pull/2667
Nothing controversial, I expect; getting these out of the way so we can
focus on discussing bigger things.
While studying the existing text rendering pipeline and infrastructure, in order to better document it and add some easier user-facing ways to render rich text, I found it to be on a slightly overcomplicated state that seems to me to be an artifact of a migration.
Let me first briefly describe how it works with some diagrams (I am writing docs for everything and will re-use this content but wanted to propose this PR first as it will change how things look like).
We have 3 hierarchies related to rendering:
renderers: children of TextRenderer, these define how to render text (style). do not include text
formatters: children of TextFormatter, this also define how to render text (style), but a bit more higher level than TextRenderer.
elements: these are laid-out, styled and ready-render pieces of text (style + string), created by formatters
Note that the renderers and formatters kinda serve the same purpose, but the renderers are a more low-level API. Here is how it it looks like today:
Not only that but all of our TextRenderer are actually FormatterTextRenderer, which just use a formatter to "comply" with the "renderer" interface. There are no "raw" TextRenderers anymore.
This current structure seems to clearly derive from an unfinished transition into the new, higher level and more flexible "formatter" structure while still supporting the "renderers" while we transition. That transition, though, seems to have completed.
The current state makes things more complicated as our current versions of components (TextComponent and TextBoxComponent), specifically the former, have branching code to support both the raw TextRenderer and the specific FormatterTextRenderer implementation, which breaks the inheritance encapsulation and voids its purpose in the first place.
The purpose of this PR is to simplify this class structure by:
remove the base TextRenderer
rename FormatterTextRenderer to TextRenderer: all renderers are formatter-based now
update all references
simplify the code of the components by only dealing with (former FormatterTextRenderer) TextRenderers
This is what it looks like now:
Note that while this is a breaking change, it should not affect most users as they should be using much more user-facing classes, like the aforementioned components, instead of the backing implementations. In fact even if they use the renderers, they probably use one of the concrete implementations anyway, TextPaint or SpriteFontRenderer. Notwithstanding, I added migration instructions below.
This is a small step towards a world where we completely combine the converts of renderers and formatters, as, in my eyes, they do the same thing (carry the style information w/o the text).
Possible future (but definitely breaking) changes:
remove renderers entirely and use formatters directly
consider renaming formatters to renderers (or not)
rename TextPaint as the name does not make sense in any hierarchical model we use
(what I am really interested in): allow the creation of text_ and text_box_ components directly from renderers + elements
This fixes: https://github.com/flame-engine/flame/issues/2647
Reverts: https://github.com/flame-engine/flame/pull/2602
OBE: https://github.com/flame-engine/flame/pull/2650
To be clear, the memory leak issue is not addressed because it is not a
Flame issue. Flame properly calls the `onRemove` event and if it is
needed to pass that on to children, users should do as documented:
https://docs.flame-engine.org/latest/flame/game.html#lifecycle.
So if you are using `GameWidget.controlled`, add a tappable component
and then remove the game, the lifecycle looks like:
```
flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove
```
If you do not use `GameWidget.controlled`, add a tappable component and
then remove the game, the lifecycle looks like:
```
flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove
```
If you do not use `GameWidget.controlled`, do not add a tappable
component and then remove the game, the lifecycle looks like:
```
flutter: onLoadFuture
flutter: mount
flutter: onMount
flutter: attach
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove
```
Previously, the lifecycle would look like:
```
flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget
flutter: detach // These unnecessary calls have been eliminated
flutter: removeGameStateListener // These unnecessary calls have been eliminated
flutter: onRemove // These unnecessary calls have been eliminated
flutter: attach
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove
```
I have updated the below diagram for those who may need it in the
future:
This adds, imo, the missing piece in the event lifecycle based on all the other lifecycle work I have been doing.
Currently, when dispose is called in game_widget.dart, it calls disposeCurrentGame which has the call for onRemove, at issue though is, disposeCurrentGame is also called by didUpdateWidget if the oldWidget.game != widget.game. It may not be needed, but I feel like because FlameGame is inherently a stateless set of widgets (children of the LeafRenderObjectWidget), the hook for onRemove is muddled with what is NOT actually a dispose event. So my thought is, introduce the onDispose method to game which gets called by making disposeCurrentGame({isDispose = false}) then in the actual overridden dispose method, pass that as true, so we can then call currentGame.onDispose() after the call to currentGame.onRemove().
Update components diagram on the docs to be simpler.
It seems we no longer have any abstract classes to note (other than the
abstract concept of "Effects").
I feel right now the colouring (while pretty) is just "highlighting" the
effects node on the graph for no particular reason.
I think it would be simpler and more immediate to understand if we
remove this distinction on this diagram.
Just a thought, though. Hard to assess what will be clearer to someone
not so familiar with the concepts.
Current version:
New version:
``` mermaid
graph TD
%% Config %%
classDef default fill:#282828,stroke:#F6BE00;
%% Nodes %%
Component(Component)
Misc("
TimerComponent
ParticleComponent
SpriteBatchComponent
")
Effects("Effects<br/>(See the effects section)")
Game(Game)
FlameGame(FlameGame)
PositionComponent(PositionComponent)
Sprites("
SpriteComponent
SpriteGroupComponent
SpriteAnimationComponent
SpriteAnimationGroupComponent
ParallaxComponent
IsoMetricTileMapComponent
")
HudMarginComponent(HudMarginComponent)
HudComponents("
HudButtonComponent
JoystickComponent
")
OtherPositionComponents("
ButtonComponent
CustomPainterComponent
ShapeComponent
SpriteButtonComponent
TextComponent
TextBoxComponent
NineTileBoxComponent
")
%% Flow %%
Component --> Misc
Component --> Effects
Component --> PositionComponent
Component --> FlameGame
Game --> FlameGame
PositionComponent --> Sprites
PositionComponent --> HudMarginComponent
PositionComponent --> OtherPositionComponents
HudMarginComponent --> HudComponents
```
Update PR template so "Migration instructions" is after the breaking
change checkbox.
I believe that was where it was intended to be, but please assess if
this makes sense.
Add a class-level doc comment to `SpriteFontTextFormatter`, analogous to
`TextPainterTextFormatter`.
Also, super-specify the return type of the `format` method to facilitate
understanding (again, this makes it more aligned with its twin
`TextPainterTextFormatter`).
The DialogueView is responsible for handling and (almost always) displaying the dialog to the user. A very common use case requires the DialogueView to be a PositionComponent or other visual component in the game. This would require extending from both PositionComponent and DialogueView.
Problem: The current DialogueView implementation is an abstract class. Dart does not support multiple inheritance. This means that the case above is difficult to implement.
Solution: Changing the DialogueView from an abstract class to a mixin class allows the developer to extend PositionComponent and mix in DialogueView. The class can now be used as a base class OR as a mixin. All existing tests still pass. An additional test has been added that validates the use as a mixin (extending one class and mixing in DialogueView).
Added prefix parameter to TiledComponent.load for allowing tiled maps files in different assets folder structures. It still defaults to assets/tiles. I created this PR because I discovered it when I wanted to add a tiled map in a different folder structure. I also saw this issue #2641. And though lets add it.
Compute isometric tile scale based on the map's tile size rather than the tile image's default size. This fixes an issue where scale was incorrect when those two sizes differed.
I was running this code:
```dart
void attack() async {
print('Attack started');
current = PlayerState.attack;
final attackTicker = animationTicker!;
await attackTicker.completed;
print('Attack ticker completed');
attackTicker.reset();
current = PlayerState.idle;
}
```
and `attackTicker.completed` would get stuck as permanently completed
after the first completion.
The viewport should receive events before the world, otherwise all huds will get the events after the world components, if there are any world components underneath them.
This uses the `ComponentKey` API to register and unregister dispatchers,
so that we don't have to create unnecessary dispatcher components that
are removed directly in `onMount` if there already is a dispatcher or
the same type added.
This also opens up for registering components to the dispatchers so that
we don't have to go through the whole tree to deliver events which it
does currently.
In addressing https://github.com/flame-engine/flame/issues/2403, this PR
removes the game widget `onRemove` call which was technically locked
into an internal method and places the call in `FlameGame` that can then
be triggered by `Game` which can be triggered by `GameRenderBox.detach`.
So why not use `dispose` as was discussed in the issue? I would refer to
the Flutter API docs:
https://api.flutter.dev/flutter/widgets/State/dispose.html
_Called when this object is removed from the tree permanently._
_The framework calls this method when this
[State](https://api.flutter.dev/flutter/widgets/State-class.html) object
will never build again. After the framework calls
[dispose](https://api.flutter.dev/flutter/widgets/State/dispose.html),
the [State](https://api.flutter.dev/flutter/widgets/State-class.html)
object is considered un[mounted]
(https://api.flutter.dev/flutter/widgets/State/mounted.html) and the
mounted property is false. It is an error to call
[setState](https://api.flutter.dev/flutter/widgets/State/setState.html)
at this point. This stage of the lifecycle is terminal: there is no way
to remount a
[State](https://api.flutter.dev/flutter/widgets/State-class.html) object
that has been disposed._
So, the issue comes down to you may be removing a `FlameGame` but not
want it permanently destroyed. Since you need the state maintained, then
by naming the method `dispose()`, users would think it permanently
destroys the game. So, this keeps it with `onRemove` and thus inline
with user expectations imo.
A user simply needs to override `onRemove` and set `FlameGame.disposeAll
= true` then call `super.onRemove()` which did not exist previously for
`FlameGame`; thus, I do not think this is a breaking change.
```dart
@override
void onRemove() {
disposeAll = true;
super.onRemove();
}
```
Currently, due to my lack of knowledge of bloc, the test is not working as the game is never mounted, so I am posting the PR in order to get some assistance with that. I tried to use the exact code that was originally posted in the issue, so we can validate the fix resolves that specific leak. This code currently passes all tests and the behavior when stepping through the code, is as would be expected for the life cycle.
I am hoping @imaNNeo can take a look at the test and provide some pointers.
I have just finished steps 1-4 of the Klondike tutorial and noticed a
couple of minor things that look to be outdated with the current dart /
flame versions.
### Step 3 - `late` keyword isn't required
When following this step the analyser hightlight this rule
[unnecessary_late](https://dart.dev/tools/linter-rules/unnecessary_late)
(introduced in Dart 2.16.0) which states:
> Top-level and static variables with initializers are already evaluated
lazily as if they are marked late.
I updated the notes around why the laziness is important so the reader
can still understand the reasoning.
This change introduces a very simple pool for `CollisionProspect`s so
that those objects don't have to be re-created each tick.
It means that the `CollisionProspect` needs to be mutable though, so the
code becomes a little bit harder to read since sets can't be used
anymore.
CameraComponent.canSee wasn't performing any kind of sanity checks on the given components world or mounted-ness. This PR adds these checks to correctly return false if the component is not mounted or if the optional world is not the same as camera's current target world.
Creates an axis-aligned bounding box (AABB) around the ray to short-circuit checking.
In the general case, we skip ~75% of the checks because the ray originates at some point in the 2D cartesian space and goes towards infinity. The AABB in that case is a quadrant of space, and so only entities intersecting with this quadrant of space need to be checked.
In the case where the raycast is called with a maxDistance, the savings are much higher. A relatively short ray will generate a small AABB around its origin and end point. In a typical game world, there will be many entities that do not intersect with this AABB, and can be skipped.
