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

feat: Added children parameter to Component constructor #1525

Merged
merged 13 commits into from
Apr 10, 2022

Conversation

st-pasha
Copy link
Contributor

@st-pasha st-pasha commented Apr 9, 2022

Description

This PR allows to pass the list of children as a parameter directly into the Component's constructor. This is an alternative to standard add() or addAll() methods.

Checklist

  • 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

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

Related Issues

Resolves #1522

@erickzanardo
Copy link
Member

I think there a few components missing, SpriteAnimationComponent for example.

Also we probably should do the same on the bridge packages

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docs, and I thought maybe Erick wanted to implement this (and might have already started) since he opened the issue so always good to discuss on the issue who's going to implementing it before doing it.
Other than that it looks good!

@st-pasha
Copy link
Contributor Author

st-pasha commented Apr 9, 2022

I think there are a few components missing, SpriteAnimationComponent for example.

Right, so my thinking here is that we probably don't want to add children to those components which do not have children usually. For example, Effects are components, but it is not common to add other components to effect components.

Since I'm not exactly sure which components usually do or do not have children, I'll make a list and then you let me know which ones should get the parameter.

@spydon
Copy link
Member

spydon commented Apr 9, 2022

Since I'm not exactly sure which components usually do or do not have children, I'll make a list and then you let me know which ones should get the parameter.

I would say at least all Components deriving from PositionComponent.
BodyComponentin flame_forge2d should also have it, but #1506 should be merged first.
There are probably some more too?

@st-pasha
Copy link
Contributor Author

st-pasha commented Apr 9, 2022

  • CircleHitbox
  • CompositeHitbox
  • PolygonHitbox
  • RectangleHitbox
  • ScreenHitbox
  • ButtonComponent
  • HudButtonComponent
  • HudMarginComponent
  • JoystickCompoenent
  • SpriteButtonComponent
  • CustomPainterComponent
  • IsometricTileMapComponent
  • NineTileBoxComponent
  • ParallaxComponent
  • ParticleSystemComponent
  • SpriteAnimationComponent
  • SpriteAnimationGroupComponent
  • SpriteBatchComponent
  • TextBoxComponent
  • TextComponent
  • TimerComponent
  • CircleComponent
  • PolygonComponent
  • RectangleComponent
  • ShapeComponent
  • FlareActorComponent (flame_flare)
  • BodyComponent (flame_forge2d)
  • RiveComponent (flame_rive)
  • SvgComponent (flame_svg)
  • TiledComponent (flame_tiled)

@spydon
Copy link
Member

spydon commented Apr 9, 2022

The hitboxes and the TimerComponent doesn't need the field I'd say.

@erickzanardo
Copy link
Member

Flame flare is kind obsolete because flame rive, so I would say that it can be omitted too

@spydon
Copy link
Member

spydon commented Apr 9, 2022

The CompositeHitbox would be nice to have the children field on btw, maybe with a tighter type than Component to indicate that hitboxes should be added to it.

Copy link
Contributor

@alestiago alestiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left some minor comments. But overall this is quite useful.

I wonder if it would be valuable to also ad tests for each subclass of Component to check if children is correctly set.

Comment on lines +152 to +154
The two approaches can be combined freely: the children specified within the
constructor will be added first, and then any additional child components
after.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we also clarify, if the added children via children are accesible (or not) from an lifecycle method such as onLoad by reading children?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added clarification.

Comment on lines +66 to +67
game.add(component);
await game.ready();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensureAdd?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

game.ready() does the same, without the need to use a special extension.

Copy link
Member

@erickzanardo erickzanardo Apr 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do write one line less though

@spydon
Copy link
Member

spydon commented Apr 10, 2022

After checking again I think that SpriteBatchComponent and ParticleSystemComponent shouldn't have this argument either.
After removing those I think we can merge this. :)

@st-pasha
Copy link
Contributor Author

After checking again I think that SpriteBatchComponent and ParticleSystemComponent shouldn't have this argument either.
After removing those I think we can merge this. :)

Done.

@spydon spydon enabled auto-merge (squash) April 10, 2022 19:39
@spydon spydon merged commit f0b31fc into flame-engine:main Apr 10, 2022
@st-pasha st-pasha deleted the ps/component-children branch April 10, 2022 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve components with a declarative API
4 participants