Simplifying AppState transitions

Development
41

Yeah, and the last one just went outside… mentioning just one solution to the exclusion of all others seems quite close to endorsing.



But enough of that silliness. There was indeed useful advice, it just didn’t stand out well enough as such:


@normen said:
Not putting the logic into the AppStates. If you have e.g. An entity system all your logic and data is in there. AppStates are a tool to access the update loop, nothing else.


Well, it's not true in that general wording.
Not if you take the "App" in AppState at face value and really put the logic inside AppStates.
Also, not if you take the precedents set by FlyCamAppstate and StatsAppState, which certainly do contain all the logic needed for them.
But it's certainly valid for some other designs. I can see how one can design an application to use AppStates just as service providers. And I can also see how useless a goTo() function would be in such a context.
A better alternative would be a static function that switched between sets of AppStates, avoiding detaching/attaching AppStates that are supposed to be active in both old and new state. That thing would be useful to both designs: Those that control the AppState configuration from inside AppStates and those that control it from the outside.

Then I took a step back and asked myself: "Why did I even build the thing?"
And the answer was: "Because detaching and reattaching an AppState puts it through stateDetached, cleanup, stateAttached, initialize, which needlessly reinitializes the AppState."
So... maybe detach+attach, if it happens while inside the same cycle of the update loop, should really be a null operation.
Maybe. It might mess with the semantics of code that purposely detaches and reattaches an AppState just to reinitialize it.

Which means that enable and disable are already doing everything that's needed.
Which normen has been saying all along, actually. He just didn't explain why, so it remained unclear under which circumstances that advice should be followed and under which other circumstances it should be checked whether it applies.
In fact I routinely refuse to follow advice that I can't check for validity. I've seen too many things crash and burn horribly. Hey, one of the more egregious examples happened just last week to one of our webhosting customers - I'll be posting that to thedailywtf.com one of these days. Watch out for a story involving CNAME.

Back to the issue at hand - maybe a better explanation of the roles of attach/detach and enable/disable will be in order.
If there's any interest, I'll check the Javadocs and/or tutorials and write something up.
To make that happen, I want a "yes we'll consider adding it if it matches facts" from a dev (any dev). I don't want to be stonewalled and spend another couple of evenings of useless arguments until I finally find out what exactly isn't right, that's too much of a waste of time.
(For those who have really followed this: Yes, it's an attempt at contributing back. I'm just a hopeless optimist, it seems.)
42

I think of app states as a way to extend application’s functionality… like composition versus inheritance. It also gives you the added ability to swap things out at runtime.



So yes, it’s very nice for service-type stuff which is why I pulled fly cam and stats, etc. out into their own services so that an app can choose to have them or not. “Behavior configuraiton through subclassing” is something I try to avoid in general. It’s now entirely possible to have an app that does not subclass SimpleApplication (it’s still too limited but that’s for another discussion)… not that I necessarily encourage that.



initialize() and cleanup() are like your constructor and destructor. You would use these to setup and release things that should be true the entire time the app state is attached. For example, you could load some static geometry so that you can add it to the scene when enabled without any lag.



I have a base class that has protected abstract enable() and disable() methods because I actually do most of my app state setup and removal in those… and this way I can make sure that they properly get called during cleanup if the state is enabled and stuff. I can do all setup and teardown in these safely. I reserve init and cleanup just for long-loading or global stuff.



I have a couple main app states that represent the whole state of the application. For example, MainMenuState handles the main menu. LoginState handles logging in and then GameState runs the actual game. These don’t share any states other than the common services available to all states so I’ve never had a dependency transition problem. GameState sets up things like the HUD, camera, etc., etc… Within that there are two substates right now because Mythruna supports both build mode and game mode. Each of these already assumes that the common services (hud, chat bar, gui, etc.) are already setup for them and so they only deal with the stuff they need. The main GameState is still responsible for setting up the shared stuff.



To one of your other points:

For your problem on detaching only to then reattach again… you can check to see if the app state is already attached and then conditionally attach it. It dirties up the code a bit, I guess, but it’s a valid idiom. if( stateManager.getState(Foo.class) == null ) stateManager.attach( new Foo() ) or whatever. Though it doesn’t really help you with whether to detach or not… often the method doing the transition will know anyway… on both counts.

43

And sure go ahead if theres correct and better javadoc diffs then we’ll surely add them.

44

Hmm… here’s the plan:


  1. Rework the class Javadoc to describe the interface as a whole instead of its functions. Also, give a short overview about the design options.
  2. Rearrange the order of the member functions so that they follow the lifecycle of an AppState. Functions that may be called anytime go last.
  3. For each function, document when calls to it may happen: from which thread they may be called, what state transitions they respond to. (States: detached, initializing, running, terminating.)



    Does that sound useful?
45

The overview of design options would be better off in the wiki, with a reference to the wiki page from the javadoc.



Other than that it sounds good though :slight_smile:

46
@toolforger said:
Hmm... here's the plan:

1) Rework the class Javadoc to describe the interface as a whole instead of its functions. Also, give a short overview about the design options.
2) Rearrange the order of the member functions so that they follow the lifecycle of an AppState. Functions that may be called anytime go last.
3) For each function, document when calls to it may happen: from which thread they may be called, what state transitions they respond to. (States: detached, initializing, running, terminating.)

Does that sound useful?


Yeah, I've always thought I should move some of the docs I put on AppStateManager over to AppState... and I desperately need to commit my own app state base class that properly handles enable/disable for the subclasser. Just need to figure out what to call it. :):
47

I just wanted to type the word AppState, because I don’t think it’s been typed enough in this thread.



Oh… and… APPSTATE!

1 Like
48

1 Like
49

I could not help myself:

5 Likes
50

^ lol

51

Heh. Actually, “currently recommended best practice” is to avoid putting logic into subclasses of framework classes. I think that’s solid advice, particularly in Java where you can’t inherit from more than one class.



BTW when documenting the AppState API, some nagging doubts solidified to “this is incomplete or too much, but no good in its present state”. Unfortunately, fixing that is going to break backwards compatibility.

I’m not sure how to proceed about this. Currently, I’m simply ignoring it.

52
@toolforger said:
Heh. Actually, "currently recommended best practice" is to avoid putting logic into subclasses of framework classes. I think that's solid advice, particularly in Java where you can't inherit from more than one class.

BTW when documenting the AppState API, some nagging doubts solidified to "this is incomplete or too much, but no good in its present state". Unfortunately, fixing that is going to break backwards compatibility.
I'm not sure how to proceed about this. Currently, I'm simply ignoring it.


You will have to be more specific. When messing with this stuff recently, in some cases my hands were tied and in others I have specific design considerations in mind.
53

Okay… there’s actually just one issue:

The AppStateManager shouldn’t be passed in with just some of the routines.



Either pass it in on stateAttached. If the AppState cares about it, it will store the AppStateManager in some member variable and use that.

Or pass it in on every member function. Then the AppState does not need to store the AppStateManager.



The former approach ties the AppState to a single AppStateManager at a time, the latter allows attaching an AppState to multiple AppStateManagers at the same time. (There might be efficiency trade-offs, too, but I have no idea whether they are even significant.)



Right now, the only thing that the AppState can sensibly do with the passed-in AppStateManager is to check whether it’s still the same AppStateManager as of stateAttached. Which is kinda silly.

54
@toolforger said:
Okay... there's actually just one issue:
The AppStateManager shouldn't be passed in with just some of the routines.

Either pass it in on stateAttached. If the AppState cares about it, it will store the AppStateManager in some member variable and use that.
Or pass it in on every member function. Then the AppState does not need to store the AppStateManager.

The former approach ties the AppState to a single AppStateManager at a time, the latter allows attaching an AppState to multiple AppStateManagers at the same time. (There might be efficiency trade-offs, too, but I have no idea whether they are even significant.)

Right now, the only thing that the AppState can sensibly do with the passed-in AppStateManager is to check whether it's still the same AppStateManager as of stateAttached. Which is kinda silly.


What it allows you to do is completely ignore the "almost never needed" stateAttached() and stateDetached() methods. But they are still there if you need them.

An AppState should only belong to one state manager at a time and we should probably add code to AppStateManager to enforce this if it doesn't already.
55

Second point first:

I agree that AppStates aren’t supposed to be attached to more than one AppStateManager. Not at the same time anyway.

I also agree that it’s desirable to have this enforced.

I don’t think it’s only partially possible with the current AppState interface though - you can check isInitialized, but it won’t cover the case of multiple attach calls during the same cycle of the update loop.



First point:

a) Subclasses of the AppState interface need to implement stateAttached anyway. Subclasses of AbstractAppState don’t need to override stateAttached if AbstractAppState#stateAttached saves a reference to the AppStateManager.

b) Passing AppState to stateDetached still serves no useful purpose.



One last thing: I have a first draft of the reworked interface ready, but the patch is unreadable because too much has moved.

Is it okay if I post a link to github instead? It allows inspecting each intermediate change that I care to upload, so it should be easier to follow the changes there.

56

Sorry but this is getting a bit too complicated, I don’t think anyone will find the time to compare changes on remote servers (imagine we’re supposed to do that with multiple branches) especially with changed classes. Simple diffs for proposed changes or doc additions would be much better. Though I don’t see that any API changes to the AppState classes will be made before release, its not like jME3 is in the planning phase still.

57
@toolforger said:
Second point first:
I agree that AppStates aren't supposed to be attached to more than one AppStateManager. Not at the same time anyway.
I also agree that it's desirable to have this enforced.
I don't think it's only partially possible with the current AppState interface though - you can check isInitialized, but it won't cover the case of multiple attach calls during the same cycle of the update loop.

First point:
a) Subclasses of the AppState interface need to implement stateAttached anyway. Subclasses of AbstractAppState don't need to override stateAttached if AbstractAppState#stateAttached saves a reference to the AppStateManager.
b) Passing AppState to stateDetached still serves no useful purpose.

One last thing: I have a first draft of the reworked interface ready, but the patch is unreadable because too much has moved.
Is it okay if I post a link to github instead? It allows inspecting each intermediate change that I care to upload, so it should be easier to follow the changes there.


Do not change the AppState interface. a) it will break everyone. b) there is nothing about it that really needs to be changed.

True we cannot enforce it... I guess we just have to mention it in the docs.

Most apps should be subclassing AbstractAppState anyway and they don't have to do anything with stateAttached and stateDetached. AbstractAppState does not keep a reference to the state manager and this should not be a requirement. The stateAttached, stateDetached, initialize, and cleanup methods are like event notifications and its useful to pass whatever source information is available... in this case the state manager triggering the event. cleanup() is a special case since the state manager has already been stateDetached() by that point. If I were going to break everyone then it would be to pass the state manager to cleanup, though... but I can make a logical case for why it isn't.

update() is more of an operational method and it makes sense that it doesn't need the state manager... though if I'd written the interface, I'd probably have passed it in.

None of these are compelling enough cases to break backwards compatibility for *every app that uses app states*.
58

And note regarding moving the methods around… I don’t think this is strongly needed since the javadoc will just sort them alphabetically anyway. That would greatly simplify your patch, too.

59

I think it would be cleaner if AbstractAppState saved a protected reference to the AppStateManager, also this wouldn’t break compatibility.

Now some methods would unnecessarily pass the AppStateManager wich you could just ignore or you could duplicate them with a new that doesn’t pass AppStateManager. If so you can choose which to override (the new one) and still not break compatibility.



In the next API change you just remove the deprecated methods. Isn’t that a good solution?

60

I don’t really have a big problem with the current methods so I’m not inclined to send everyone through the deprecation pain. I don’t have a problem with abstract app state keeping a reference to the state manager… though I already have an alternate base class I want to commit that I’d recommend get extended instead of abstract app state and it already does that.



That one is a harmless change, though.