Why does my if statments stop working when I remove System.out statments

Newbie & Debugging Questions
#1

here’s that code
the game loop just in case the whole thing is the source of my issue
GameState is a object that holds three enum’s one for current, saved, and test.
the states get changed by other classes from Jbutton presses

 
while(done == false){
			if(player.getHealth() <= 0){
                            System.out.println("player died is called");
                            gameOver();
                            break;
                        }
                        update();
			
		}

the code in question


public void update(){
      if(gameState.getCurrentGameState() == GameState.mainMenu){
             
      }else if(gameState.getCurrentGameState() != gameState.getTestState()){
          vc.removeObjectsFromChoiceArea();
          System.out.println("states where different thus game direction should"
                  + "be adjusted");
          if(gameState.getCurrentGameState() == GameState.normal){
              System.out.println(gameState.getCurrentGameState());
              System.out.println("normal called");
              //reminds the player where they are and rebuilds buttons
              reestablishRoom();
              gameState.setTestState(GameState.normal);
          
      }else if(gameState.getCurrentGameState() == GameState.begining){
            
            System.out.println(gameState.getCurrentGameState());
            System.out.println(gameState.getTestState());
            System.out.println("begining called");
            gameState.setTestState(gameState.getCurrentGameState());
            
            firstGo();
            
          }else if(gameState.getCurrentGameState() == GameState.battle){
              if(gameState.getSavedGameState() == GameState.battle){
                  battle.returnToBattle();
                  gameState.setSavedGameState(null); 
                  
              }else{
              battlePhase();
              }

          }else if(gameState.getCurrentGameState() == GameState.inventory){
              gameState.setSavedGameState(gameState.getTestState());
              gameState.setTestState(gameState.getCurrentGameState());
              invent.go();

              
          }else if(gameState.getCurrentGameState() == GameState.event){
              gameState.setSavedGameState(gameState.getCurrentGameState());
              gameState.setTestState(gameState.getCurrentGameState());
              
      
          }
      }else if(gameState.getCurrentGameState() == GameState.inventory){
          
      }

in the else if statement with the first go present I’ve found the removing the

  System.out.println(gameState.getCurrentGameState());
            System.out.println(gameState.getTestState());
            System.out.println("begining called");
            gameState.setTestState(gameState.getCurrentGameState());
            
            firstGo();

statments that the game never calls that branch I’m wonder is that because the System.out.println forces the threads to refocus? I haven’t made any explicit threads here but I’m using swing components which I believe do have threads in the background or something.

Any help is greatly appreciated. if I can make anything more clear let me know.

#2

What do you have in gameOver(); ?

#3

gameOver just gives text to the view controller to add to the screen and then sets done = to true. This my first big project that doesnt come from a book so its gone though many reworks to change with my growing knowledge. I just figured out how to turn it to follow the MVC pattern more. why do you want to know whats in gameOver? gameOver is never called when I’m having issues if thats what your thinking.

#4

try playerHealth < 1 !

#5

I dont see how thats relevant here, and I dont see the difference between ph< 1 and ph <= 0

#6

You say that you only remove the print statements, and yet your explanation and code say otherwise:

  System.out.println(gameState.getCurrentGameState());
            System.out.println(gameState.getTestState());
            System.out.println("begining called");
            gameState.setTestState(gameState.getCurrentGameState());
            
            firstGo();

If I understand you, then this code is what you are removing, correct? It doesn’t look like you are only removing print statements. Not calling setTestState() or firstGo() must be causing your problem, so there must be something in there that causes the if statement to “trigger.”

#7

Be very wary of attributing code problems to threads or other mysterious ‘effects’ - unless you’ve found the JVM bug-of-the-century it’s much, much more likely that it’s a problem in your code.

Couple of suggested approaches:

Have you tried debugging the code line-by-line to see what’s happening? Using print to dump out status messages is fine but adding a breakpoint to the update() method and stepping through the code is more likely to diagnose this problem.

Have you checked the various game-state getter methods for side-effects? e.g. accidentally changing the state when you’re getting it.

That code is quite convoluted IMHO - it’s difficult to follow so I’m not surprised you are having logic problems (not having a go here, just suggesting).
[]Consider using a switch statement rather than lots of chained if…else clauses (assuming your game state thing is an enum).
[]What is the need for three states? And are you sure you’re getting/setting the right one in each case?
[*]Maybe strip the code right down / comment it out and start with the first couple of state changes, if that works add the next one, and so on.

I assume this is a SWING based game? Is the while statement running in it’s own background thread? Are the various methods that change the game (e.g. reestablishRoom) correctly modifying the GUI on the event-dispatch thread?

  • stride
#8

If you want help, you have to provide an MCVE (not a disconnected snippet like you’ve posted, but not your whole project either) so we can see exactly what you’re doing.

Also, attributing your problem to things like “thread focusing” and “Swing background threads” is a recipe for disaster. Take the time to understand what you’re talking about. Recommended reading: http://docs.oracle.com/javase/tutorial/uiswing/concurrency/

#9

I only remove the print statements not the methods.

I posted in the newbie section because I’m learning and consider myself a newbie. I would assume it would be expected that I would be lacking knowledge and proper skills. I’ve read a book and have read a lot of those tutorial however doing things in a tutorial or book is different from trying your own project.

With that said I’m sure the problem is my code/lack of knowledge. Which is why I posted the question so I can learn what may be the issue.

I have all getters only return and nothing for test. Like I said the code works when the system.out statements are in place. Yes it is a enum I can use a switch statement. Is that really better or just do you feel its more proper.

yes I’ve made sure the right things are being called and tested. The states are used to move the game between main menu, normal moving around, battles, and inventory.

I’ll strip it down and try that out.

I haven’t created any additional threads.

Anyways Thank-you for your help I’ll look into those things.

Thank-you everyone for your help.

#10

Cool. I had another quick look through your code but can’t think of anything else to add to the suggestions me and others have already posted.

Let us know how you get on and be sure to post the solution as it might help other newbies in the future :slight_smile:

Good luck!

  • stride
#11

So I read up on the concurrency, concurrency with swing, and watched a few videos. So I’m wondering if I’m asking to much of the event thread. What is to much for it.

I pulled the gamestate tester out of the program and wrote this smaller program.


//imports were here

public class GameStatePractice extends JFrame{
enum gameState{
    normal,
    battle,
    begining
}
    private volatile boolean done = false;
    JButton gameStateChanger;
    
    
    JLabel showGameState;
    gameState gs = gameState.begining;
    gameState test = null;
    public static void main(String[] args) {
    GameStatePractice gsp = new GameStatePractice();

     gsp.go();
     
    
}
    public void go(){
        
        this.setSize(300,300);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        showGameState = new JLabel("" + gs);
        gameStateChanger = new JButton("Change current state");
        gameStateChanger.addActionListener(new changeState());
        
        this.getContentPane().add(BorderLayout.CENTER, showGameState);
        this.getContentPane().add(BorderLayout.SOUTH, gameStateChanger);
        this.setVisible(true);
        (new Thread(new GameLoop())).start();
        
    }
   
    public void update(){
     
          
       //test to see if states are different   
       if(gs != test){
         System.out.println(""+gs+" "+test);
          if(gs == gameState.normal){
              
              showGameState.setText("game state was tested at normal");
              test = gameState.normal;
              
          }else if(gs == gameState.begining){
            showGameState.setText("game state was tested at begining");
            test = gameState.begining; 
          }else if(gs == gameState.battle){
              showGameState.setText("game state was tested at battle");
              test = gameState.battle;
          }
        
        this.repaint();
        showGameState.repaint();
        
    }
   }
    
    public class GameLoop implements Runnable {

    public void run() {
        while(done == false){
            try {
                update();
                Thread.sleep(500);
            } catch (InterruptedException ex) {
                
            }
		}
    }
 }

     public class changeState implements ActionListener{
        public void actionPerformed(ActionEvent e){
            
            if(gs == gameState.normal){
                gs = gameState.battle;
            }else if(gs == gameState.begining){
                gs = gameState.normal;
            }else if(gs == gameState.battle){
                gs = gameState.normal;
            }
        }
        
    }
 }

The above worked, so I’m not sure whats wrong in my game. Thats why I’m asking that question. Also The reading suggest I need to work with SwingUtilities for my gui update stuff currently in the game it just gets added and done from the button press or the main thread. I’m going to keep messing with it but as always any help is appreciated.

#12

[quote=“cloudwindgate,post:11,topic:50196”]
There is no hard number (What would the unit be? On what system?) for what is “too much” for the EDT. Basically, you shouldn’t do long-running tasks on the EDT, because it causes your GUI to become unresponsive, since GUI events are processed on the EDT. An example would be loading a large file when you click a button. Your entire GUI will become unresponsive while the EDT loads the file. Instead, you’d spawn off another Thread (or a SwingWorker) that loaded the file, and then update the GUI when the file was done loading.

On the other hand, anything that modifies the GUI must be done on the EDT, otherwise you’ll get strange behavior and possible crashes. That’s what the SwingUtility.invokeLater() and invokeAndWait() methods are for. In the above example of loading a file on another Thread, you could use SwingUtilities.invokeLater() to update the GUI on the EDT when the Thread was done loading the file.

[quote=“cloudwindgate,post:11,topic:50196”]
The new code you posted modifies the GUI from the update() function ,and the update() function is called from a non-EDT thread. This is a pretty big no-no. You might not notice anything too bad because the GUI changes are pretty minimal, but this is a terrible habit to get into.

#13

I did read about the SwingUtility and those calls however I was just wondering how do I gauge it.

This is another one of those things the book never talked about, so as far as I knew that was just how it was done. I often see game loops with a Update(); and Render(); call. So I guess I just assumed this was the way. Thank-you for letting me know I was doing it wrong I have alot of code to figure out how to rework so it can apply to that rule.

Thank-you for your help Kevin

#14

Is this better?


//the imports where here

public class GameStatePractice extends JFrame{
enum gameState{
    normal,
    battle,
    begining
}
    private volatile boolean done = false;
    JButton gameStateChanger;
    
    
    JLabel showGameState;
    gameState gs = gameState.begining;
    gameState test = null;
    public static void main(String[] args) {
    GameStatePractice gsp = new GameStatePractice();

     gsp.go();
     
    
}
    public void go(){
        
        this.setSize(300,300);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        showGameState = new JLabel("" + gs);
        gameStateChanger = new JButton("Change current state");
        gameStateChanger.addActionListener(new changeState());
        
        this.getContentPane().add(BorderLayout.CENTER, showGameState);
        this.getContentPane().add(BorderLayout.SOUTH, gameStateChanger);
        this.setVisible(true);
        (new Thread(new GameLoop())).start();
        
    }
   
    public void update(){
     
          
        SwingUtilities.invokeLater(new Runnable(){
    public void run(){
        if(gs != test){
         System.out.println(""+gs+" "+test);
          if(gs == gameState.normal){
              
              showGameState.setText("game state was tested at normal");
              test = gameState.normal;
              
          }else if(gs == gameState.begining){
            showGameState.setText("game state was tested at begining");
            test = gameState.begining; 
          }else if(gs == gameState.battle){
              showGameState.setText("game state was tested at battle");
              test = gameState.battle;
          }
        
       
        showGameState.repaint();
        
    }
    }
 });
 }
    
    public class GameLoop implements Runnable {

    public void run() {
        while(done == false){
            try {
                update();
                Thread.sleep(100);
            } catch (InterruptedException ex) {
                
            }
		}
    }
 }
   


     public class changeState implements ActionListener{
        public void actionPerformed(ActionEvent e){
            
            if(gs == gameState.normal){
                gs = gameState.battle;
            }else if(gs == gameState.begining){
                gs = gameState.normal;
            }else if(gs == gameState.battle){
                gs = gameState.normal;
            }
        }
        
    }
 }
#15

Looks fine to me, although if you’re doing everything on the EDT, why not just use a Swing Timer instead of a Thread?

#16

Yikes! Since when is it recommended to do the “update” part of a game loop on the EDT? :o The presentation layer has plenty to do without having to do updates as well. You lose multi-core benefits if everything is done on the EDT. This is a big reason why the Swing timer is so terrible for getting good frame rates. “Killer Game Programming” has a classic comparison of game loops versus the use of the util.Timer versus the Swing.Timer, and the Swing.Timer gets clobbered big time. It’s really not a recommended option for a game loop control. (util.Timer passes their tests, though).

Ideally, the game loop update() code should not act on any Swing components directly. It should only be acting on the “model” layer, the underlying data. Sometimes we get lazy and and use the .value method of a given control, rather than posting that value to a model-layer variable. Sometimes we get away with it, other times it leads to debugging headaches.

The render() code (aka “presentation” layer) can refer to the model data as needed to determine how to render, but it should only read, never change anything at the “model” level. The render code can/should all be on the EDT. AFAIK this happens automatically when you act on a Swing control.

When I first saw the code you wrote, I was highly suspicious of some of the practices such as executing a function within the println method. It is kind of asking for trouble and difficult debugging. That function could easily be hiding side effects. If that function changes anything in your underlying data or game state, then deleting or commenting out that comment could have very unpredictable effects.

There is a really good book on Java coding style that I’m re-reading at the moment. I highly recommend it. The emphasis is writing code that you (or anyone else) can read and debug easily, even 6 months or 6 years after the code was written and it is stone cold in your mind.

Robert C. Martin “Clean Code – A Handbook of Agile Software Craftsmanship”

#17

I agree that Threads are much better for performance than Swing Timers. But in the above example, what logic could he possibly move to another Thread?

I suppose he could have the if statements on another Thread and then wrap the bodies in SwingUtilities.invokeLater(), but what would be the point of that?

#18

@Kevin - I think you are right about this example. I just got triggered by the mention of Swing.Timers.

#19

The issue did seem to lie in the underlying threads or more accurately my lack of knowledge(in any of it) of how thread safe swing components are. Placing the gameState changer into an invokeLater of the SwingUtility fixed the issue and the issue only resurfaced if other methods dare called the problematic method.

I have a lot to learn and I’m not saying this is the absolute answer as I don’t know enough to do so.

I would just like to thank everyone for there suggestion and helpful comments.