FlxGroup.kill() and Flxgroup.revive()

[FlixelCommunityBot]

Issue #202 by: dreadknought

I've noticed that FlxGroup.kill() kills all members of the group, as well as the group object itself, but that FlxGroup.revive() only revives the group object itself, and not its members. Perhaps there could be a fix such that FlxGroup.killmembers() and FlxGroup.revivemembers() would only kill and revive its members, whereas FlxGroup.kill() and FlxGroup.revive() would only kill and revive the group object itself.

[FlixelCommunityBot]

Comment by: dreadknought

I forgot to mention this is in Flixel v2.55, so I apologize if this has already been fixed in the development tree.

[Dovyski]

I think the only wrong behavior here is FlxGroup#revive() which revives the group object itself only. In order to ensure a consistent API, I suggest both methods, kill() and revive(), should operate on all members instead of the group object alone.

What do you think?

[IQAndreas]

I suggest both methods, kill() and revive(), should operate on all members instead of the group object alone.

I agree. revive() and kill() should go hand in hand, performing exactly opposite functions. At the moment, FlxGroup doesn't even include an override for the revive() function, so I'm assuming it was just an oversight on Adam's part.

There may be a case when a developer wants to revive a FlxGroup without reviving all the members (maybe just a specific number of them), but as this is a less common use, they would probably do better extending FlxGroup and overriding.

[Dovyski]

I don't know if we tag it as an enhancement or as a bug. Since it is already working and I don't believe it is causing much trouble right now, maybe we could move this issue to the "Future updates" milestone.

[IQAndreas]

I don't know if we tag it as an enhancement or as a bug.

I'd call it a bug since it goes against "expected" behavior.

Maybe we could move this issue to the "Future updates" milestone.

It was such a quick fix, I thought I might as well add it rather than push it forward. It's so minor that it shouldn't break anyone's existing code.

[Dovyski]

It's a bug indeed. I didn't know how much we should change the code to implement it, but as you said, it's a quick fix. Nice you fixed it for the current release :)

[Dovyski]

Merged bdaeb9f.

[Dovyski]

This fix caused a regression with Mode (non-disapearing particles bug). Previously FlxGroup was calling FlxBasic#revive() that only changes alive and exists of the group itself, which was the cause of the report.

However after FlxGroup#revive() was implemented, it revives all members of the group, which is not a good thing for classes extending FlxGroup, such as FlxEmitter. When the emitter revives itself now, all its particles (even the inactive ones) will revive together.

I see two solutions for this problem:

• Keep the FlxGroup#revive(), changing FlxEmitter to revive differently. We should also check if other classes will be impacted by this as well, so they can be changed accordingly.
• Revert the new FlxGroup#revive().

Considering that several games probably use FlxGroup the same way FlxEmitter does, I think the most prudent action is to revert the changes.

[IQAndreas]

Considering that several games probably use FlxGroup the same way FlxEmitter does

We could add an optional paramter FlxGroup#revive(reviveChildren:Boolean = false). That way it stays reverse-compatible.

However, I would prefer it if reviveChildren defaulted to true, and we explicitly set it to false from inside of FlxEmitter. But in that case, how common do you think it is that other games use that sort of recycling? They would need to be aware of this new parameter in order to continue functioning.

[IQAndreas]

Argh, I just realized that FlxGroup#revive() is an overridden method, so we would need all FlxBasic objects to include that parameter. What a can of worms.

Other alternatives:

• Add a reviveAll() to FlxGroup - in that case, why not a killAll() instead of assuming everything inside the group should die with it? One important question that has a bearing on this, can a FlxBasic object be inside of more than one group? In that case, if the group is killed, you don't want to kill all items in the group too, since they might be alive elsewhere.
• Add an autoReviveChildren property (preferably protected) which defaults to false when using any recycling on objects.
• Add a reviveSelf() which only calls super.revive(), keeping the current behavior of the overridden revive().
• Manually set alive = true and exists = true from in FlxEmitter

This would all be much easier if there was a FlxRecyclingGroup which inherits from FlxGroup, but adds all the recycling logic, and allows you to limit the number of children. This class by default wouldn't revive children as well.

[IQAndreas]

Also, I did a quick check, and the only class that actually uses the revive function is FlxEmitter (it doesn't check if the FlxGroup is dead first, it forces a revive anyway.)

IQAndreas: There may be a case when a developer wants to revive a FlxGroup without reviving all the members (maybe just a specific number of them), but as this is a less common use, they would probably do better extending FlxGroup and overriding.

Oh me a year ago; such wise words, yet unaware of how FlxEmitter was already plotting against you.