HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Event-based Xbox Controller polling

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pollingcontrollerbasedxboxevent

Problem

The goal of the following code is to be able to use an Xbox Controller in a Java program, it works with the jinput library.

Unfortunately this library is rather old, has little to no documentation and uses a polling system. There are some existing event-based (xbox) controller input systems, but most of them are embedded in 3D game engines, so I got the idea to make my own library, these are the ultimate goals:

  • It should support multiple types of controllers.



  • It should have some kind of plug-in system for the native DLLs, so instead of jinput I should be able to use some other library.



These goals have not been met yet, as currently it just wraps over the jinput library.

In order to understand some of the weird stuff that is going on in my classes, you will need to understand the following things that are going on in the jinput library:

  • The left and right triggers are represented as a single z-axis, so you are not able to identify if the left or right trigger is held down, you instead get an average value of both triggers. If you hold down the left trigger only, you get a value of 1, if you hold down the right trigger only you get a value of -1 and if you hold down both triggers or neither, then you get a value of 0.



  • It uses 0f and 1f to represent not pressed and pressed button states.



  • For the D-pad (directional pad) it uses the values 0f, 0.125f, 0.25f, 0.375f, 0.5f, 0.625f, 0.75f, 0.875f and 1f for respectively the off state and the possible buttons that have been pressed down.



  • The event system provided by jinput is non-blocking, meaning that still need to loop every time interval and read all events that have occured since last time.



  • I haven't been able to get the rumblers of the controller to work, so there is no support for this at all in my library.



Some relevant information about jinput can be found in this topic rather than in non-existing documentation.

With that said, I have implemented the following:

  • An event-based system wher

Solution

A few thoughts, based on general concerns

startListening and stopListening suggest lifecycle management, which in turn suggests that there is a state machine here. The fact that you are throwing IllegalStateException is supporting evidence. But your state machine implementation is currently implicit. You should make it explicit.

In particular

  • Can startListening be called again after stopListening has been invoked?



  • Does it make sense to call addListener or setDeadzone after startListening has been invoked?



I don't know what the right answer is for the controller library; but the library should be written in such a way that the answers are obvious to the users.

It seems to me that the XboxGamepad you have implemented has too many different responsibilities.

  • In your example, where you are invoking addListener and setDeadzone, it looks like a Builder



  • In startListening and stopListening, it looks like a LifeCycleManager



  • In startListening, it appears to be a Factory



  • In poll, it's an EventListener



For instance, I made an attempt at refactoring the startListening method.

public void startListening() {
    if (pollTimer != null) {
        throw new IllegalStateException("You are already listening to events");
    }
    pollTimer = new Timer(true);
    this.pollTimer.scheduleAtFixedRate(new TimerTask() {
        @Override
        public void run() {
            poll();
        }
    }, 0, pollDelay);
}


So the first thing I see is that XboxGamepad doesn't need a pollDelay itself; it is just passing it through to somebody else. For instance, suppose we had a little PollingScheduleFactory, implemented so:

class PollingScheduleFactory {
    private final int pollDelay;

    public Timer schedule(final Runnable task) {
        Timer pollTimer = new Timer(true);
        pollTimer.scheduleAtFixedRate(new TimerTask() {
            @Override
            public void run() {
                task.run();
            }
        }, 0, pollDelay);

        return pollTimer;
    }
}


Then startListening would look like

public void startListening() {
    if (pollTimer != null) {
        throw new IllegalStateException("You are already listening to events");
    }
    pollTimer = this.pollingScheduleFactory.schedule(new Runnable() {
            @Override
            public void run() {
                poll();
            }      
    });
}


Which didn't buy me as much as I had hoped. What I think the code should look like is

public void startListening() {
    if (pollTimer != null) {
        throw new IllegalStateException("You are already listening to events");
    }
    pollTimer = this.pollingScheduleFactory.schedule(pollTask);
}


if you can re-use the same task after the next stop/start, or

public void startListening() {
    if (pollTimer != null) {
        throw new IllegalStateException("You are already listening to events");
    }
    pollTimer = this.pollingScheduleFactory.schedule(pollTaskFactory.newTask());
}


if you need a new instance every time around. The code looks cleaner, but it doesn't eliminate the need to create an inner class instance somewhere.

But if can separate the polling from the LifeCycle management, then we get a much nicer line. From your example, I'm imagining something like

XboxGamepad.Builder builder = new XboxGamepad.Builder(controller, 10);
builder.setDeadzone(...);
builder.addListener(...);
XboxGamepad gamepad = builder.build();

gamepad.startListening();


Where Builder.build() looks something like....

public XboxGamepad build () {
    PollTask pollTask = new PollTask(gamepad, eventListeners, ...);
    PollingScheduleFactory scheduleFactory = new PollScheduledFactory(pollDelay);
    ...
    return new XboxGamepad(pollTask, scheduleFactory);
}


Again, if the pollTask can be different after each start, then a pollTaskFactory would be passed in instead.

If I were implementing this library, I'd want to go the extra mile to use a Fluent Builder. And I would also consider setLinearDeadzone and setRadialDeadzone -- a Fluent Builder, or a family of Fluent Builders, can take a lot of the burden off of your clients without necessarily removing the flexibility offered by the generic interface.

public interface Event {
    long getTime();
}


If all of your events are going to be handling time the same way, then you should probably be implementing an AbstractEvent that manages that for you.

abstract class AbstractEvent implements Event {
    private final long time;

    protected AbstractEvent(long time) {
        this.time = time;
    }

    long getTime() {
        return this.time;
    }
}


The Deadzone code looks very suspicious

```
private static class Deadzone {
private Component eventAxis;
private final List axes;
private final DeadzoneType deadzoneType;
private final float value;

private Deadzone(Component eventAxis, List

Code Snippets

public void startListening() {
    if (pollTimer != null) {
        throw new IllegalStateException("You are already listening to events");
    }
    pollTimer = new Timer(true);
    this.pollTimer.scheduleAtFixedRate(new TimerTask() {
        @Override
        public void run() {
            poll();
        }
    }, 0, pollDelay);
}
class PollingScheduleFactory {
    private final int pollDelay;

    public Timer schedule(final Runnable task) {
        Timer pollTimer = new Timer(true);
        pollTimer.scheduleAtFixedRate(new TimerTask() {
            @Override
            public void run() {
                task.run();
            }
        }, 0, pollDelay);

        return pollTimer;
    }
}
public void startListening() {
    if (pollTimer != null) {
        throw new IllegalStateException("You are already listening to events");
    }
    pollTimer = this.pollingScheduleFactory.schedule(new Runnable() {
            @Override
            public void run() {
                poll();
            }      
    });
}
public void startListening() {
    if (pollTimer != null) {
        throw new IllegalStateException("You are already listening to events");
    }
    pollTimer = this.pollingScheduleFactory.schedule(pollTask);
}
public void startListening() {
    if (pollTimer != null) {
        throw new IllegalStateException("You are already listening to events");
    }
    pollTimer = this.pollingScheduleFactory.schedule(pollTaskFactory.newTask());
}

Context

StackExchange Code Review Q#107143, answer score: 4

Revisions (0)

No revisions yet.