Changing the way FlxStates are called

[Cheemsandfriends]

I was looking into the issues tab from flixel when I saw #2541 and I felt like it looked pretty damn cool so I thought, why not try to make a pr about it?

Fixes #2541

[Geokureli]

this is a good start, but there's more to it than just that. The whole point is to call the state constructor after the old state is destroyed so that we don't need the create() method anymore

Also, this feature will probably get merged last

[Cheemsandfriends]

@Geokureli wdym with "doing it locally?" you want me to make like a small fork introducing that or some shit like that?

[Geokureli]

Yeah, typically I make changes to flixel using my forked repos. I set haxelib to use my local git versions via haxelib dev flixel path/to/my/repo/.,

I do this for addons, ui, demos, snippets, templates and tools. when you make a change to flixel, Github Actions test your new version of flixel against the dev branch of all of these additional repos, and in this case it's definitely going to fail, so we test them all locally and make branches for each repo that will merged at the same time, once all our local tests work well together.

NGL, this is a big task, and you'll likely have to learn a lot to get it done, but time is on your side, and I'm feeling pretty patient right now. especially because Austin East is gonna try to rewrite the flixel rendering system for flixel 5.0.0

[Cheemsandfriends]

Hm... so how can I let you know about the changes? I post them here or something like that?

[Geokureli]

you can check out Cheemsandfriends:patch-5 in VSCode via its github ui: just make sure your local repo points to your personal fork, you may have to pull once to load all the new remote branches

once that is set up changes you commit and push will be added to the patch-5 branch and will show up here, triggering a new round of github actions

you can also ditch this branch and PR and make your own locally, publish it and then you'll be able to start a new PR using that branch when you go to https://github.com/Cheemsandfriends/flixel

[Cheemsandfriends]

I've been trying to add this for some days but idk why flixel-addons seems to work weird with my implementation? even tho I changed the variables to ()->FlxState

[Cheemsandfriends]

I've got a question @Geokureli, what is this code supposed to do exactly? I don't quite understand it

[Geokureli]

I've got a question @Geokureli, what is this code supposed to do exactly? FlxGame.hx#L527 I don't quite understand it

For full context, here's the full code:

if (FlxG.vcr.paused)
{
	if (FlxG.vcr.stepRequested)
	{
		FlxG.vcr.stepRequested = false;
	}
	else if (_state == _requestedState)
	{
		// some unrelated debug code
		return;
	}
}

you can pause the game using the debug menu's VCR, if you do, return is called preventing the game from updating or drawing, however if _state == _requestedState is false, than return should be bypassed, allowing the game to update and draw normally. a different requestedState means that FlxG.switchState was called, and at the end of the game's update loop, the actual stateSwitch is done, here.

[Geokureli]

hey @Cheemsandfriends! I'm planning on merging this for 6.0.0, I just want it to be the last thing merged before releasing 6.0.0 because it's such a breaking change, like, it breaks everything!

Also I never mentioned this but I actually made this change in my own branch in June, before you finalized this branch. I also made branches in flixel-addons and all the others, that will coincide. I'll be using this branch over mine, in the end, so you get the contribution stats, but I'll cross-check both our branches to see if either one of us missed something. but when I last looked it all seemed good

[T1mL3arn]

Seen this too late to make a proposal, but anyway - why not use Either ? I believe with this pattern it wouldn't be a breaking change.

[Geokureli]

Seen this too late to make a proposal, but anyway - why not use Either ? I believe with this pattern it wouldn't be a breaking change.

I believe it still would be a breaking change, since Either needs the enum value constructor i.e.: FlxG,.switchState(Left(new PlayState())); or FlxG,.switchState(Right(PlayState.new));

However we could use OneOfTwo, or perhaps OneOfThree<()->FlxState, FlxState, Class<FlxState>>, check the type received and have a runtime deprecation warning.

The problem with this is that we wouldn't gain any of the benefits of this change, though, The whole point of the new system is to delay the state constructor call until after the old state is destroyed. I'd have to think about it to be sure but it might not be possible to have it both ways.

Good suggestion though, I'll get back to you

[Geokureli]

@EliteMasterEric came up with a way to give a deprecation warning on the old type

function main()
{
	output(new FlxState("a")); // Warning : use ()->FlxState)
	output(FlxState.new.bind("b"));
	output(()->new FlxState("c"));
}

function output(state:NextState)
{
	if (state is FlxState)
		trace("received state instance");
	else
		trace("received state maker");
}

abstract NextState(Dynamic) to FlxState to ()->FlxState
{
	@:from
	@:deprecated("use ()->FlxState)")
	static public function fromState(state:FlxState):NextState
	{
		return cast state;
	}
	
	@:from
	static public function fromMaker(func:()->FlxState):NextState
	{
		return cast func;
	}
}

class FlxState
{
	public var foo:String;
	
	public function new (foo:String) this.foo = foo;
}

[EliteMasterEric]

I was originally not a fan of this until I realized it lets you reset with constructor arguments by using bind()

Would this change essentially obsolete the create() function as well?

[Geokureli]

it lets you reset with constructor arguments by using bind()

that's one of the big features, here. State constructor arguments are extremely powerful, but no one uses them

Would this change essentially obsolete the create() function as well?

We would keep create for backwards compatibility, but the main reason for this change is to prevent issues from creating Flixel objects in the state constructor, (since, in current flixel, the next state's constructor is called before the previous state is destroyed) however we will likely always keep the create() function in all future versions because FlxG.state will be always be null in your state's constructor. so if you make objects in your state constructor that reference FlxG.state, or if global utils reference FlxG.state, you'll have a bad time

[EliteMasterEric]

Ah, so new FlxState() is pre-switch and create() is post-switch? Good to know.

Once the OneOfTwo change is implemented (could we make a generic DeprecatedInFavorOf<FlxState, Void->FlxState> instead for later reuse?), what other obstacles are there to merging this change?

[Geokureli]

looks like its doable, https://github.com/HaxeFlixel/flixel/pull/2733 assuming this passes CI. feel free to try out the branch, just know that this isn't a real pr

Edit: it didn't work, this will be a breaking change no matter what, but most people don't override switchTo, so this still might make people's transition smoother

[EliteMasterEric]

Checking back in, the new @:op(a()) might be useful here to make the code more clean, there's definitely a way to avoid a type check here. if (state is FlxState) is expensive on several platforms IIRC.

[Geokureli]

Checking back in, the new @:op(a()) might be useful here to make the code more clean, there's definitely a way to avoid a type check here. if (state is FlxState) is expensive on several platforms IIRC.

Is the "cost" of it actually perceivable in any of it's cases? When it comes to optimizing code its easy to say code A is more efficient than code B, but what that often actually means is "there will be noticeable slowdown if done 1,000 times a frame". Calling is a couple times a frame should have no real perceivable impact when it's a couple times a frame, and it seems easier to read and maintain

[EliteMasterEric]

I guess I'm just brainstorming use cases for newly available language features.

I do think that creating an abstract which convert a class instance to a supplier at compile time is better for code readability, and is easier to phase out later on.

Here's what my proposed code would look like for context:

function main()
{
	output(new FlxState("a")); // Warning : use ()->FlxState)
	output(FlxState.new.bind("b"));
	output(()->new FlxState("c"));
}

function output(buildState:StateSupplier)
{
        var state:FlxState = buildState();
}

abstract StateSupplier(()->FlxState) to FlxState to 
{
        @:op(a())
        public function build():FlxState
        {
                return this();
        }

	@:from
	@:deprecated("use ()->FlxState)")
	static public function fromState(state:FlxState):StateSupplier
	{
		return () -> state;
	}
	
	@:from
	static public function fromMaker(func:()->FlxState):StateSupplier
	{
		return cast func;
	}
}

class FlxState
{
	public var foo:String;
	
	public function new (foo:String) this.foo = foo;
}

The major benefit here is that output(StateSupplier) doesn't even need to KNOW if the StateSupplier was created from an FlxState or from a function, and when you're ready to fully remove the deprecated functionality you can simply replace the method signature with output(buildState:()->FlxState) without modifying the method body.

[Geokureli]

I guess I'm just brainstorming use cases for newly available language features.

How new is this feature? I'm hesitant to raise HaxeFlixel's minimum version, too much, too fast, and for this case the () op has very little benefit over having a create() method, especially since this is only used on the backend. The noteworthy differences of your approach are:

1. It converts the FlxState to a function immediately, rather than on create.
2. You convert the state instance to a function that returns that instance.

Your method is more simple but unfortunately doesn't work with FlxG.resetState, which switches to a new instance of the current state by calling the supplied state-creating function. In your case reset state would just switch to the current state instance. and we can't just switch to Type.createInstance(Type.getClass(currentState), []); is resetState because that ignores constructor args in cases such as: FlxG.switchState(()->new PlayState(foo, bar)). This is why I have a create() and a getConstructor(), each using isTypeOf(); each of which is only called once per state switch, which should perform fine on all targets.

[Geokureli]

added https://github.com/HaxeFlixel/flixel/pull/2768, once it's merged I'll update FlxTransitionState and everything that uses switchTo for an outro to use startOutro. After that, I'll test https://github.com/HaxeFlixel/flixel/pull/2733 with switchTo removed and it should pass, but it will still be a breaking change

[Geokureli]

good news, it worked! Gonna close this PR in light of #2733 which is a more frictionless way of introducing this.

I appreciate everyone's help on this!