Changing the way FlxStates are called #2565

Cheemsandfriends · 2022-05-27T20:56:28Z

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

… thing

Geokureli · 2022-05-28T01:00:21Z

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

Geokureli

This is going to be a pretty massive PR. I don't recommend making all of the changes directly in github on a web browser. it should be made locally, and we'll need to change Flixel-ui, flixel-addons, AAAAAALLLL of the demos, some snippets, the tutorial, and probably more, and we'll need to test them all together before anything is merged

Geokureli · 2022-05-28T01:04:16Z

flixel/FlxG.hx

 	{
-		if (state.switchTo(nextState))
-			game._requestedState = nextState;
+		if (state.switchTo(nextState()))


state.switchTo() should take a ()->FlxState rather than a FlxState instance, it should store the function in _requestedState and wait until the new stuff is all set up to call it, creating the state

Geokureli · 2022-05-28T01:09:14Z

flixel/FlxG.hx

@@ -379,7 +379,7 @@ class FlxG
 	 */
 	public static inline function resetState():Void
 	{
-		switchState(Type.createInstance(Type.getClass(state), []));
+		switchState(() -> Type.createInstance(Type.getClass(state), []));


once we start storing the state creator function we can just call that to create a new one, rather than needing to use Type.createInstance.

with Type.createInstance FlxStates can't have constructor args, by passing in functions we can do things like

FlxG.switchState(()->{ return new MyState(arg1, arg2); });

and we'll know how to switch to that state AND reset it

Oh, so like a function? I like it, I like it

flixel/FlxState.hx

Geokureli · 2022-05-28T01:10:53Z

flixel/system/FlxSplash.hx

@@ -199,7 +199,7 @@ class FlxSplash extends FlxState
 		#end
 		FlxG.stage.removeChild(_sprite);
 		FlxG.stage.removeChild(_text);
-		FlxG.switchState(Type.createInstance(nextState, []));
+		FlxG.switchState(() -> Type.createInstance(nextState, []));


nextState should also be a ()->FlxState, and the FlxGame constructor should take a function instead of a Class Ref

Hm So like making the PlayState.new also in FlxGame?
I liked that you didn't need to add anything, just the name but ok

where before you had

addChild(new FlxGame(400, 300, MenuState);

now you can just do

addChild(new FlxGame(400, 300, MenuState.new);

the reason for this is to allow constuctor args, like

addChild(new FlxGame(400, 300, MenuState.new.bind(arg));

Cheemsandfriends · 2022-05-28T11:14:39Z

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

Geokureli · 2022-05-28T17:36:00Z

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 · 2022-05-28T18:19:46Z

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

Geokureli · 2022-05-28T18:26:54Z

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 · 2022-08-13T22:49:48Z

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 · 2022-08-13T23:58:57Z

I've got a question @Geokureli, what is this code supposed to do exactly?

I don't quite understand it

Geokureli · 2022-08-16T18:58:26Z

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 · 2023-01-20T19:43:33Z

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 · 2023-02-14T09:18:31Z

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 · 2023-02-14T17:58:48Z

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 · 2023-02-14T18:33:01Z

@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 · 2023-02-14T19:28:37Z

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 · 2023-02-14T19:36:31Z

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 · 2023-02-14T22:24:18Z

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 · 2023-02-16T06:34:32Z

looks like its doable, #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 · 2023-04-06T19:50:49Z

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 · 2023-04-06T20:00:49Z

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 · 2023-04-06T20:52:00Z

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 · 2023-04-07T17:06:07Z

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:

It converts the FlxState to a function immediately, rather than on create.
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 · 2023-04-07T21:00:33Z

added #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 #2733 with switchTo removed and it should pass, but it will still be a breaking change

Geokureli · 2023-05-25T06:17:40Z

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!

Cheemsandfriends added 3 commits May 27, 2022 22:46

Add the new way of adding the substates

01f8ef5

override the FlxGroup's new thing so that it works with the State.new…

3f72b87

… thing

adding the FlxSplash switch state thingy

f69464c

Geokureli requested changes May 28, 2022

View reviewed changes

Cheemsandfriends added 5 commits August 14, 2022 14:52

Merge branch 'HaxeFlixel:dev' into patch-5

cf894af

Change the way the FlxGame is called

921d325

Change switchState() and resetState()

706f425

Change nextState variable

292fe03

forgot to remove some trace testing

bff148b

Cheemsandfriends requested a review from Geokureli August 14, 2022 13:00

Merge branch 'HaxeFlixel:dev' into patch-5

ba94f73

Geokureli added this to the 6.0.0 milestone Jan 20, 2023

Geokureli mentioned this pull request Apr 7, 2023

add startOutro, deprecate switchTo #2768

Merged

Geokureli closed this May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing the way FlxStates are called #2565

Changing the way FlxStates are called #2565

Cheemsandfriends commented May 27, 2022

Geokureli commented May 28, 2022 •

edited

Loading

Geokureli left a comment

Geokureli May 28, 2022

Geokureli May 28, 2022

Cheemsandfriends May 28, 2022

Geokureli May 28, 2022

Cheemsandfriends May 28, 2022

Geokureli May 28, 2022 •

edited

Loading

Cheemsandfriends commented May 28, 2022

Geokureli commented May 28, 2022

Cheemsandfriends commented May 28, 2022

Geokureli commented May 28, 2022 •

edited

Loading

Cheemsandfriends commented Aug 13, 2022

Cheemsandfriends commented Aug 13, 2022

Geokureli commented Aug 16, 2022 •

edited

Loading

Geokureli commented Jan 20, 2023 •

edited

Loading

T1mL3arn commented Feb 14, 2023

Geokureli commented Feb 14, 2023 •

edited

Loading

Geokureli commented Feb 14, 2023 •

edited

Loading

EliteMasterEric commented Feb 14, 2023

Geokureli commented Feb 14, 2023

EliteMasterEric commented Feb 14, 2023

Geokureli commented Feb 16, 2023 •

edited

Loading

EliteMasterEric commented Apr 6, 2023

Geokureli commented Apr 6, 2023 •

edited

Loading

EliteMasterEric commented Apr 6, 2023

Geokureli commented Apr 7, 2023 •

edited

Loading

Geokureli commented Apr 7, 2023 •

edited

Loading

Geokureli commented May 25, 2023

Changing the way FlxStates are called #2565

Changing the way FlxStates are called #2565

Conversation

Cheemsandfriends commented May 27, 2022

Geokureli commented May 28, 2022 • edited Loading

Geokureli left a comment

Choose a reason for hiding this comment

Geokureli May 28, 2022

Choose a reason for hiding this comment

Geokureli May 28, 2022

Choose a reason for hiding this comment

Cheemsandfriends May 28, 2022

Choose a reason for hiding this comment

Geokureli May 28, 2022

Choose a reason for hiding this comment

Cheemsandfriends May 28, 2022

Choose a reason for hiding this comment

Geokureli May 28, 2022 • edited Loading

Choose a reason for hiding this comment

Cheemsandfriends commented May 28, 2022

Geokureli commented May 28, 2022

Cheemsandfriends commented May 28, 2022

Geokureli commented May 28, 2022 • edited Loading

Cheemsandfriends commented Aug 13, 2022

Cheemsandfriends commented Aug 13, 2022

Geokureli commented Aug 16, 2022 • edited Loading

Geokureli commented Jan 20, 2023 • edited Loading

T1mL3arn commented Feb 14, 2023

Geokureli commented Feb 14, 2023 • edited Loading

Geokureli commented Feb 14, 2023 • edited Loading

EliteMasterEric commented Feb 14, 2023

Geokureli commented Feb 14, 2023

EliteMasterEric commented Feb 14, 2023

Geokureli commented Feb 16, 2023 • edited Loading

EliteMasterEric commented Apr 6, 2023

Geokureli commented Apr 6, 2023 • edited Loading

EliteMasterEric commented Apr 6, 2023

Geokureli commented Apr 7, 2023 • edited Loading

Geokureli commented Apr 7, 2023 • edited Loading

Geokureli commented May 25, 2023

Geokureli commented May 28, 2022 •

edited

Loading

Geokureli May 28, 2022 •

edited

Loading

Geokureli commented May 28, 2022 •

edited

Loading

Geokureli commented Aug 16, 2022 •

edited

Loading

Geokureli commented Jan 20, 2023 •

edited

Loading

Geokureli commented Feb 14, 2023 •

edited

Loading

Geokureli commented Feb 14, 2023 •

edited

Loading

Geokureli commented Feb 16, 2023 •

edited

Loading

Geokureli commented Apr 6, 2023 •

edited

Loading

Geokureli commented Apr 7, 2023 •

edited

Loading

Geokureli commented Apr 7, 2023 •

edited

Loading