FlxSpriteGroup: setting origin should cause members to pivot around the same point

[47rooks]

Currently, rotation and scaling on a FlxSpriteGroup produces strange results. The problem stems from the fact that the originTransform function does not set all the member origin fields to point to the same place - to do this the origin of each member must be set relative to its own position. The current originTransform sets all the origins to the same value. This change fixes this computing the correct relative origin for each child. A unit test is added also. I have also tested this with an application which demonstrates that the rotation and scaling work as expected.

[MondayHopscotch]

Definitely been bitten by this before (one of the many reasons I've historically avoided FlxSpriteGroup). I was doing a jam at the time and never got back around to looking into it. Thanks for digging into and fixing this!

[MondayHopscotch]

Does this behave "intuitively" if the entire group is moved? I'm not in a spot to test it myself, but it feels like there still may be weirdness if you set origin -> move group -> scale.

[47rooks]

I modified my test program - a little robot holding a brick - the robot and the brick are separate sprites in the same group. I can scale the group, drive it about, rotate and scale up and down and move again. All looks ok.

[Geokureli]

can I get a little test case that highlights the before and after behavior? I understand there's a unit test, but I would like something more practical

[47rooks]

Sure. Do you mean as part of this change (if so where in the repo would i put that ?) or just a link to a test repo of my own ?

[47rooks]

Here is a simple playstate that demonstrates the behaviour. Compile it with an existing flixel and one with my change and you can see how the rotation and scaling work. Keys - A - rotate counterclockwise, D - rotate clockwise, H - scale by 3.0, J scale to 0.5. If you need something more complex let me know.

package;

import flixel.FlxG;
import flixel.FlxSprite;
import flixel.FlxState;
import flixel.group.FlxSpriteGroup;
import flixel.util.FlxColor;

class PlayStateOriginDemo extends FlxState
{
	var _repairBot:FlxSpriteGroup;

	override public function create()
	{
		super.create();
		FlxG.camera.bgColor = 0xff008080;

		// Repair Bot
		repairBot();
	}

	function repairBot():Void
	{
		_repairBot = new FlxSpriteGroup(FlxG.width / 2.0, FlxG.height / 2.0);
		_repairBot.screenCenter();
		add(_repairBot);

		var bot = new FlxSprite();
		bot.makeGraphic(100, 100);

		var block1 = new FlxSprite(90, 90);
		block1.makeGraphic(20, 20, FlxColor.RED);

		_repairBot.add(bot);
		_repairBot.add(block1);

		add(_repairBot);

		_repairBot.origin.set(50, 50);
	}

	override public function update(elapsed:Float)
	{
		super.update(elapsed);

		// Repair bot movement
		if (FlxG.keys.pressed.A)
		{
			_repairBot.angle -= 10;
		}
		else if (FlxG.keys.pressed.D)
		{
			_repairBot.angle += 10;
		}
		else if (FlxG.keys.justReleased.H)
		{
			_repairBot.scale.set(3.0, 3.0);
		}
		else if (FlxG.keys.justReleased.J)
		{
			_repairBot.scale.set(0.5, 0.5);
		}
	}
}

[Geokureli]

I'm wondering if this is considered a breaking change, existing projects may be expecting the old behavior, even though it's not desirable. we may need to wait until flixel 6.0.0

[47rooks]

Yeah I wondered the same thing. I wondered if this behavior should be governed by a flag maybe with the default being the existing behavior. Maybe a flag called say localOrigins set to true by default.

[Geokureli]

I would also like to point out, in your example, if block1 moves after the initial setup, it will no longer rotate and scale around the shared origin.

Screen.Recording.2023-12-13.at.12.28.45.PM.mov

I don't think i would want FlxSpriteGroup to account for this, but I do think that rotating groups in flixel is just gonna be wonky

[47rooks]

Yeah any per component update like that would require resetting the origin. Bit like hit box updates I guess. So maybe some doc update is also required

[MondayHopscotch]

I would also like to point out, in your example, if block1 moves after the initial setup, it will no longer rotate and scale around the shared origin.

Screen.Recording.2023-12-13.at.12.28.45.PM.mov
I don't think i would want FlxSpriteGroup to account for this, but I do think that rotating groups in flixel is just gonna be wonky

This definitely drifts into somewhat weird territory. You'd have to know to update the origin after moving individual elements within a FlxSpriteGroup to keep them sync'd.

What is the "intended use" of a FlxSpriteGroup here? Set it up and don't touch the individual components inside the group? (No way to force that, I know)

[Geokureli]

Yeah I wondered the same thing. I wondered if this behavior should be governed by a flag maybe with the default being the existing behavior. Maybe a flag called say localOrigins set to true by default.

I'm trying to think of cases where people would want the old behavior, and I'm struggling to see a valid case. My thought process is: if you're setting the group's origin, you likely want all the objects to pivot around that point, if you wanted them to all have the same origin, they would all need to be the same size and you could just loop through and set them all. A bool would make this no longer a breaking change which is good in the short term, but them we have this potentially useless bool hanging around forever. I think I would rather make the breaking change on a major version when we release a "Flixel 6.0.0 Migration Guide".

Until then, if this is something you need in your project sooner, you can extend it and override the origin setter

So maybe some doc update is also required
yeah

[47rooks]

Ok the only use for the existing behavior that I could think of was as a group of similar objects that you wanted to spin or scale. Maybe a group of pinwheels spinning for example. But they'd have to be the same size and there are other ways you could do it.

Not terribly likely. But I hear you about the useless flag forever.

[47rooks]

Until then, if this is something you need in your project sooner, you can extend it and override the origin setter

Extension doesn't work unless you use haxe 5.0. There is a bug which leads to a type mismatch. At least I couldn't figure out how.

[Geokureli]

Extension doesn't work unless you use haxe 5.0. There is a bug which leads to a type mismatch. At least I couldn't figure out how.

thanks for reminding me i was gonna try this: HaxeFlixel/flixel-ui#261

[47rooks]

thanks for reminding me i was gonna try this: HaxeFlixel/flixel-ui#261

Oh... cool yeah something like that might work.

[Geokureli]

Merging to branch release6 for the upcoming flixel 6.0.0 alpha, thanks!