Need some critiquing

I’m fairly new to jMonkey… I should mention I’m only a few months into java development as well. As well as it is my first language to dive into… haaaa i know right. Anyways I just wanted to see about getting someone to critique this code and recommend better ways of doing things. This is very basic stuff, but I want to make sure I’m going in the right direction. Thanks in advance

Main

[java]
package mygame;

import com.jme3.app.SimpleApplication;
import com.jme3.niftygui.NiftyJmeDisplay;
import de.lessvoid.nifty.Nifty;

public class Main extends SimpleApplication {

private Nifty nifty;

public static boolean gs = false;
public static boolean changestate;

GameInterface gameInterface = new GameInterface();
GameState gameState = new GameState();

public static void main(String[] args) {
    Main app = new Main();
    app.start();
}

@Override
public void simpleInitApp() {
    
    initializeNiftyInterface();
    initializeGameInterface();
    
}

@Override
public void simpleUpdate(float tpf) {
    if (changestate){
        if (gs){
            initializeGameState();
            changestate = false;
        }
        if (!gs){
            gameState.setEnabled(false);
            changestate = false;
        }
    }
}

public void initializeNiftyInterface(){
    NiftyJmeDisplay niftyDisplay = new NiftyJmeDisplay(getAssetManager(), getInputManager(), getAudioRenderer(), getGuiViewPort());
    nifty = niftyDisplay.getNifty();
    getGuiViewPort().addProcessor(niftyDisplay);
    nifty.fromXml("Interface/GameInterface.xml", "start", gameInterface);
}

public void initializeGameInterface(){
    stateManager.attach(gameInterface);
    gameInterface.initialize(stateManager, this);
    gameInterface.setEnabled(true);
    
    flyCam.setDragToRotate(true);
    flyCam.setEnabled(false);
}

public void initializeGameState(){
    stateManager.attach(gameState);
    gameState.initialize(stateManager, this);
    gameState.setEnabled(true);
    
    flyCam.setDragToRotate(false);
    flyCam.setEnabled(true);
}

}
[/java]

This is a new appstate for testing in switching from the “menu” to the actual game.
[java]
package mygame;

import com.jme3.app.Application;
import com.jme3.app.state.AbstractAppState;
import com.jme3.app.state.AppStateManager;
import com.jme3.material.Material;
import com.jme3.math.ColorRGBA;
import com.jme3.math.Vector3f;
import com.jme3.scene.Geometry;
import com.jme3.scene.Node;
import com.jme3.scene.shape.Box;

public class GameState extends AbstractAppState {

private Main app;
private Node rootNode;
private Geometry boxGeom;

@Override
public void initialize(AppStateManager stateManager, Application app) {
    this.app = (Main) app;
    this.rootNode = this.app.getRootNode();

    Box b = new Box(Vector3f.ZERO, 1, 1, 1);
    boxGeom = new Geometry("Box", b);
    Material mat = new Material(this.app.getAssetManager(), "Common/MatDefs/Misc/Unshaded.j3md");
    mat.setColor("Color", ColorRGBA.Red);
    boxGeom.setMaterial(mat);
}

@Override
public void setEnabled(boolean enabled) {
    super.setEnabled(enabled);
    if (enabled) {
        rootNode.attachChild(boxGeom);
    } else {
        rootNode.detachChild(boxGeom);
    }
}

@Override
public void update(float tpf) {
    super.update(tpf);
}

}
[/java]

Here is the games interface

[java]
package mygame;

import com.jme3.app.Application;
import com.jme3.app.SimpleApplication;
import com.jme3.app.state.AbstractAppState;
import com.jme3.app.state.AppStateManager;
import com.jme3.scene.Node;
import de.lessvoid.nifty.Nifty;
import de.lessvoid.nifty.screen.Screen;
import de.lessvoid.nifty.screen.ScreenController;

public class GameInterface extends AbstractAppState implements ScreenController {
private SimpleApplication app;
private Nifty nifty;
private Screen screen;
private Node rootNode;

public void bind(Nifty nifty, Screen screen) {
    this.nifty = nifty;
    this.screen = screen;
}

public void onStartScreen() {
    
}

public void startGame(String nextScreen){
    nifty.gotoScreen(nextScreen);
    Main.gs = true;
    Main.changestate = true;
}

public void onEndScreen() {
    
}

public void quitGame() {
    app.stop();
}

@Override
public void initialize(AppStateManager stateManager, Application app) {
    this.app = (Main) app;
    this.rootNode = this.app.getRootNode();   
}

@Override
public void setEnabled(boolean enabled) {
    super.setEnabled(enabled);
    if (enabled) {
        
    } else {
        
    }
}
@Override
public void update(float tpf) {
    super.update(tpf);
}

}
[/java]

And here is the interface xml for switching from “start” to “hud”

[xml]

[/xml]

Like i said all of this is very basic stuff, I just don’t want to get off to a wrong start. And if I already am please let me know, I am just wanting to learn! Thanks again! And a big thanks to the jMonkey team and everything that you do :slight_smile:

Looks okay. As a tip: implement the cleanup() methods of your AppStates even if they are in WIP state, always good to code clean from the start, you will love yourself for it later :wink:

I see you don’t have code for activation and deactivation of (i.e. enabling/disabling) GameInterface.
Nifty as such can’t be deactivated, the usual approach is to always have Nifty running and switch it to an empty screen.

You may want to test whether you can switch cleanly between AppStates. I.e. define a hotkey that swaps the AppStates.
That would also allow you to get your feet wet with input handling.

For another test, set something up that switches between AppStates automatically (say, every 0.1 seconds). Fire up JVisualVM and see whether the game starts allocating memory like mad - if it does, you have a memory leak, and figuring that out should give you some traction.

Just a few avenues to explore.
I don’t see anything else particularly odd with your code, but I haven’t checked all the details so it’s pretty well possible I overlooked something.

A few things:

  • [java]
    public static boolean gs = false;
    public static boolean changestate;[/java]

This means, anyone at any time, can change these variables, not very encapsulated. Instead of setting these as static, change them to member variables:

Then replace these calls, with setters:
[java]Main.gs = true;
Main.changestate = true;[/java]

  • [java]
    public void simpleUpdate(float tpf) {
    if (changestate){
    if (gs){
    initializeGameState();
    changestate = false;
    }
    if (!gs){
    gameState.setEnabled(false);
    changestate = false;
    }
    }[/java]

You first check if gs is true, then check if it isn’t. This is what an else statement is for :), so you can simplify this to:
[java]
public void simpleUpdate(float tpf) {
if (changestate){
if (gs){
initializeGameState();
} else {
gameState.setEnabled(false);
}
changestate = false;
}[/java]

[java]GameInterface gameInterface = new GameInterface();
GameState gameState = new GameState();[/java]

Did you mean for these to have package access? (the default when you don’t specify an accessor)

[java]if (enabled) {

    } else {

    }[/java]

:-?

Thank you all for the comments very much appreciated!

@normen said: Looks okay. As a tip: implement the cleanup() methods of your AppStates even if they are in WIP state, always good to code clean from the start, you will love yourself for it later ;)
Normen, would it be best to run super.cleanup() inside of cleanup() when I am turning the AppState off? When is the best time to use it? I'm still kind of new to java, if want point me in the right direction rather than explaining I can do some research :)
@toolforger said: I see you don't have code for activation and deactivation of (i.e. enabling/disabling) GameInterface. Nifty as such can't be deactivated, the usual approach is to always have Nifty running and switch it to an empty screen.

You may want to test whether you can switch cleanly between AppStates. I.e. define a hotkey that swaps the AppStates.
That would also allow you to get your feet wet with input handling.

For another test, set something up that switches between AppStates automatically (say, every 0.1 seconds). Fire up JVisualVM and see whether the game starts allocating memory like mad - if it does, you have a memory leak, and figuring that out should give you some traction.

Just a few avenues to explore.
I don’t see anything else particularly odd with your code, but I haven’t checked all the details so it’s pretty well possible I overlooked something.


Thanks for the info here, at first I wasnt aware that Nifty could not be activated and deactivated on the fly. I was trying to put Nifty into an AppState and quickly found out it wasn’t going to work. I will add an empty screen that will help! Also thanks for the memory leak test recommendation that will help as well.

@wezrule said: A few things:
  • [java]
    public static boolean gs = false;
    public static boolean changestate;[/java]

This means, anyone at any time, can change these variables, not very encapsulated. Instead of setting these as static, change them to member variables:

Then replace these calls, with setters:
[java]Main.gs = true;
Main.changestate = true;[/java]

  • [java]
    public void simpleUpdate(float tpf) {
    if (changestate){
    if (gs){
    initializeGameState();
    changestate = false;
    }
    if (!gs){
    gameState.setEnabled(false);
    changestate = false;
    }
    }[/java]

You first check if gs is true, then check if it isn’t. This is what an else statement is for :), so you can simplify this to:
[java]
public void simpleUpdate(float tpf) {
if (changestate){
if (gs){
initializeGameState();
} else {
gameState.setEnabled(false);
}
changestate = false;
}[/java]

[java]GameInterface gameInterface = new GameInterface();
GameState gameState = new GameState();[/java]

Did you mean for these to have package access? (the default when you don’t specify an accessor)

[java]if (enabled) {

    } else {

    }[/java]

:-?

Thanks wezrule got this stuff fixed up :slight_smile:

@normen said: Looks okay. As a tip: implement the cleanup() methods of your AppStates even if they are in WIP state, always good to code clean from the start, you will love yourself for it later ;)

Normen would you say this is a better way of cleaning up the AppState? Is there anything I missed here?

[java]
package mygame;

import com.jme3.app.Application;
import com.jme3.app.state.AbstractAppState;
import com.jme3.app.state.AppStateManager;
import com.jme3.asset.AssetManager;
import com.jme3.material.Material;
import com.jme3.math.ColorRGBA;
import com.jme3.math.Vector3f;
import com.jme3.scene.Geometry;
import com.jme3.scene.Mesh;
import com.jme3.scene.Node;
import com.jme3.scene.shape.Box;

public class GameState extends AbstractAppState {

private Main app;
private Node rootNode;
private Geometry boxGeom;
private AssetManager assetManager;

@Override
public void initialize(AppStateManager stateManager, Application app) {
    super.initialize(stateManager, app);
    this.app = (Main) app;
    this.rootNode = this.app.getRootNode();
    this.assetManager = this.app.getAssetManager();
    
    Box b = new Box(Vector3f.ZERO, 1, 1, 1);
    b.setMode(Mesh.Mode.Lines);
    boxGeom = new Geometry("Box", b);
    Material mat = new Material(this.assetManager, "Common/MatDefs/Misc/Unshaded.j3md");
    mat.setColor("Color", ColorRGBA.Red);
    boxGeom.setMaterial(mat);
    this.rootNode.attachChild(boxGeom);
    
}

@Override
public void setEnabled(boolean enabled) {
    super.setEnabled(enabled);
    if (enabled) {
        
    } else {
        cleanup();
    }
}

@Override
public void update(float tpf) {
    super.update(tpf);
    this.boxGeom.rotate(.01f, 0, 0);
}

@Override
public void cleanup(){
    super.cleanup();
    this.rootNode.detachChild(boxGeom);
}

}[/java]

Looks good.

Do not call cleanup() from setEnabled(). cleanup is called for you when you detach the state.

If you want to have stuff you do on enable/disable then you have to add that logic but that’s not what cleanup is for. cleanup() is the opposite of initialize().

1 Like
@pspeed said: Do not call cleanup() from setEnabled(). cleanup is called for you when you detach the state.

If you want to have stuff you do on enable/disable then you have to add that logic but that’s not what cleanup is for. cleanup() is the opposite of initialize().

Gotcha, that makes sense :slight_smile: Thanks!

I guess I wasn’t aware that cleanup() is automatically called upon when detaching the state. Good to know :slight_smile:

For what method is called when:
http://hub.jmonkeyengine.org/javadoc/com/jme3/app/state/AppState.html

The top javadoc tries to explain it all.

A thread about improving that javadoc had the last, almost-complete version in http://hub.jmonkeyengine.org/forum/topic/simplifying-appstate-transitions/page/6/#post-184470 .
I now see that never made it into JME. Is that work rejected? Should it be finished?

@toolforger said: A thread about improving that javadoc had the last, almost-complete version in http://hub.jmonkeyengine.org/forum/topic/simplifying-appstate-transitions/page/6/#post-184470 . I now see that never made it into JME. Is that work rejected? Should it be finished?

I haven’t looked at the thread in a while. My memory is that by the end it went way too much into threading and a bunch of other unrelated things. I always intended to go back and fix the javadoc with just the important bits but never did.

The unrelated stuff and threading got all sorted out in the end, though I’ll have to go back and double-check that it’s all properly addressed.
Should I open a new thread to get it done now, or should I put the current state of affairs into the googlecode issue tracker so it can be finished up later?

@toolforger said: The unrelated stuff and threading got all sorted out in the end, though I'll have to go back and double-check that it's all properly addressed. Should I open a new thread to get it done now, or should I put the current state of affairs into the googlecode issue tracker so it can be finished up later?

Issue tracker might be best.

'kay, will do.

I hope @brentautry is still getting some attention; how are you getting along?

@toolforger said: 'kay, will do.

I hope @brentautry is still getting some attention; how are you getting along?

I’m doing alright! However… I can’t seem to be able to figure out how to clean up this AppState. When I detach the AppState the Scene is still visible. Any suggestions here?

[java]
public class Stage extends AbstractAppState {

private Main app;
private Node rootNode;
private AssetManager assetManager;
private AppStateManager stateManager;
private InputManager inputManager;
private ViewPort viewPort;
private BulletAppState bullet;

private CollisionShape sceneShape;
private Spatial scene;
private DirectionalLight dl;
private RigidBodyControl landscape;

@Override
public void initialize(AppStateManager stateManager, Application app) {
    super.initialize(stateManager, app);
    this.app = (Main) app;
    
    this.rootNode = this.app.getRootNode();
    this.assetManager = this.app.getAssetManager();
    this.stateManager = this.app.getStateManager();
    this.inputManager = this.app.getInputManager();
    this.viewPort     = this.app.getViewPort();
    this.bullet = this.stateManager.getState(BulletAppState.class);
    
    scene = assetManager.loadModel("Scenes/Neighborhood.j3o");
    sceneShape = CollisionShapeFactory.createMeshShape((Node) scene);
    landscape = new RigidBodyControl(sceneShape, 0);
    bullet.getPhysicsSpace().add(landscape);
    this.rootNode.attachChild(scene);

    //light
    dl = new DirectionalLight();
    dl.setDirection(new Vector3f(0f, -100f, 0).normalizeLocal());
    this.rootNode.addLight(dl);
}

@Override
public void setEnabled(boolean enabled) {
    super.setEnabled(enabled);
    if (enabled) {
        
    } else {
        
    }
}

@Override
public void update(float tpf) {
    super.update(tpf);
}

@Override
public void cleanup(){
    super.cleanup();
    
    this.rootNode.detachChild(scene);
    this.rootNode.removeLight(dl);
}

}
[/java]

you not removing the physics control from the physics space in cleanup, but that doesn’t explain why you can still see the scene. Are you attaching/detaching them correctly? make sure your not calling initialize yourself

@wezrule said: you not removing the physics control from the physics space in cleanup, but that doesn't explain why you can still see the scene. Are you attaching/detaching them correctly? make sure your not calling initialize yourself

Looks like I was initializing it in my main… that did it! Thank you so much.

Is this what you mean on removing physics control? I’ll add this line to my cleanup method
[java]
this.bullet.getPhysicsSpace().remove(landscape);
[/java]

no worries, and yep, that will do it

@pspeed said: Issue tracker might be best.

Submitted as http://code.google.com/p/jmonkeyengine/issues/detail?id=589