KeyBindingManager's support of key combinations in non-repeatable mode is very poor. If there are no general objections to me patching KeyBindingManager, I'll extract just this fix from my delta and post it here for your review.
Issue #1: Meta keys like SHIFT and CONTROL are consumed when repeatable is not set. This means that it just is not going to work as you want if you want to use non-repeatable meta combinations. EXAMPLE: Map LSHIFT-HOME. Hold LSHIFT down and click&release HOME: it triggers. Still holding the LSHIFT key down, click HOME: No action since the SHIFT has been consumed. (KeyBindingManager does not allow for any key-specific repeatability).
Issue #2: KeyBindingManager supports multiple lists of keys. Unfortunately, if you use the same key more than once in non-repeat mode, the first check for that key will clobber all the rest, since it will consume it. EXAMPLE: Map HOME + SHIFT and also HOME + CTRL. The second mapping will never trigger, since the check for SHIFT-HOME will consume the HOME key.
Issue #3: No support for negativity in key combinations. (You would actually have to fix the previously listed problem to hit this one). EXAMPLE: If I map SHIFT-HOME and map just HOME (after applying fix to previous problem), HOME will always trigger, regardless of whether SHIFT-HOME triggers. If that is not what you want, there is no way to tell KeyBindingManager to match HOME without SHIFT. I have not fixed this limitation and am working around it by checking states in my app.
No objection here, the KeyBindingManager has needed work for a while.
Keep up the good work blaine 
Patch:
Index: src/com/jme/input/KeyBindingManager.java
===================================================================
--- src/com/jme/input/KeyBindingManager.java (revision 4280)
+++ src/com/jme/input/KeyBindingManager.java (working copy)
@@ -32,8 +32,11 @@
package com.jme.input;
+import java.util.Arrays;
import java.util.ArrayList;
import java.util.HashMap;
+import java.util.Set;
+import java.util.HashSet;
/**
* <code>KeyBindingManager</code> maintains a list of command and
@@ -50,11 +53,67 @@
public class KeyBindingManager {
//singleton instance
private static KeyBindingManager instance = null;
+ private Set<Integer> alwaysRepeatableKeyCodes =
+ new HashSet<Integer>(Arrays.asList(
+ KeyInput.KEY_LMETA,
+ KeyInput.KEY_LCONTROL,
+ KeyInput.KEY_LSHIFT,
+ KeyInput.KEY_RMETA,
+ KeyInput.KEY_RCONTROL,
+ KeyInput.KEY_RSHIFT
+ ));
+ private Set<Integer> queuedKeyRestrictions = new HashSet<Integer>();
+ private Set<Integer> queuedKeyReleases = new HashSet<Integer>();
+ /*
+ * This method is for users who map the same keyboard key to more than one
+ * action mapping, and who use non-repeatable mode.
+ * In this case, call isValidCommand with applyKeyRestricts false, then
+ * directly invoke this method afterwards.
+ *
+ * @see #isValidCommand(String, boolean, boolean)
+ */
+ public void applyQueuedKeyRestrictions() {
+ if (queuedKeyRestrictions.size() > 0)
+ synchronized (queuedKeyRestrictions) {
+ for (Integer newKey : queuedKeyRestrictions)
+ restrictKey[newKey] = true;
+ queuedKeyRestrictions.clear();
+ }
+ if (queuedKeyReleases.size() > 0) {
+ synchronized (queuedKeyReleases) {
+ for (Integer oldKey : queuedKeyReleases)
+ restrictKey[oldKey] = false;
+ queuedKeyReleases.clear();
+ }
+ }
+ }
+
+ /**
+ * The state of these keys is always determined by whether the key is now
+ * pressed, regardless of "repeatable" settings.
+ * <P/>
+ * To change the list, you must supply a new list here, since the default
+ * list is immutable.
+ * (You could copy the default list and then add or subtract, of course).
+ * <P/>
+ */
+ public void setAlwaysRepeatableKeyCodes(
+ Set<Integer> alwaysRepeatableKeyCodes) {
+ this.alwaysRepeatableKeyCodes = alwaysRepeatableKeyCodes;
+ }
+
+ /**
+ * @see #setAlwaysRepeatableKeyCodes(Set<Integer>)
+ */
+ public Set<Integer> getAlwaysRepeatableKeyCodes() {
+ return alwaysRepeatableKeyCodes;
+ }
+
//key mappings
private HashMap<String, ArrayList<KeyCodes>> keyMap;
- private boolean[] restrictKey = new boolean[256];
+ private boolean[] restrictKey = new boolean[256];
/**
* Private constructor is called by the getInstance method.
@@ -156,15 +215,38 @@
return isValidCommand(command, true);
}
+ /*
+ * For backwards compatibility.
+ * This method will not work right for multiple mappings involving the same
+ * keyboard key.
+ *
+ * @see #isValidCommand(String, boolean, boolean)
+ */
+ public boolean isValidCommand(String command, boolean allowRepeats) {
+ return isValidCommand(command, allowRepeats, true);
+ }
+
/**
* <code>isValidCommand</code> determines if a command is executable in
* the current state of the keyboard. That is, is a valid key pressed to
* execute the requested command.
+ * <P>
+ * If both allowRepeats and applyKeyRestricts are false, then the caller must
+ * call .applyQueuedKeyRestrictions after an atomic set of inputs has
+ * been checked.
+ * This normally means that you need to call applyKeyRestricts() in your
+ * update function after all of your .isValidCommand() calls.
+ * </P>
+ *
* @param command the command to check.
* @param allowRepeats allow repetitious key presses.
+ * @param applyKeyRestricts Only has effect if allowRepeats is false.
+ * The purpose of applyKeyRestricts of false is to allow you to check
+ * for the same key in multiple key bindings.
* @return true if the command should be executed, false otherwise.
*/
- public boolean isValidCommand(String command, boolean allowRepeats) {
+ public boolean isValidCommand(
+ String command, boolean allowRepeats, boolean applyKeyRestricts) {
ArrayList<KeyCodes> keyList = keyMap.get(command);
if(null == keyList) {
return false;
@@ -188,6 +270,7 @@
if (value) {
return true;
}
+ if (applyKeyRestricts) applyQueuedKeyRestrictions();
}
return false;
@@ -198,15 +281,30 @@
* If a key is down and not restricted, the key is set as restricted and true is returned.
* If a key is down and restricted, false is returned.
* If a key is not down and is restricted, the restriction is cleared.
+ * <P>
+ * If the specified key is in the alwaysRepeatableKeyCodes list (which
+ * should contain meta keys like SHIFT and CONTROL), then sticky behavior
+ * will be entirely ignored and the returned value will just indicate
+ * whether the key is now down.
+ * </P>
* @param key The key to test
* @return True if the key is a fresh key input.
*/
private boolean getStickyKey(int key) {
- if (!restrictKey[key] && KeyInput.get().isKeyDown(key)) {
- restrictKey[key] = true;
- return true;
- } else if (!KeyInput.get().isKeyDown(key) && restrictKey[key])
- restrictKey[key] = false;
+ if (alwaysRepeatableKeyCodes != null
+ && alwaysRepeatableKeyCodes.contains(Integer.valueOf(key)))
+ // The null check is just in case the user has disabled the check
+ // by setting the set to null.
+ return KeyInput.get().isKeyDown(key);
+ if (!restrictKey[key] && KeyInput.get().isKeyDown(key))
+ synchronized (queuedKeyRestrictions) {
+ queuedKeyRestrictions.add(Integer.valueOf(key));
+ return true;
+ }
+ else if (!KeyInput.get().isKeyDown(key) && restrictKey[key])
+ synchronized (queuedKeyReleases) {
+ queuedKeyReleases.add(Integer.valueOf(key));
+ }
return false;
}
@@ -247,4 +345,4 @@
public class KeyCodes {
public int[] keys;
}
-}
No newline at end of file
+}
Good work Blaine.
Thanks guys. And thanks for moving the Topic to the right forum.
Please tell me if it changes anything in the way the AWT input system works… it is a bit broken :x
gouessej said:
Please tell me if it changes anything in the way the AWT input system works... it is a bit broken :x
This delta effects whether keystrokes invoked mapped actions or not. I believe that AWK keystokes are mapped from the 3D input system. Therefore, this could effect which keystroke events make it to your AWT system, possibly effecting focus changes indirectly.
However, I have just reviewed the delta line-by-line and there is only one possible behavior change unless you use my new 3-parameter isValueCommand() method. I recommend the following procedure to definitively implicate or vindicate my work.
- Call yourKeyBindingManager.setAlwaysRepeatableKeyCodes(null) after instantiating but before using your KeyBindingManager. Run yourKeyBindingManager.getAlwaysRepeatableKeyCode() just to verify the value is really set to null. This will make the behavior exactly as it was before. If your problem goes away, then please alert me ASAP, because some subsystem is depending on the meta keys, and I need to change the default behavior. Otherwise, proceed....
- I have checked the change history of this class, and since my change is atomic and nobody has modified the file since, you can see if the problem goes away when you back out my work. To do this, check out revision 4123 of KeyBindingManager.java, overwriting your synchronized version, and rebuild. Verify that the class gets rebuilt and touch the file if necessary. If you aren't handy with Subversion, you can just download the file from http://jmonkeyengine.googlecode.com/svn-history/r4123/trunk/src/com/jme/input/KeyBindingManager.java and copy it over top of the file under your src/ directory. I don't think you will see any change in behavior, but if you do, please provide me with a test case.
Change committed with svn rev. 4292.
Won't that hurt the backwards compatibility of FirstPersonHandler, if SHIFT is already mapped to another action in existing applications for example?
we could also extend an existing test like TestKeyInput or similar.
Core-Dump said:
Won't that hurt the backwards compatibility of FirstPersonHandler, if SHIFT is already mapped to another action in existing applications for example?
we could also extend an existing test like TestKeyInput or similar.
I am thinking that it would be backwards-compatible, and would not commit until I am confident about that. I don't have the neurons available to think it through now though. If it's not safe for any reason, then definitely will not change any base class.
Good idea about TestKeyInput or similar.
Hello.
This might sound kind of off the wall, but I am new and I was just curious if it would be possible to create a test class which I could run and learn from which utilized the multiple keys per action.
Perhaps just a simple spin off of a basic black background scene with a sphere loaded, and you move forward by hitting shift + w and back by hitting control + s
Just something simple like that, it would be really helpful.
Thank you also for your contribution to fixing this code up nicely.
Eggsworth said:
Hello.
This might sound kind of off the wall, but I am new and I was just curious if it would be possible to create a test class which I could run and learn from which utilized the multiple keys per action.
Perhaps just a simple spin off of a basic black background scene with a sphere loaded, and you move forward by hitting shift + w and back by hitting control + s
Just something simple like that, it would be really helpful.
Thank you also for your contribution to fixing this code up nicely.
I wouldn't want to bloat the code base by adding a test/demo for this, but what do you think about this... A good feature which only a couple first person RPGs have (IIRC, Oblivion and Age of Conan) is to use SHIFT to accelerate movement. What if I modified the FirstPersonHandler to use SHIFT to accelerate at least the WSAD movements? Besides being generally useful (like walk vs. run), it would provide an example of both features of the patch described in this topic.
Eggsworth said:
Hello.
This might sound kind of off the wall, but I am new and I was just curious if it would be possible to create a test class which I could run and learn from which utilized the multiple keys per action....
...
I've looked, and I can't think of a quick way to add to TestKeyInput, because of the way it operates. I won't take the time to do anything more than quick because I have to work on higher-priority stuff.
It would be easy for me to add a key combo to one of the existing demos, but that would detract from the point being exemplified by that specific demo. And I don't want to bloat the code base with a new class just for this (like the demo you described in your post).
Therefore, I'll just post my own working code that sets up HOME and SHIFT-HOME to home the camera to 2 different positions. This allows me to home my camera back to the starting position or to match the default camera position of the program Blender. This should get you... and others in a similar situation... goling.
KeyBindingManager.getKeyBindingManager().add("BLENDERHOME", new int[] {
KeyInput.KEY_HOME, KeyInput.KEY_LSHIFT });
KeyBindingManager.getKeyBindingManager().add("BLENDERHOME", new int[] {
KeyInput.KEY_HOME, KeyInput.KEY_RSHIFT });
KeyBindingManager.getKeyBindingManager().add(
"CAMHOME", KeyInput.KEY_HOME);
... // then in your update method:
boolean blenderHomed =
kbm.isValidCommand("BLENDERHOME", false, false);
if (blenderHomed
|| kbm.isValidCommand("CAMHOME", false, false)) {
TransformQuaternion camTrans = blenderHomed
? blenderCamTransform : cameraHomeTransform;
Camera cam = DisplaySystem
.getDisplaySystem().getRenderer().getCamera();
cam.setLocation(new Vector3f(camTrans.getTranslation()));
cam.setAxes(camTrans.getRotation());
cam.update();
log.debug("Homed to "
+ (blenderHomed ? "BLENDER" : "HOME"));
}
} finally {
kbm.applyQueuedKeyRestrictions();
}
Notice that I can not check for the CAMHOME key combo independently of the BLENDERHOME combo, because of the issue noted below, that there is no way yet to map a negative, so even though I have mapped SHIFTHOME, HOME will still trigger also when I hit SHIFTHOME.