Spell remains available when characteristic decrease

[ghost]

I loot an item which is able to upgrade my Mental characteristic. It made me able to use a new spell, so I send it on shortcut bar to try it. Then I removed that object so I should be no longer able to cast that spell, but using the shortcut still works.

[ghost]

I looked into your code to try to fix that. I may have a solution, but it would imply modifications in the way you designed the relationship between PowerManager, MenuPower and StatBlock.

First, a question (if the reply to it is false, then you can forgot what is following it): Is StatBlock some kind of representation for characters for rules? By characters, I mean the Hero, Monsters, but also NPC (maybe one day you will want to add things to allow them to change their mind depending on Hero's actions).

I think you should (but, I do not know perfectly your design so I might be totally wrong) insert a list of power in that class, and when a character try to cast something (in a "bool StatBlock::cast(int power_index);" method) ask to the power if it can be cast. Obviously, it means that Power become a true class, and become able to alter the StatBlock which try to cast it.

This solution means that "int MenuPowers::click(Point mouse)" will just do something like: "return stats->cast(index)" where index is the index of the spell.

If you agree with such a solution, I could implement it. If not, I will continue to try to understand better your design to try to fix that "cheatbug" ;) While I am on this: do you have some design explanations somewhere? I searched the wiki and found nothing about that.

And the last question, which is NOT RELATED but I prefer to ask : can the lacks of balancing in the game be considered a bug? I have some details to discuss about them, but I am not sure if it is the right place to do that.

[pennomi]

bmorel, It seems like a power doesn't actually check if its prerequisites are filled before it activates. I think the easiest way to fix this problem is to add this check in when the player uses a power.

As far as the "power balancing" issues are concerned, I think Clint has said before that if someone creates a new mod that balances the powers, and does it well, he'd merge that in. If you'd like some help creating a mod (it doesn't require any code - just changing the data files), I'd be happy to give you some tips or to create the mod with you. (I've considered doing that myself for a little while now.)

[ghost]

I was more interested in resolving that bug, but didn't want to spend time if my work has no chance to be taken into account. In fact, I have some ideas to solve it, but it involve a better separation between game interface and game mechanics, first, and then it need some changes which will not have influences into configuration files, but will change relations between classes. And it is an idea that I can realize. I don't fear the code: I love the code ;)

In term of mod changes, I think power balancing is really weird too, and have some idea to balance it: archer's skill and weapons are from far the most powerful of all. Or their skills are not in the good order maybe: pin is one of the first ones, and it is the most powerful skill of all. Then, their weapons have unlimited weapons, unlimited distance, and deal same damages than mage's and warriors one. Mages have a lack of health points, and warriors can not shoot at distance, so the difference between them is not important, but archers have a huge distance, and it make them virtually invulnerable. warriors are the weakest people. They does not have enough health points and does not deal enough damages to counter balance the lack of distance of their attacks.

I am considering to modifying the mod too, but I does not know if modifications into have take place here or not, it is why I asked about that.

[pennomi]

bmorel, good to know you're not afraid of the code. (We need more people like that coding on Flare!) ;) Honestly, I think this bug would probably be as easy as a one-line fix. Even if it was fairly difficult, it certainly wouldn't need a large restructuring of the classes. Keep in mind Clint is more likely to accept a small patch than a large one.

As far as the balancing changes you mentioned, all of those tweaks can be made by only changing the data files, except for giving the warrior more health. By changing just the data files, you can modify damage percentages, add cooldowns to powers, increase mana costs, change their orders in the skill tree, or you can even go so far as to give archers limited ammo. If you are interested in doing changes like this, just let me know, as I have a lot of experience with doing just that.

[ghost]

I can understand that clint will not easily accept huge patchs, and making a huge path was not my intent. But the possible fix I have seen is far from the "one line patch". It is possible to fix that with a short modification. Something like 20 lines, I guess. But such a fix will just work for a time. The problem, as far as I remember, is that the "buttonbar" own the skills, but it does not have the informations about character. So, a call is made to a "powermanager", which will use the address of hero's variables structure and do the work to remove mana if he have enough mana. But "characteristics" are not in that structure, and the check for "characteristics" when adding a power to the "buttonbar" is made by menus. It could seem to not be a problem, but in term of design, let the UI manipulating datas from other objects is a problem: it make things harder to understand for newcommers (currently, it is ok because code base is small) and changes made on one class affect many others. And classes were invented to be easy to reuse. I don't know if I was clear enough or not...

To conclude, I was asking if modifying some classes relations was acceptable or not, to know if go to the lightweight, but dirty solution, or a bigger changes, but with cleaner result. Because bigger changes are not always the longer to implement or maintain. By example, powers, which are currently a structure, should be the only responsible about being triggered or not. So it could be logical to put the control in that structure (making it a class in fact) with a method using a reference on the character which try to trigger the power. Then, the power can check if it's owner is able to throw it or not, and if yes, it is able to remove it's cost to the mana pool. Such design have some side effects: the buttonbar is no longer responsible of knowing is a power can be used by the player: modifications in players characteristics is possible, allowing a temporary debuff to disable skills, by example. potential modification of how the player is controlled will not affect the power check when monsters cast spell, they do it in the same way as the player, because they are, as the player, characters. So the code which manage that will be identical, and in one place: easier to debug

For the stuff I discussed about balancing, I know everything is modifiable in mods. But I just asked if it was relevant to send an issue or not for that, because flare is theoretically just the game engine, and I am not sure if issues about the game take place here. But I have to admit, I didn't tried to modify anything in mods.

[daniel-santos]

Well, ideally you want to decouple the user interface components from the game logic. You can achieve partial decoupling by encapsulating the game logic functions of other classes (StatBar, PowerManager, Avatar, etc.) and having the MenuActionBar call methods of those classes instead of examining the data of foreign objects.

However, to fully decouple, you should have your UI components know nothing about game data at all. Instead, you have a presentation manger that interacts with both the game data and UI components to supply the UI components with the information it obtains from the game objects & if input comes from UI components, it passes that information along as needed. This is known as the Mediator design pattern in the GOF book (http://en.wikipedia.org/wiki/Design_Patterns).

[pennomi]

I talked with Clint about this. Here's how he says it should be solved:

"Maybe give PowerManager a function: inputs are a power ID and a statblock, and PowerManager says if the minimum reqs are met. Also give ActionBar a function that checks the current action bar and removes any that are no longer useable (or at least, disables them?). Finally, inside GameStatePlay::checkEquipmentChange(), if there was an equipment change, call the ActionBar function that checks all the power slots."

Should be fairly easy.

[daniel-santos]

Well, here's my patch set. Basically, I prefer to encapsulate wherever possible, so I'll let the Power say what it's minimum required discipline level is and the StatBlock tell you if it can use a power. I'll copy & paste it here, but I know github will format it funny. I haven't figured out how you're supposed to do patches in github yet.

From a232d2a1f1501a6295df04dddffb03c83d36a682 Mon Sep 17 00:00:00 2001 From: Daniel Santos daniel.santos@pobox.com Date: Thu, 19 Apr 2012 03:29:13 -0500 Subject: Grey out & disallow unusable powers. MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------1.7.3.4"

This is a multi-part message in MIME format. --------------1.7.3.4 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit

Greys out powers on action bar that cannot be used due to:

• required weapon not euipped
• lacking required discipline -- such powers can be put in the actionbar when using an item that increases an attribute (e.g., Scathelocke's Spellbook), and the item is later removed, dropping the discipline below the required value
• character being stunned
• character being dead
• character not having cleaned their room or done their homework

Unfortunately, there are a number of places where similar checks are made. I've added a few functions to help centralize this logic, but I haven't implemented it well. Example, calling StatBlock::canUsePower() in Avatar::logic(), but I haven't removed subsequent code that makes

some of the same checks.

src/Avatar.cpp | 5 ++++- src/MenuActionBar.cpp | 11 +++++++++-- src/PowerManager.h | 8 ++++++++ src/StatBlock.cpp | 11 +++++++++++ src/StatBlock.h | 14 +++++++++----- 5 files changed, 41 insertions(+), 8 deletions(-)

--------------1.7.3.4 Content-Type: text/x-patch; name="0002-Grey-out-disallow-unusable-powers.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="0002-Grey-out-disallow-unusable-powers.patch"

diff --git a/src/Avatar.cpp b/src/Avatar.cpp index ac46e33..2a3455b 100644 --- a/src/Avatar.cpp +++ b/src/Avatar.cpp @@ -339,7 +339,10 @@ void Avatar::logic(int actionbar_power, bool restrictPowerUse) { // handle power usage if (allowed_to_use_power && actionbar_power != -1 && stats.cooldown_ticks == 0) {

target = screen_to_map(inp->mouse.x, inp->mouse.y + powers->powers[actionbar_power].aim_assist, stats.pos.x, stats.pos.y);

+

• if (!stats.canUsePower(powers->getPower(actionbar_power), actionbar_power))

•             break;

  + // check requirements if (powers->powers[actionbar_power].requires_mp > stats.mp) break; diff --git a/src/MenuActionBar.cpp b/src/MenuActionBar.cpp index b621d53..d9ceeab 100644 --- a/src/MenuActionBar.cpp +++ b/src/MenuActionBar.cpp @@ -192,8 +192,15 @@ void MenuActionBar::render() { dest.x = offset_x + (i * 32) + 64;

  if (hotkeys[i] != -1) {

• slot_enabled[i] = (hero->hero_cooldown[hotkeys[i]] == 0) && (slot_item_count[i] != 0); //see if the slot should be greyed out
• renderIcon(powers->powers[hotkeys[i]].icon, dest.x, dest.y);
• const Power *power = powers->getPower(hotkeys[i]);
• slot_enabled[i] = (hero->hero_cooldown[hotkeys[i]] == 0)
• && (slot_item_count[i] != 0)
• && !hero->stun_duration
• && hero->alive
• && hero->canUsePower(power, hotkeys[i]); //see if the slot should be greyed out
• unsigned icon_offset = 0;/* !slot_enabled[i] ? ICON_DISABLED_OFFSET :
• (hero->activated_powerslot == i ? ICON_HIGHLIGHT_OFFSET : 0); */

•     renderIcon(power->icon + icon_offset, dest.x, dest.y);
  }
  else {
    SDL_BlitSurface(emptyslot, &src, screen, &dest);

  diff --git a/src/PowerManager.h b/src/PowerManager.h index 456a434..19089cf 100644 --- a/src/PowerManager.h +++ b/src/PowerManager.h @@ -36,6 +36,7 @@ FLARE. If not, see http://www.gnu.org/licenses/

  include

  include

  +#include

  const int POWER_COUNT = 1024; const int POWER_MAX_GFX = 64; @@ -306,6 +307,8 @@ public: void handleNewMap(MapCollision _collider); bool activate(int power_index, StatBlock src_stats, Point target); float calcTheta(int x1, int y1, int x2, int y2);

• const Power *getPower(unsigned id) {assert(id < (unsigned)POWER_COUNT); return &powers[id];}

• bool canUsePower(unsigned id) const;

  Power powers[POWER_COUNT]; std::queue<Hazard > hazards; // output; read by HazardManager @@ -318,6 +321,11 @@ public: SDL_Surface runes;

  int used_item; +

• /* Return the required stat value for the specified power. /
• static unsigned getRequiredStatValue(unsigned powerid, unsigned stat) {
• return (powerid > 19 || stat != powerid % 4) ? 0 : (powerid / 4) * 2 + 1;

• } };

  endif

  diff --git a/src/StatBlock.cpp b/src/StatBlock.cpp index 53d1258..7bcea36 100644 --- a/src/StatBlock.cpp +++ b/src/StatBlock.cpp @@ -24,6 +24,7 @@ FLARE. If not, see http://www.gnu.org/licenses/

  include "StatBlock.h"

  include "FileParser.h"

  include "SharedResources.h"

  +#include "PowerManager.h"

  using namespace std;

@@ -468,3 +469,13 @@ Renderable StatBlock::getEffectRender(int effect_type) { StatBlock::~StatBlock() { }

+bool StatBlock::canUsePower(const Power *power, unsigned powerid) const {

• return (!power->requires_mental_weapon || mental_weapon_power >= 0)
• && (!power->requires_offense_weapon || ranged_weapon_power >= 0)
• && (!power->requires_physical_weapon || melee_weapon_power >= 0)
• && mp >= power->requires_mp
• && (unsigned)physoff >= PowerManager::getRequiredStatValue(powerid, 0)
• && (unsigned)physdef >= PowerManager::getRequiredStatValue(powerid, 1)
• && (unsigned)mentoff >= PowerManager::getRequiredStatValue(powerid, 2)
• && (unsigned)mentdef >= PowerManager::getRequiredStatValue(powerid, 3); +} diff --git a/src/StatBlock.h b/src/StatBlock.h index 00e7dfc..288a1bc 100644 --- a/src/StatBlock.h +++ b/src/StatBlock.h @@ -30,6 +30,8 @@ FLARE. If not, see http://www.gnu.org/licenses/

  include

  include

+class Power; + const int STAT_EFFECT_SHIELD = 0; const int STAT_EFFECT_VENGEANCE = 1;

@@ -111,10 +113,10 @@ public: int mental_additional;

// getters for full base stats (character + additional)

• inline int get_offense() { return offense_character + offense_additional; }
• inline int get_defense() { return defense_character + defense_additional; }
• inline int get_physical() { return physical_character + physical_additional; }
• inline int get_mental() { return mental_character + mental_additional; }
• inline int get_offense() const { return offense_character + offense_additional; }
• inline int get_defense() const { return defense_character + defense_additional; }
• inline int get_physical() const { return physical_character + physical_additional; }

• inline int get_mental() const { return mental_character + mental_additional; }

  // derived stats ("disciplines") int physoff; @@ -205,7 +207,9 @@ public: int power_index[POWERSLOT_COUNT]; int power_cooldown[POWERSLOT_COUNT]; int power_ticks[POWERSLOT_COUNT];

• +

• bool canUsePower(const Power *power, unsigned powerid) const; + int melee_range; int threat_range; Point hero_pos;

--------------1.7.3.4--

[pennomi]

Daniel, the way you do patches in GitHub is by forking the repository (search around this repo for a fork button) and then pushing your changes to your fork. Then you can (through the GitHub interface) issue a pull request, which will get sent upstream to this repo.

Or I guess you could email the files to me, and I'd do it through my repo when I get home from work. (But really you should do it through your GitHub account!)

http://help.github.com/fork-a-repo/

[daniel-santos]

thanks! will post pull request now

[clintbellanger]

Fixed, thanks Daniel Santos