HaxeFlixel / flixel

Free, cross-platform 2D game engine powered by Haxe and OpenFL
https://haxeflixel.com/
MIT License
1.93k stars 427 forks source link

Cleaning up `FlxSubState`s #3128

Closed DetectiveBaldi closed 2 months ago

DetectiveBaldi commented 2 months ago

A lot of this code is very old, with some odd workarounds for certain things.

Changes: FlxSubStates are now updated and drawn through FlxGame. This removes the need for FlxState.tryUpdate. _subStateOpened and _subStateClosed have been removed and fully replaced with their public counterparts. Remove resetSubState and related variables.

This is a draft because I want to see how CI reacts and also discuss breaking changes.

To-Do

DetectiveBaldi commented 2 months ago

Not sure why the remaining builds are failing, I'm not sure if it's related to this pull or not

Geokureli commented 2 months ago

Don't worry about CI in this case.

I don't understand the point of these changes, to be honest and I think it will cause some issues, before I list those issues I want to know what benefits this change offers because I don't see it

Edit: I also think this is multiple changes rolled into one, like I can see the point of making the signals public, but is that at all related to updating the substate from FlxGame?

DetectiveBaldi commented 2 months ago

Don't worry about CI in this case.

I don't understand the point of these changes, to be honest and I think it will cause some issues, before I list those issues I want to know what benefits this change offers because I don't see it

The main point is just to reduce complicity, having variables like _requestSubStateReset and _requestedSubState that can easily being removed for something shorter and more understandable. States having a separate internal update method is also just another way to cause confusion.

Edit: I also think this is multiple changes rolled into one, like I can see the point of making the signals public, but is that at all related to updating the substate from FlxGame?

This is true, the signal changes aren't really the main point, nor do they affect it, I just saw it and went "why not"

Geokureli commented 2 months ago
  1. the purpose of tryUpdate is to catch thrown errors from the update tree, it should be there
  2. the state should update the substate, not the game, I see no benefit to this change, and actually I think this will break things, since the substate can have substates, which can have substates, should the game check all the way down for sub-sub-substates? it's simpler having the state update and draw it's substate, which handles it's own substates
  3. _requestSubStateReset and _requestedSubState are used to control when the substates are added and reset, and it prevents errors, otherwise there are weird edge cases and caveats caused by calling open or close at certain times
Geokureli commented 2 months ago

If you see a problem that requires a large structural change like this, I recommend starting by creating an issue that points out the problems it is causing you, and we'll work together on a fix or a workaround. In this case this is a large structural change that introduces new issues and doesn't actually fix any problems, along with other unrelated changes, and arbitrary style changes