Fix for some stupid programming

[sli]

Removes the need to use "[NONE]" in the attack equipment slots, opting for just unsetting the option. Why it was written to require "[NONE]" in the first place still eludes me. This is the kind of thing a programmer fresh out of college does: reinvent the wheel in a slower, stupider form than just checking for an empty string. If there was a specific reason that "[NONE]" was required, then it needs to be fixed based around this patch because that's completely asinine.

Code: Select all

Index: src/AI.pm
===================================================================
--- src/AI.pm	(revision 6335)
+++ src/AI.pm	(working copy)
@@ -730,7 +730,7 @@
 				message TF("Encounter Monster : %s\n", $monsters{$ID}{'name'});
 				if ($config{"autoSwitch_$i"."_rightHand"}) {
 
-					if ($config{"autoSwitch_$i"."_rightHand"} eq "[NONE]" && $char->{equipment}{'rightHand'}) {
+					if ($config{"autoSwitch_$i"."_rightHand"} eq "" && $char->{equipment}{'rightHand'}) {
 						$Runeq = 1;
 						message TF("Auto UnEquiping [R]: %s\n", $config{"autoSwitch_$i"."_rightHand"}), "equip";
 						$char->{equipment}{'rightHand'}->unequip();
@@ -744,7 +744,7 @@
 				}
 
 				if ($config{"autoSwitch_${i}_leftHand"}) {
-					if ($config{"autoSwitch_${i}_leftHand"} eq "[NONE]" && $char->{equipment}{leftHand}) {
+					if ($config{"autoSwitch_${i}_leftHand"} eq "" && $char->{equipment}{leftHand}) {
 						if (!($Runeq && $char->{equipment}{rightHand} == $char->{equipment}{leftHand})) {
 							message TF("Auto UnEquiping [L]: %s\n", $config{"autoSwitch_${i}_rightHand"}), "equip";
 							$char->{equipment}{leftHand}->unequip();
@@ -798,7 +798,7 @@
 
 		if ($config{"autoSwitch_default_rightHand"}) {
 
-			if ($config{"autoSwitch_default_rightHand"} eq "[NONE]" && $char->{equipment}{'rightHand'}) {
+			if ($config{"autoSwitch_default_rightHand"} eq "" && $char->{equipment}{'rightHand'}) {
 				$Runeq = 1;
 				message TF("Auto UnEquiping [R]: %s\n", $config{"autoSwitch_default_rightHand"}), "equip";
 				$char->{equipment}{'rightHand'}->unequip();
@@ -812,7 +812,7 @@
 		}
 
 		if ($config{"autoSwitch_default_leftHand"}) {
-			if ($config{"autoSwitch_default_leftHand"} eq "[NONE]" && $char->{equipment}{'leftHand'}) {
+			if ($config{"autoSwitch_default_leftHand"} eq "" && $char->{equipment}{'leftHand'}) {
 				if (!($Runeq && $char->{equipment}{'rightHand'} == $char->{equipment}{'leftHand'})) {
 					message TF("Auto UnEquiping [L]: %s\n", $config{"autoSwitch_default_leftHand"}), "equip";
 					$char->{equipment}{'leftHand'}->unequip();

[freegoods]

[NONE] is the way to make kore unequip its weapon (E.g.: for killing plants). And an empty string does nothing.
So, when you apply this patch kore will unequip its weapon every time it uses a skill or meets some monster if autoSwitch is not set for every case. Have you tested the patch?

[sli]

I know what [NONE] is used for, I'm just saying it's a really ugly implementation. When Kore goes from one autoSwitch block with a weapon, to another with nothing, it should unequip the weapon. Because it's specified that no weapon is being switched to, so inference should lie on just that: no weapon. This makes it much more congruent with the rest of the config file.

I didn't test it, because I don't play RO anymore, but I wanted to point out that this certainly should be changed.

[freegoods]

I thought that if another block has no weapon mentioned, nothing should be done by kore. Why put weapon name in every block?

Am I not right?

[Chontrad]

Agreed with freegoods! But why don't you change the [NONE] to a simpler word? Like NO,OFF or stripe(-)? This word is kinda frustating when the time is come to you to modify your config file.

[Motivus]

I thought that if another block has no weapon mentioned, nothing should be done by kore. Why put weapon name in every block?

Am I not right?

That's how it is supposed to work, because it's only on certain occasions you want to switch weapons. A lot of the time you just want to keep on what you currently have equipped.

[sli]

I thought that if another block has no weapon mentioned, nothing should be done by kore. Why put weapon name in every block?

Am I not right?

Isn't there a setting for default equipment? That combined with smarter equip blocks would make it work no problem. Realistically, how many weapons is someone going to carry? One for each element? Maybe a special shield if you use one, but since there's no dual wielding, I don't think specifying weapons each time is so bad. Or possibly add a switch setting for inheriting (ie, inherit [0|1]) the defaults instead of ignoring the missing options in the block (as currently is done). Using an arbitrary string is just bad form, that's all I'm saying.

[kali]

It would be bad form, I'd agree. But unless we can get a working implementation of something that's "elegant" yet still functional (ideally better than current, if not the same) we're stuck with this implementation.

Admittedly, with the very few active developers we currently have, "bad-form" designs such as these (and these aren't the only bad-form design you'd see in openkore), as long as they are functional, are of lesser priority than outstanding bugs (which I'll have to thank the contributors for - may you inspire others to contribute as well).

Of course, this doesn't mean we're being closed-minded about the issue and simply resisting change. I guess if we thresh out the pros and cons for this a bit further we can arrive at a better plan of action

[sli]

Well yeah, I figured as much. I posted that patch as a concept, not an actual fix.