Beginner Question on Best Programming Practices

I made a little fps based mainly on the collision and picking tutorials which works great. The town is loaded along with Oto, and the player can run around marking points with red spheres. I even thought to set the y axis of the walk vector to zero after getting the camera direction so you cannot “walk” into the air when looking up XD

Now I want to redo the same game in a cleaner, more professional format. Though it is unnecessary for the small amount of code I have, I want to have 2 appstates: 1 for loading a chosen scene, and 1 for player input. Afterwards I may make a control for Oto and funnel the 2 appstates through a 3rd just to make sure I know exactly how to use them. This is all for educational purposes.

My question at this point involves input listening within an appstate. I’m not sure how to put in the listener. If I use AnalogListener it says it cannot override the abstract onAnalog(…) method. As I’m sure you will see, the inputManager in the setEnabled() method is just sort of hanging there with no listener initiated…

[java]package mygame;

import com.jme3.app.Application;
import com.jme3.app.state.AbstractAppState;
import com.jme3.app.state.AppStateManager;
import com.jme3.bullet.BulletAppState;
import com.jme3.bullet.control.CharacterControl;
import com.jme3.math.Vector3f;
import com.jme3.bullet.collision.shapes.CapsuleCollisionShape;
import com.jme3.input.FlyByCamera;
import com.jme3.input.InputManager;
import com.jme3.input.KeyInput;
import com.jme3.input.controls.KeyTrigger;
import com.jme3.renderer.Camera;

/**
*

  • @author Josh
    */
    public class PlayerAppState extends AbstractAppState {

    private Application app;
    private BulletAppState bulletAppState;
    private Camera cam;
    private FlyByCamera flyCam;
    private CharacterControl player;
    private Vector3f walkDirection = new Vector3f();
    private boolean left = false, right = false, up = false, down = false, shoot = false;
    private InputManager inputManager;

    @Override
    public void initialize(AppStateManager stateManager, Application app) {
    this.app = (Application)app;
    this.inputManager = this.app.getInputManager();
    this.cam = this.app.getCamera();

     bulletAppState = new BulletAppState();
     stateManager.attach(bulletAppState);
     
     setUpKeys();
     
     CapsuleCollisionShape capsuleShape = new CapsuleCollisionShape(1.5f, 6f, 1);
     player = new CharacterControl(capsuleShape, 0.05f);
     player.setJumpSpeed(20);
     player.setFallSpeed(30);
     player.setGravity(30);
     player.setPhysicsLocation(new Vector3f(0, 10, 0));
     
     bulletAppState.getPhysicsSpace().add(player);
     
     flyCam.setMoveSpeed(100);
    

    }

    @Override
    public void update(float tpf) {
    Vector3f camDir = cam.getDirection().clone().multLocal(0.6f);
    camDir.setY(0);
    Vector3f camLeft = cam.getLeft().clone().multLocal(0.4f);
    walkDirection.set(0, 0, 0);
    if (left) { walkDirection.addLocal(camLeft); }
    if (right) { walkDirection.addLocal(camLeft.negate()); }
    if (up) { walkDirection.addLocal(camDir); }
    if (down) { walkDirection.addLocal(camDir.negate()); }
    player.setWalkDirection(walkDirection);
    cam.setLocation(player.getPhysicsLocation());
    }

    @Override
    public void setEnabled(boolean pause){
    inputManager.addListener(this, “Left”).isEnabled(pause);
    }

    @Override
    public void cleanup() {
    super.cleanup();
    }

    private void setUpKeys() {
    inputManager.addMapping(“Left”, new KeyTrigger(KeyInput.KEY_A));
    inputManager.addMapping(“Right”, new KeyTrigger(KeyInput.KEY_D));
    inputManager.addMapping(“Up”, new KeyTrigger(KeyInput.KEY_W));
    inputManager.addMapping(“Down”, new KeyTrigger(KeyInput.KEY_S));
    inputManager.addMapping(“Jump”, new KeyTrigger(KeyInput.KEY_SPACE));
    }

    public void onAnalog(String binding, boolean value, float tpf) {
    if (binding.equals(“Left”)) {
    left = value;
    } else if (binding.equals(“Right”)) {
    right = value;
    } else if (binding.equals(“Up”)) {
    up = value;
    } else if (binding.equals(“Down”)) {
    down = value;
    } else if (binding.equals(“Jump”)) {
    player.jump();
    }
    }
    }[/java]

Well. Is this the full state code? I cant see your state implementing analoglistener.

I tried all the ways I can think of, but none of them work. This code snippet around onAnalog(…) is what I have now.

[java]private AnalogListener analogListener = new AnalogListener() {
public void onAnalog(String binding, boolean value, float tpf) {
if (binding.equals(“Left”)) {
left = value;
} else if (binding.equals(“Right”)) {
right = value;
} else if (binding.equals(“Up”)) {
up = value;
} else if (binding.equals(“Down”)) {
down = value;
} else if (binding.equals(“Jump”)) {
player.jump();
}
}
};[/java]

and also changing

[java]public void setEnabled(boolean pause){
inputManager.addListener(analogListener, new String[]{“Left”}).isEnabled(pause);
}[/java]

It gives me an error under the AnalogListener line and the addListener line.

addListener() doesn’t return anything so the .isEnabled on the end is bad… also, I’m not sure where isEnabled is even coming from or what you are passing a parameter to it.

Learning to code in Java and trying to program a 3D game at the same time is one of the hardest things you can do. You might want to focus on some Java tutorials before tackling the harder stuff.

I understand that, and I have indeed learned everything I know from online tutorials, which is why I am taking things one step at a time. I have no way of knowing of about any holes in my knowledge without applying it.

I’m not quite as bad off as you might think though. My problem was that I should’ve been using ActionListener all along, and had put onAnalog(String, BOOLEAN, Float). Right arguments for the wrong method! The setEnabled() method came from frantically trying to find a solution to my problem, and was indeed a newb mistake.

Thank you so much for replying on X-mas Eve. The good company of this forum is a real help.

Personally, I don’t like anonymous classes. If your application state is dedicated to some task, why wouldn’t you just implement the AnalogListener in the application state?
Then, you could override the onAnalog method like in your first code fragment. And you can register it this way:
[java] inputManager.addListener(this, new String[]{“Left”}); [/java]

@yang71: Personally I don’t like overladen classes that inherit and extend everything and anything :wink:

@yang71 I agee with @normen. There are a lot of reasons why anonymous inner classes are better. Just a few examples include: you can have multiple versions of the same interface i.e. listen to multiple objects), you can implement clashing methods if listening to different things, you can’t accidentally call the listener methods from another class.

But all of that applyes for non-anonymous inner classes too (with only difference that definition of what they do is somewhere else in same file, which is sometimes bad and sometimes good. My rule of thumb is if its more than oneliner, make it named class.

And possible problem - annonymous class created in nonstatic context (instance from whitch it was created) always have reference to its context and if its passed out and if it should be kept longer than its context, it will prolong lifetime of it’s context to its own.

1 Like

Yeah, I generally prefer non-anonymous inner classes as well. Properly hides the listener/etc., allows the listener to easily keep its own state (and can have args passed on constructor I mean), can be static (to avoid the hidden memory leak), and can be loaded up with multiple interfaces as it makes sense.

For example, it’s quite common to load up an inner class with a variety of swing listener interfaces related to a JTextField (focus listener, action listener, change listener, etc) to keep all of the functionality in one place.

Yes, non-anonymous inner classes are fine. Your post sounded like you were recommending implementing the interface directly in the outer class.

@Zarch : As far as listening to multiple objects, I tend to prefer using a custom ActionListener that reverts back to reflection for calling a specific method. In a “delphi-like” manner. That avoids the hideous switch-case upon the event source. However, I confess I’ve not checked the performance overhead, so I would not use it for critical methods. For GUI, this is usually not a problem.

@pspeed : Non-anonymous inner classes are fine. I agree.

@yang71 That sounds terrible to me. You lose performance, compile time type checking, all sorts of benefits that the inner class language feature is there to give you.

Why not just implement the listeners as inner classes? (Anonymous or not makes no real difference.) That way you get no switch statement either… Just create an inner class for each thing you are listening to and put the appropriate code in it…

Reflection using Method object delegation is not really any slower and you can use annotations to do the hook up if compile time checking is desired. It can be super convenient and clean up a lot of code. Though generally runtime (at startup) checking is good enough.

I use this approach for every JME button that I have mapped. So much easier to associate the F3 button to a toggleHud() method than it is to dirty up code with an inner class that just calls toggleHud().

Roll on the next version of Java and the new closures stuff they are talking about, that looks nice :slight_smile:

This doesn’t seem exactly on topic, but… well, I got everything here to work as I wanted, except one thing. I can’t change the movement speed. Everything is working now, but changing the value of flyCam.setMoveSpeed() does nothing.

Not seeing anything wrong with my code, I went back and copy/pasted the collision tutorial. The flyCam.setMoveSpeed() still didn’t change anything! What’s up with that?

We’d have to see your code since the code you posted before didn’t even have a valid reference to the fly cam fron SimpleApplication. Some random reference in your own class isn’t going to mean anything. Might as well point to nowhere.

Here’s my revised code, but like I said, It does the same thing with the collision tutorial’s code.

[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.bullet.BulletAppState;
import com.jme3.bullet.collision.shapes.CapsuleCollisionShape;
import com.jme3.bullet.control.CharacterControl;
import com.jme3.input.FlyByCamera;
import com.jme3.input.InputManager;
import com.jme3.input.KeyInput;
import com.jme3.input.controls.ActionListener;
import com.jme3.input.controls.KeyTrigger;
import com.jme3.math.Vector3f;
import com.jme3.renderer.Camera;

/**
*

  • @author Josh
    */
    public class PlayerAppState extends AbstractAppState{

    //private Application app;
    private SimpleApplication app;
    private FlyByCamera flyCam;
    private Camera cam;
    private CharacterControl player;
    private Vector3f walkDirection = new Vector3f();
    private boolean left = false, right = false, up = false, down = false, shoot = false;
    private InputManager inputManager;

    @Override
    public void initialize(AppStateManager stateManager, Application app) {
    //this.app = (Application)app;
    //Allows borrowing of objects from application class

     this.app = (SimpleApplication)app;
     
     this.inputManager = this.app.getInputManager();
     this.cam = this.app.getCamera();
     //Brings over methods from application class
     
     this.flyCam = this.app.getFlyByCamera();//.setMoveSpeed(10);
     
     BulletAppState bas = app.getStateManager().getState(BulletAppState.class);
     //Brings over appState from another class
     
     
     
     setUpKeys();
     
     CapsuleCollisionShape capsuleShape = new CapsuleCollisionShape(1.5f, 6f, 1);
     player = new CharacterControl(capsuleShape, 0.05f);
     player.setJumpSpeed(30);
     player.setFallSpeed(30);
     player.setGravity(30);
     player.setPhysicsLocation(new Vector3f(0, 20, 20));
     bas.getPhysicsSpace().add(player);
     
     flyCam.setMoveSpeed(1);
    

    }

    @Override
    public void update(float tpf) {
    Vector3f camDir = cam.getDirection().clone().multLocal(0.6f);
    camDir.setY(0);
    Vector3f camLeft = cam.getLeft().clone().multLocal(0.4f);
    walkDirection.set(0, 0, 0);
    if (left) { walkDirection.addLocal(camLeft); }
    if (right) { walkDirection.addLocal(camLeft.negate()); }
    if (up) { walkDirection.addLocal(camDir); }
    if (down) { walkDirection.addLocal(camDir.negate()); }
    player.setWalkDirection(walkDirection);
    cam.setLocation(player.getPhysicsLocation());
    }

    @Override
    public void cleanup() {
    super.cleanup();
    }

    private void setUpKeys() {
    inputManager.addMapping(“Left”, new KeyTrigger(KeyInput.KEY_A));
    inputManager.addMapping(“Right”, new KeyTrigger(KeyInput.KEY_D));
    inputManager.addMapping(“Up”, new KeyTrigger(KeyInput.KEY_W));
    inputManager.addMapping(“Down”, new KeyTrigger(KeyInput.KEY_S));
    inputManager.addMapping(“Jump”, new KeyTrigger(KeyInput.KEY_SPACE));
    inputManager.addListener(actionListener, “Left”,“Right”,“Up”,“Down”,“Jump”);
    }

    private ActionListener actionListener = new ActionListener() {
    public void onAction(String binding, boolean value, float tpf) {
    if (binding.equals(“Left”)) {
    left = value;
    } else if (binding.equals(“Right”)) {
    right = value;
    } else if (binding.equals(“Up”)) {
    up = value;
    } else if (binding.equals(“Down”)) {
    down = value;
    } else if (binding.equals(“Jump”) && value) {
    player.jump();
    }
    }
    };
    }[/java]

You set the camera location every frame.

[java]cam.setLocation(player.getPhysicsLocation());[/java]

You can’t both have flycam fly around and the camera follow the player. There is only one camera.

AH!! Thank you. The collision tutorial really threw me off since it put in the flyCam and the cam. So I set the speed by varying the float in:

[java]Vector3f camDir = cam.getDirection().clone().multLocal(2f);[/java]

I feel like an idiot. That shouldn’t have been such a problem.