[zookeeper]

I noticed that the otherwise magnificent Controlconfigdefaults.tbl doesn't have a way to disable certain binds entirely (for example, FotG won't have glide so we don't want the glide-related binds to clutter up the control config screen and confuse people), so I wrote a patch to allow that. Disabling a bind both removes it from the control config screen and prevents the associated action from triggering in-game.

One thing to codereview is whether it's acceptable to not initialize the disabled bool member to false; I didn't, because I didn't find any suitable place to do it. Also, if there are other places in the game where the removed binds might be displayed to the user, I wouldn't mind hearing about them.

Patch:

Code: [Select]

Index: controlconfig/controlsconfig.cpp
===================================================================
--- controlconfig/controlsconfig.cpp (revision 9143)
+++ controlconfig/controlsconfig.cpp (working copy)
@@ -387,7 +387,7 @@
 
 void control_config_conflict_check()
 {
- int i, j, a, b, c, shift = -1, alt = -1;
+ int i, j, shift = -1, alt = -1;
 
  for (i=0; i<CCFG_MAX; i++) {
  Conflicts[i].key = Conflicts[i].joy = -1;
@@ -410,11 +410,16 @@
 
  for (i=0; i<CCFG_MAX-1; i++) {
  for (j=i+1; j<CCFG_MAX; j++) {
- if (Control_config[i].key_id >= 0) {
- c = 0;
- a = Control_config[i].key_id;
- b = Control_config[j].key_id;
- if (a == b) {
+ if ((Control_config[i].key_id >= 0) && (Control_config[i].key_id == Control_config[j].key_id)) {
+ // If one of the conflicting keys is disabled, then this silently clears
+ // the disabled key to prevent the conflict; a conflict is recorded only
+ // if both keys are enabled
+
+ if (Control_config[i].disabled)
+ Control_config[i].key_id = (short) -1;
+ else if (Control_config[j].disabled)
+ Control_config[j].key_id = (short) -1;
+ else {
  Conflicts[i].key = j;
  Conflicts[j].key = i;
  Conflicts_tabs[ Control_config[i].tab ] = 1;
@@ -423,10 +428,18 @@
  }
 
  if ((Control_config[i].joy_id >= 0) && (Control_config[i].joy_id == Control_config[j].joy_id)) {
- Conflicts[i].joy = j;
- Conflicts[j].joy = i;
- Conflicts_tabs[ Control_config[i].tab ] = 1;
- Conflicts_tabs[ Control_config[j].tab ] = 1;
+ // Same as above
+
+ if (Control_config[i].disabled)
+ Control_config[i].joy_id = (short) -1;
+ else if (Control_config[j].disabled)
+ Control_config[j].joy_id = (short) -1;
+ else {
+ Conflicts[i].joy = j;
+ Conflicts[j].joy = i;
+ Conflicts_tabs[ Control_config[i].tab ] = 1;
+ Conflicts_tabs[ Control_config[j].tab ] = 1;
+ }
  }
  }
  }
@@ -454,7 +467,7 @@
 
  Num_cc_lines = y = z = 0;
  while (z < CCFG_MAX) {
- if (Control_config[z].tab == Tab) {
+ if (Control_config[z].tab == Tab && !Control_config[z].disabled) {
  k = Control_config[z].key_id;
  j = Control_config[z].joy_id;
 
@@ -2117,6 +2130,9 @@
  return 0;
  }
 
+ if (Control_config[id].disabled)
+ return 0;
+
  if (Control_config[id].type == CC_TYPE_CONTINUOUS) {
  if (joy_down(Control_config[id].joy_id) || joy_down_count(Control_config[id].joy_id, 1)) {
  control_used(id);
Index: controlconfig/controlsconfig.h
===================================================================
--- controlconfig/controlsconfig.h (revision 9143)
+++ controlconfig/controlsconfig.h (working copy)
@@ -46,6 +46,7 @@
  short key_id;  // actual key bound to action
  short joy_id;  // joystick button bound to action
  int used; // has control been used yet in mission?  If so, this is the timestamp
+ bool disabled; // whether this action should be available at all
 } config_item;
 
 /**
Index: controlconfig/controlsconfigcommon.cpp
===================================================================
--- controlconfig/controlsconfigcommon.cpp (revision 9143)
+++ controlconfig/controlsconfigcommon.cpp (working copy)
@@ -625,6 +625,14 @@
                 if (optional_string("$Type:"))
                 {stuff_string(szTempBuffer, F_NAME, iBufferLength);
                  r_ccConfig.type = (char)mEnumNameToVal[szTempBuffer];}
+                 
+                 if (optional_string("+Disable")) {
+                    r_ccConfig.key_id = (short)-1;
+                    r_ccConfig.joy_id = (short)-1;
+                   
+                    r_ccConfig.disabled = true;
+                } else
+                    r_ccConfig.disabled = false;
                 
                 // Nerf the buffer now.
                 szTempBuffer[0] = '\0';
Index: fred2/sexp_tree.cpp
===================================================================
--- fred2/sexp_tree.cpp (revision 9143)
+++ fred2/sexp_tree.cpp (working copy)
@@ -5476,7 +5476,7 @@
  sexp_list_item head;
 
  for (i=0; i<CCFG_MAX; i++) {
- if (Control_config[i].key_default > 0) {
+ if (Control_config[i].key_default > 0 && !Control_config[i].disabled) {
  head.add_data_dup(textify_scancode(Control_config[i].key_default));
  }
  }
Index: io/keycontrol.cpp
===================================================================
--- io/keycontrol.cpp (revision 9143)
+++ io/keycontrol.cpp (working copy)
@@ -2040,6 +2040,9 @@
 {
  Assert(n >= 0);
 
+ if (Control_config[n].disabled)
+ return 0;
+
  // check if the button has been set to be ignored by a SEXP
  if (Ignored_keys[n]) {
  if (Ignored_keys[n] > 0) {

Example tabling:

Code: [Select]

#ControlConfigOverride

$Bind Name: Cycle Forward Primary Weapon
    +Disable
   
#End


[chief1983]

Might need a FRED patch to prevent mission designers from requiring a disabled key to be pressed on an event.  And log when a mission attempts to require one.

[z64555]

One thing to codereview is whether it's acceptable to not initialize the disabled bool member to false; I didn't, because I didn't find any suitable place to do it.

What's wrong with initializing it inside the struct config_item declaration?

I mean, it's just a plain ol' struct without any constructors (per se) or methods... so if any function where to initialize it, it would have to be right at each and every function that uses it.

[Edit] Derp: C++ no like assignments in struct declarations anymore. 

[zookeeper]

Might need a FRED patch to prevent mission designers from requiring a disabled key to be pressed on an event.  And log when a mission attempts to require one.

That doesn't seem to be necessary: if you, say, have an event which triggers on key-pressed, then the keypress does register correctly and thus the event does trigger, but the associated action won't occur if it's been disabled.

EDIT: Also with z64555's help I figured out the initialization bit, updated patch in the first post.

[chief1983]

I think it's more complicated than that.  If the mission editor tries to have me press "Afterburner", but afterburner is a disabled command, it probably wouldn't (or at least mightn't) be bound to anything.  Then there's no keypress to register.

[zookeeper]

I think it's more complicated than that.  If the mission editor tries to have me press "Afterburner", but afterburner is a disabled command, it probably wouldn't (or at least mightn't) be bound to anything.  Then there's no keypress to register.

Oh. Yeah, I guess that makes sense. I'll see what I can do.

Also, as discussed in IRC, keybind conflict resolution is another thing I need to tweak a little bit. Currently you could have a disabled bind still have a key assigned to it and cause a conflict when you're remapping another bind to the same key.

[zookeeper]

Updated the patch in the first post again. I think it should now prevent FRED from displaying disabled actions as possible values for key-pressed and such, and the initialization for the disabled flag is probably finally being done as it should. Also disabled actions now can't cause conflicts when mapping another action to the disabled action's default key, but I'm still working on the conflicts which could arise when using a pilot which contains an existing mapping for a disabled action.

[chief1983]

As I mentioned, I think the least invasive way to do that would be to silently see if a conflicted keybind action is disabled, and if so, simply clear it then so the conflict doesn't happen.  Only mucks with the pilot file when the user starts changing controls then.

[zookeeper]

Patch updated again. Now, when such a conflict is detected, it gets silently resolved by clearing the disabled action . However, printing debug messages in the rare case when a mission checks for a disabled action with key-pressed ended up being rather difficult, so rather than write some kind of a half-assed hack to do it, I'd rather just leave it out.

Any further comments?