Issue with AppStates w/ Testcase and w/ Possible Patch

I ran into an issue, where i detached a state, and was like “Why is it calling my update method when its detached”, this was after I had already cleaned it up, hence it broke :). I then tracked it to the update method of AppStateManager (shown below):

[java]

public void update(float tpf){



// Cleanup any states pending

terminatePending();



// Initialize any states pending

initializePending();



// Update enabled states

AppState array = getStates();

for (AppState state : array){

if (state.isEnabled()) {

state.update(tpf);

}

}

}

[/java]



The issue arises when you remove an appstate from another appstate in the update loop. The appstate will still be in the “array” variable if it was attached after the one which is detaching it. A testcase is shown below:



[java]

package com.mmm.util.test;



import com.jme3.app.SimpleApplication;

import com.jme3.app.state.AbstractAppState;



public class TestAppState extends SimpleApplication {



public static void main(String args) {

new TestAppState ().start();

}



@Override

public void simpleInitApp() {

stateManager.attach(appState1);

stateManager.attach(appState2);

}



private AbstractAppState appState1 = new AbstractAppState() {

@Override

public void update(float tpf) {

if (stateManager.hasState(appState2)) {

System.out.println("App State 1 update");

stateManager.detach(appState2);

System.out.println("APP State 2 Detached!!, it’s update method should not be called");

}

}

};



private AbstractAppState appState2 = new AbstractAppState() {

@Override

public void update(float tpf) {

System.out.println("App State 2 update");

}

};

}

[/java]



The results shown here:

App State 1 update
APP State 2 Detached!!, it's update method should not be called
App State 2 update


A solution which works, not sure if its the best is to add setEnabled (false) in the stateDetached() of AbstractAppState.java, but this assumes that the user has a derived stateDetached () method, and calls the super method inside:
[java]
# This patch file was generated by NetBeans IDE
# It uses platform neutral UTF-8 encoding and n newlines.
--- Base (BASE)
+++ Locally Modified (Based On LOCAL)
@@ -70,6 +70,7 @@
}

public void stateDetached(AppStateManager stateManager) {
+ setEnabled(false);
}

public void update(float tpf) {
[/java]

Another possibility is to check that the state is still inside the states array before executing the update method on it
1 Like

Checking the state is still inside the array would be order n-squared - and also might not work due to the nature of copy-on-write-array-list. Not a great solution really.



Three alternative solutions:

  1. Document this behaviour. Any app state detaching another one should disable it first if they want to avoid the second call.


  2. The State Manager calls setEnabled(false) before calling detach when it is told to detach a state. This changes behaviour though and may potentially break peoples applications. (For example if they expect the state to remember its enabled state while attaching/detaching).


  3. Enqueue attaches/detaches so they happen the following frame. (Or at least after the list iteration has finished).
4 Likes

Ah yeh i forgot people can attach it again after, and then it would still be disabled ^^, hmm. Nice suggestions, with explanations :slight_smile: im not fussed what is chosen, as long as it fixes the issue :slight_smile: and doesn’t break other stuff (which mine potentially would :))

Well (1) is simplest so that’s what I’d go for :smiley:

This is not a bug except in your code. You cannot disable an app state just because it’s detached because you are comingling two completely separate states. If I were to add back that app state then the enabled state would be messed up… which is mentioned in later comments.



So I think the bug is actually in your code as calling the update an extra time should not cause problems. Just be aware that it can happen.



For example, cleanup() is not called until after update is finished… so how did your cleanup code get called before update()?



We should document that removing an app state from another app state does not guarantee that its update won’t be called at least one more time and that if you do something silly like “anything in stateDetatched() since that’s a stupid method to use for almost any purpose” then you may have issues. :wink:

I still think its a bug irrespective of problems in my code :slight_smile: You can see from the code that the stateManager update was intended to call update only on appatates which are attached and enabled. Which is what I would also expect to happen.

I think it’s more a corner case than a bug. The behaviour in this specific case is not what might be expected but it’s not invalid either. The solution is either to change or document it but documenting it is easiest and guaranteed not to break existing code.



As already said you can work around it easily enough. Just have the state doing the detach both disable and detach the state. That’s valid because it’s inside your code so you have control over the state machine, it’s not a change in state behaviour being made by the engine under you.

Imma jus subclass Appstate and enable/disable the state on stateAttached/detached, as it suits my needs fine. An extra call to update, breaks a lot of my stuff as I’m probably gonna end up having about 60 app states (you can never have enough :)) and chopping and changing between them rapidly. An extra call to update causes a lot of dmg for me.



Thought I would share the issue anyways. As it gave me a few headaches.



5 days into development and I’m finally bug free :slight_smile: <3

@wezrule said:
Imma jus subclass Appstate and enable/disable the state on stateAttached/detached, as it suits my needs fine. An extra call to update, breaks a lot of my stuff as I'm probably gonna end up having about 60 app states (you can never have enough :)) and chopping and changing between them rapidly. An extra call to update causes a lot of dmg for me.

Thought I would share the issue anyways. As it gave me a few headaches.

5 days into development and I'm finally bug free :) <3


I wish you would describe more about the bug in your code that caused the problem, though. Detaching an app state shouldn't cause problems even if update() is run again... some app states may actually count on this behavior (ie: if the update of one app state is run then it expects the update of others to be run even if they are actually detached before the next frame).

So why does the extra update fail in your case? Maybe we can teach you (and others) how to write better app states.

I can see how this would break app states. If detach removed all objects from the scene graph and cleaned them up then update() would then try to access those objects which are now null.

@zarch said:
I can see how this would break app states. If detach removed all objects from the scene graph and cleaned them up then update() would then try to access those objects which are now null.


This is my point so I will spell it out clearly:

Do not put any code in stateDetached()... and certainly NEVER EVER put code there that modifies the scene graph. The method is nearly useless (but not completely useless) and is NOT guaranteed to run on the render thread. This is what cleanup() is for and it is GUARANTEED to run on the render thread and AFTER all states are updated.

tl;dr: Don't ever use stateDetached() for anything ever. *

* - caveat being unless you know enough to argue why you need it for some odd use-case. But that's generally true of all of my blanket statements.

So, I want to understand, given the above, what was wrong with wezrule's app states where update() was failing... because using stateDetached() would have been a user-code bug no matter what.

Ahh yes, I see what you mean. It should be added in initialize() and removed in cleanup().



So the next question is…does update() ever get called after cleanup() in this situation?

@zarch said:
Ahh yes, I see what you mean. It should be added in initialize() and removed in cleanup().

So the next question is...does update() ever get called after cleanup() in this situation?


No. That's my point.

I don’t have too much time to devote to this discussion anymore, so i’ll just leave it with a simple example:

[java]

AppState1 {



RandomClass randomObject = new RandomClass ();



update () {

if (count == 1) {

stateManager.detachAppState(AppState2);

// no longer require this object anymore

randomObject.cleanupObject ();

randomObject = null;

} else {

doSomethingElse ();

}

}



getRandomClass () {

return randomObject;

}

}



AppState2 {

update() {

stateManager.getAppState (AppState1.class).getRandomClass ().doSomething (); // and broke

}

}



RandomClass1 {



void cleanupObject () {

doStuff ();

}



void doSomething () {



}

}[/java]



As you can see, it has nothing to do with AppState cleanup. While its not maybe a normal case, is does demonstrate the pitfalls with it.



Another example (although possibly would be more suited to using SetEnabled () directly), is if I have a counter variable, and increment it each frame. If I detach the state it would have run the update once more than i wanted. Then when i attach it again, the counter is wrong.

Your example has nothing to do with app states. You null out an object and then you make a call that you think means you can get away without checking for null but it doesn’t.



When StateManager.update() runs, it runs ALL of the app states that are attached AT THAT TIME. Regardless of when they are removed in the update loop. This does not cause problems except in code that makes too many assumptions about tenuous shared state.



Also, AppState2 will break if you attach it before AppState1, for example. It hasn’t been written robustly.



This “When StateManager.update() runs, it runs ALL of the app states that are attached AT THAT TIME.” is important because it means that states added won’t be run that pass either. States added from another thread that just happen to be added in the middle of that update loop won’t be added either, etc…



It’s the only way to have consistent behavior that you can hang your hat on.


@wezrule said:
Another example (although possibly would be more suited to using SetEnabled () directly), is if I have a counter variable, and increment it each frame. If I detach the state it would have run the update once more than i wanted. Then when i attach it again, the counter is wrong.


How is the counter "wrong"? It's counting the number of frames that the app state was updated. Which is accurate. It's only "wrong" if you detach it while update() is running and it COINCIDENTALLY just hasn't been hit yet (whether detaching from another state or another thread). But that's not a bug in JME... just a misunderstanding on the part of the caller.

Having realised I was misremembering what attach/detach do (in fact it made me paranoid enough to go check my code but I was using Initialize not attach so all is well) I agree with pspeed. The current behaviour is correct.



The only thing I’d like to see is a bit more clarity/explicitness in https://wiki.jmonkeyengine.org/legacy/doku.php/jme3:advanced:application_states about when Initialize/Cleanup are called and what “remove from the game” means in this context.



For example if I attach a state initialize gets called. If I then detach it I guess cleanup gets called.



If I attach again does initialize get called again?



If I detach/attach inside one frame do cleanup and initialize get called and if so in what order?



Etc?

@zarch said:
Having realised I was misremembering what attach/detach do (in fact it made me paranoid enough to go check my code but I was using Initialize not attach so all is well) I agree with pspeed. The current behaviour is correct.

The only thing I'd like to see is a bit more clarity/explicitness in https://wiki.jmonkeyengine.org/legacy/doku.php/jme3:advanced:application_states about when Initialize/Cleanup are called and what "remove from the game" means in this context.

For example if I attach a state initialize gets called. If I then detach it I guess cleanup gets called.

If I attach again does initialize get called again?

If I detach/attach inside one frame do cleanup and initialize get called and if so in what order?

Etc?


Yeah, this doc could be expanded a little: http://hub.jmonkeyengine.org/javadoc/com/jme3/app/state/AppStateManager.html

When I wrote it, I was fixing a lot of issues with the haphazard way that AppStateManager did things... I wanted to clearly spell out the behavior that was sort of random before.

To answer your questions, initialize gets called on the update pass following attachment. It will happen every time it is attached... whether it was attached once before or not. (For the record, if you try to attach an app state instance that is currently attached then the second attach is a no-op and attach returns false.)

If you attach and detach an app state instance within the update of some other app state then it's initialize() can cleanup() will never be called. The rule is that if initialize() is called then at some point cleanup() will be called... this is true for every state even if you never detach it. As long as the app terminates normally then all initialize()'d app states have their cleanup() method called.

Also for the record, if you detach and app state and reattach it from within an app state's update then on the next update pass it's cleanup() and initialize() methods will be called in that order.

If I sound bristly about bugs reported here it's because I spent a LOT of time making sure the behavior was consistent through all code paths. I was trying to tame a mess of inconsistencies while still maintaining a modicum of backwards compatibility... it was tricky and a fun puzzle at the same time. There could very well be bugs but I'll be quick to squash the things that are behaving explicitly as intended.

Very clear thanks. If I get a moment tomorrow I’ll do a patch with extended javadoc info and update the tutorial document. Between the two they cover most things but there are still a few corner cases as you described above and it would be nice to have the full info in both places.

@zarch said:
Very clear thanks. If I get a moment tomorrow I'll do a patch with extended javadoc info and update the tutorial document. Between the two they cover most things but there are still a few corner cases as you described above and it would be nice to have the full info in both places.


Agreed. There was an effort by someone some time back to patch the docs but it kind of went off the deep end into unnecessary stuff about threading models and so on. To me: better less information that is 100% accurate than lots of erroneous and/or unrelated information.

In my experience just tidying up bits as you come to them has more effect over the long run than any attempt at a “mass effort”. I know a few people who will start a project like that and follow it through to the end but far more that get sidetracked or disheartened half way through.



Patch committed. Wiki updated.

3 Likes