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.

[Dovyski]

It's a complex matter after all :)

I'm a fan of keeping the API as simple as possible, so I believe kill() and revive() should work on the group itself, while killAll() and reviveAll() work on the group members.

group.kill() and group.revive() allow an easy way to show/hide a huge amount of sprites (members of the group). If a sprite belongs to more than a group, for instance, group.kill() will not interfere with that. If the developer wants to explicitly kill/revive all group members, group.killAll() and group.reviveAll() will do the trick.

FlxEmitter#start() calls revive() because it wants the group itself to be visible again, not its particles. Using the kill/revive/killAll/reviveAll approach, there is no need to change that and it will work as expected.

I agree with your "year old" comment, but I think it's more useful to have kill/revive acting on the group itself than on itself and its members.

[IQAndreas]

I'm a fan of keeping the API as simple as possible, so I believe kill() and revive() should work on the group itself, while killAll() and reviveAll() work on the group members.

Perfect!

Have you already started on the code for this, or shall I throw up a quick pull request?

EDIT: After a while I assumed you hadn't started on it and went ahead and sent the pull request: #185

[Dovyski]

Sorry for the late reply, luckily you created the pull without me.

I've been thinking about this bug and the changes we are planning. I think it will make the API simpler, since kill()/revive() will work on the group itself instead of a mixed behavior of "itself/members".

As a consequence I think it can drastically affect developers, since FlxGroup#kill() will stop killing its members (it smells code break). I strongly suggest we push this change up to the next release to ensure maximum backwards compatibility.

[Dovyski]

I've pushed this issue up to the next release.