Proposed changes to game state system

Ok, I’ve been using the GameState system quite a bit and I’m finding some various tweaks that I think would be useful to the rest of the jME community.



#1 - To allow processes to be put on hold, etc when switching away from a GameState as discussed here: http://www.mojomonkeycoding.com/jmeforum/viewtopic.php?t=1187



GameStateManager:


public void switchTo(String name) {
      rootNode.detachAllChildren();
      
      current.switchFrom(); <-- new code
      current = (GameState)states.get(name);

<snip bottom of function>



This of course necessitates adding that function to the GameState class:


    /**
     * Gets called by GameStateManager when switched away from.
     */
    public abstract void switchFrom();



And then adding the implementation to StandardGameState:


/**
 * Empty.
 * @see GameState#switchFrom()
 */

  public void switchTo() {
      
  }



#2 - I don't know if anyone else has had this issue referenced here http://www.mojomonkeycoding.com/jmeforum/viewtopic.php?t=1202&postdays=0&postorder=asc&start=0 but I would like to suggest adding the following to the bottom of GameStateManager.switchTo()


<snip top of function>
      current.switchTo();
      current.getCamera().update(); <-- new code
      
      rootNode.updateRenderState();
      rootNode.updateGeometricState(0, true);
   }



#3 - The initInput method gets called before the constructor for the derived GameState class therefore you can't use your saved display refernce that you inevitabley save off at the beginning of your game state to init your mouse. I propose


protected DisplaySystem display;



be added to StandardGameState and the first line in StandardGameState.initCamera() be changed to:


display = DisplaySystem.getDisplaySystem();



#4 - There's no easy way I can see to get the current state from the GameStateManager or to get a reference to other running (but not active) states. I propose in GameStateManager to add:


/**
    * Returns the current <code>GameState</code>.
    */
   public GameState getCurrentState()
   {
       return current;
   }
   
   /**
    * Returns the <code>GameState</code> associated with the given name/key.
    *
    * @param name The name/key of the Game State to find.
    */
   public GameState getState(String name)
   {
       GameState state = (GameState) states.get(name);
       return state;
   }



That's about it. Thanks Per for getting a nice framework up and working for us to build on!

Oops, one small error there. The proposed change to the top of GameStateManager.switchTo() should read



public void switchTo(String name) {
      rootNode.detachAllChildren();
      
      current = (GameState)states.get(name); <-- I had this line below the current.switchFrom() line, oops!
      current.switchFrom();

<snipped rest of function>

@1: Totally agree, a switchFrom could be convenient.



@2: Does that actually solve your problem? As you know moving the camera and so on works in the game state test, without that cam.update()…



@3: Sorry, don’t understand what you mean.



@4: Nod, agree.



Also, didn’t you get it right the first time? :slight_smile: If doing:


      current = (GameState)states.get(name); <-- I had this line below the current.switchFrom() line, oops!
      current.switchFrom();


You'r switching from the state that you'r switching to :o

Also, when reading that switchFrom code again, I see a bug that you had nothing to do with (://): at the top of switchTo you have rootNode.detachAllChildren(), which should be replaced with rootNode.detachChild(current.getStateNode()).
"per" wrote:
@2: Does that actually solve your problem? As you know moving the camera and so on works in the game state test, without that cam.update()...

Well it works fine if I have that code in and I take out the cam.update() from my GameState's switchTo() method. It's not a big deal if I'm the only one having the issue. I'll do more testing on my theory of it being related to the 1.5 JVM.
"per" wrote:
@3: Sorry, don't understand what you mean.

In my GameStates's initInput I had the line


mouse = new AbsoluteMouse("Ingame Mouse", display.getWidth(), display.getHeight());



I have a local reference to the DisplayManager (named display of course) in my GameState that is initialized in the constructor. However, I was getting a NullPointerException for the display reference. StandardGameState's constructor contains:


public StandardGameState() {
      stateNode = new Node("State rootNode");
      initCamera();
      initInput();
   }



I though that my GameState's constructor would be called before the initInput() is called from within StandardGameState but apparently that is not correct. To resolve this I either save off a temporary reference to the DisplayManager within my initInput method or since just about everything requires a call to DisplayManager ;) why not just add a reference to it in the base class and forgo all these temporary references (and the associated method)? If I'm missing something here and getting a local copy in each method is for some reason the preferred way of doing things either in Java or jME I was unaware of that.
"per" wrote:
Also, didn't you get it right the first time? Smile If doing:



      current = (GameState)states.get(name); <-- I had this line below the current.switchFrom() line, oops!
      current.switchFrom();


You'r switching from the state that you'r switching to Surprised

Hrm, I flipped it in my own local copy of jME because I was getting a NullPointerException on the current.switchFrom line. I didn't realized it was cached as a member variable. Maybe that was some other goof in my code because you're right that would switchFrom() on the new state.

@2: If you could isolate the problem in a small test, I promise I’ll look into it.



@3: Well, the line DisplaySystem display = DisplaySystem.getDisplaySystem() will not actually allocate any new memory, since the call just passes on a reference to the DisplaySystem singleton (only one instance). So if you use that call everytime you need the DisplaySystem, everything should work fine :). The reason SimpleGame contains a reference is just for convenience (as you said, lots of stuff needs it).

"per" wrote:
@2: If you could isolate the problem in a small test, I promise I'll look into it.

I will give a go at this.
"per" wrote:
@3: Well, the line DisplaySystem display = DisplaySystem.getDisplaySystem() will not actually allocate any new memory, since the call just passes on a reference to the DisplaySystem singleton (only one instance). So if you use that call everytime you need the DisplaySystem, everything should work fine. The reason SimpleGame contains a reference is just for convenience (as you said, lots of stuff needs it).

Oh I understand it's not a huge overhead for the method call and such it just seemed like something that would be universally used so if we have input and camera reference why not have display?

Oh I understand it's not a huge overhead for the method call and such it just seemed like something that would be universally used so if we have input and camera reference why not have display?

Ok, fair enough :) What I did not understand was how the problem you had would get solved by adding this quick reference?

The “problem” it solved was having a reference to DisplayManager when initInput gets called instead of having to do DisplayManager display = DisplayManager.getDisplay(); at the top of that and every other method that needs it. Since I’m assuming everyone saves a reference to DisplayManager in their GameState implementation it is very convenient to have that built in and initialized by StandardGameState in it’s initCamera method. It is also consistent to have that reference in StandardGameState since other necessary items such as the camera and input system are already there. Also, since most people learn on SimpleGame it maintains a nice level of consistency when moving over to the GameState system.

Well, the reason StandardGameState contains a Camera and InputSystem, is that they are specific for that game state. The display system is not.



Personally, I feel that while keeping a reference to the display system isn’t a big deal :), it is kind of redundant. But maybe there are more people wanting this?

Soo… as a conclusion this is what should be added:



In GameState

/**
* Gets called by GameStateManager when switched away from.
*/
public abstract void switchFrom();



In StandardGameState

/**
 * Empty.
 * @see GameState#switchFrom()
 */
public void switchFrom() {
     
}



In GameStateManager

/**
* Returns the current <code>GameState</code>.
*/
public GameState getCurrentState() {
    return current;
}
   
/**
* Returns the <code>GameState</code> associated with the given name/key.
*
* @param name The name/key of the game state to find.
*/
public GameState getState(String name) {
    return (GameState) states.get(name);
}



PS: sorry, my cvs is screwed up atm, so I couldn't make a proper diff :(

Also, I’d like to hear opinions on what’s discussed here.



My proposal is that we ditch all the stuff that has to do with attaching stateNodes to the rootNode, and just render the current state (as DP said).



This would lead to that the GameStateManager does no longer need a reference to the rootNode, and when switching to a new GameState no attaching and detatching of nodes will be needed - all rendering is done from the render method of the current state.

I don’t see any problem with removing the attachment to rootNode. I assume the stateNode would remain and then in our render method we would just call draw on that?

Exactly - StandardGameState would still have its stateNode there, and by default draw it.

If you have these changes implemented somewhere, get them to me and I’ll put them into the cvs version.

Sure, just have to document them.