Refactor GUI

[crkellen]

---

This is a refactor of:

• Identify Scroll
• Remove Curse Scroll
• Repair Scroll
• Enchant Weapon Scroll
• Enchant Armor Scroll
• Identify Spell
• Remove Curse Spell
• Appraising GUI

This list may be expanded, pending further auditing.
This PR also contains solutions to Issues #48 and #140.
This PR also contains changes to the functionality of each of the Scrolls listed above. These changes will be outlined below.

Note: An additional change was made to an image file. I modified identifyGUI.png to itemModifyingGUI.png

---

Summary:

The current code base will be referred to as the "Current System" and this pull request's code base will be referred to as the "New System.

In the Current System, there is a GUI for the Identify Spell and Remove Curse Spell (however, see #48). All Scrolls lack any form of GUI however, and in fact, are not random. For instance, a Scroll of Enchant Armor will target the first Item in the list that is valid, which is often the Helm as it is checked first. This means that the helm will always be enchanted, unless the Player removes all other equipment other than the Item they mean to enchant. In addition to this, issues could arise with Scrolls of Repair choosing to repair an Artifact weapon (which cannot lose durability).
The code base is also scattered and non-cohesive in the Current System. Remove Curse had a few variables which were unused, and Identify had several major differences from Remove Curse. Along with this, checks for if the GUI was open had to be preformed for every individual GUI. Not to mention, there was an extremely large amount of global variables, including half of the variables that were not used outside of their local functions. In short, a refactor was entirely necessary to clean, update, and improve functionality of the code.

ItemModifyingGUI

The New System aims to fix the above listed issues by wrapping each of the Item Modifying GUI's (Identify, Remove Curse, Repair, Enchant Weapon, Enchant Armor) into a singular class, 'ItemModifyingGUI'. I have attempted my best to follow proper coding conventions and the style guidelines while implementing this refactor. In any place that you want to open the GUI, you simply need to call itemModifyingGUI->openItemModifyingGUI(Uint8 GUIType, Item* const scrollUsed);. In the case of a Spell, scrollUsed is nullptr. Rarely is there a time when anything else must be handled. The class itself handles all of the drawing, the initialization, the inventory, the controller/button code, the processing of items, and the network updates.
In cases where the game must see if a GUI is open, in order to close it, itemModifyingGUI->isActive() can be called, which returns either true or false. All functions are documents with clear comments, along with all of the variables names and code within functions utilizing clear and easy to read/understand conventions. Overall, the code should be clearly understood from little more than a glance.
ItemModifyingGUI is accessed through a global object of 'game.cpp' called itemModifyingGUI. This object is constructed at initialization, and deconstructed at deinitialization.

AppraisalGUI

In the Current System, the Appraisal GUI (the window in the top left that shows the progress on appraising an Item), is tied into the Identify GUI, although the two share nothing in common. This refactor extracts the Appraisal GUI from the Identify GUI, and places it into it's own, separate class 'AppraisalGUI'. AppraisalGUI will work identically to ItemModifyingGUI, but will only handle appraisal code. Appraisal GUI has been fully extracted, removing a number of global variables and refactoring a long list of usages spread across the Current System's code base.

New Scroll Functionality

Scrolls have had their functionality modified, to better make use of the Beatitude value of Items. A list of the new functionality will be added, pending approval.

[addictgamer]

Added the bug label since it fixes a couple bugs in addition to refactoring.

[crkellen]

In addition it changes the functionality of the Scrolls. null

[lheckemann]

Without meaning to sound hostile — do the 3000 additional lines this PR introduces actually improve maintainability? As far as I understand, this, being a refactor, is not intended to introduce new functionality but should improve maintainability. In my experience less code leads to more maintainability :)

EDIT: note that this is a first impression. I will have a look at the actual contents, and that may convince me that this is actually a really good idea!

[crkellen]

@lheckemann There may be places where code could be reduced, I did strive to remove as much as possible.
However, a significant portion of that new code is due to the new functionality of the scrolls.

There is a significant amount of use cases to be covered. In addition, I attempted my best to design this to be extremely readable and logically structured. If you have any issues with the code. Please comment at the location and I can take a look.

[lheckemann]

Yeah, I'm reading it just now. Hard to work out where to start since it's so huge (although I'm guilty of the exact same thing, if not worse, in #86!)…

[crkellen]

Don't be afraid to be critical. I'd prefer to have well documented, well structured, and well performing code over having something that causes issues down the line. Originally it was simply a refactor, however, I was tasked with changing functionality of scrolls, so it also became an enhancement.

[lheckemann]

Impressions so far (I'll be posting them a bit piecemeal, given past experiences of writing long essays in github comments only to accidentally lose them to a browser quit…)

• Redundant yet inconsistent namespacing; you named some of the members of ItemModifyingGUI with the class name in them. This is redundant and makes it harder to read; if you want to be more explicit about access to class members, I'd suggest using this->image (not sure how @addictgamer feels about this?) rather than itemModifyingGUI_IMG. Same for the methods prefixed with the class name. At the same time you don't use this naming convention for the boolean members (bIsActive, bIsCursed, …).
• Uint8 for determining type; an enum would be much better for this, as it's semantically clearer and type safe (to the extent that C/C++ get type safety…)
• Inconsistent resource acquisition/release. Why does FreeImage exist, particularly as a public method? Do we ever have a reason to call it from anywhere other than the destructor, and wouldn't it be better to just inline it into the destructor since it's just a single function call?
• Behaviour for each scroll type integrated into selection GUI class. From what I can tell, it would be neater to have the only responsibility of the class be to provide an item selection, and pass it to some other component which would handle the magical effect. I'll elaborate a bit on this later.

[crkellen]

Redundant naming of variables

I actually remember reviewing this myself. I'll go back and take another look, as I do remember there being a reason I kept certain things with a prefixed name... I believe the _IMG one was due to it being public originally. I'll check. I do know that addictgamer does not want the usage of this at all (at least to my understanding).

Uint8 for determining type

I believe this was due to legacy reasons. When converting, I made sure to have the same functionality. I will review this, but the logic may dictate using that value somewhere. If that's the case, having an enum that has to be constantly converted would be more of an issue i'd think.

FreeImage issues

There was a specific reason I created this function, and the reason escapes me. As with the others, I will review.

[lheckemann]

So we have the following, each of which require an item selection:

• Identify Spell
• Identify Scroll
• Repair Scroll
• Enchant Weapon Scroll
• Enchant Armor Scroll
• Remove Curse Spell
• Remove Curse Scroll

The GUI for the item selection would look pretty much the same between them. The key differences in functionality are which items are available for selection and what will happen to the selected item.

Which items are available is simple: a list of items, so it doesn't need to care about the inventory!

What will happen once the item has been selected has two parts:

• The choice-specific part. Apply a change to the item that was selected: std::function<void(Item&)>
• The item-agnostic part. Apply a change to the inventory over all. But the item-modifying GUI doesn't really need to know anything about what's happening here, or give it any information; so it would just be a void function: std::function<void>. I think it makes more sense to unify this with the choice-specific part though — again, the selection GUI shouldn't really care.

So in order to reduce the amount of responsibility that the item-modifying GUI has over what happens to the items, the calls to open it should probably look more like

itemModifyingGUI->openGUI(
    std::list<Item&> choices,
    std::function<void(Item&)> applySelected,
    std::string title // For good measure :)
)

choices could be set up using a helper function std::list<Item&> filterInventory(std::function<bool(const Item&>) which could be called like filterInventory([](const Item& item){ return item.beatitude < 0; }) in the case of remove curse.

applySelected would probably be a top-level function named removeCursefromItem, identifyItem, enchantItem, etc…

This should make implementing the scrolls and spells a lot simpler. For now I need to go to bed though, and I'll come back in the morning and see if the stuff I've written makes any sense. Also, I don't know if there's some other existing communications channel but I think it would be good to have a more instant way of discussing stuff; I've joined #barony on IRC at freenode.net (I'm sphalerite) and will probably be hanging around there for quite a while if you want to join too :)

[crkellen]

@lheckemann I will read what you have posted and write my own thoughts on the topic after fully understanding it. The main location for speaking is the Barony Discord server located here.

[lheckemann]

Don't be afraid to be critical.

So after a couple of hours of reviewing, my unrestrained criticism is that the changes you've made are a bad idea, even to the point of being a worse starting point for refactoring than what we had before.

I see two main things that would improve it significantly:

• Better separation of responsibilities. The item choice GUI should be there to choose an item, not to decide what to do with it once chosen. Nor should it do anything to items that weren't chosen.

• Less duplication. Code should never be copied and pasted. If there's enough commonality between two things that need to be done, it should instead be moved and parameterised. One key example of this is how items are filtered — the loops are, by and large, identical except for the criteria of inclusion/exclusion. The size of the loops results partly from the lack of abstraction over the data structure — an artifact of the game's origins in C code, which really don't need to be preserved over and over again. The drawing code is similarly duplicated across the different GUI types.

I don't want to look at this PR anymore. If you're willing to start over we can have a chat about that and work together on it in order to hopefully produce something nicer; if not, I guess what happens is up to @addictgamer.

Sorry for the harshness of my criticism, but the bottom line of my opinion is that Barony really doesn't need any more of what makes Barony hard to work with — and this is exactly the kind of code that does this. I do highly appreciate your other contributions and hope you won't be too put out by my feedback.

[crkellen]

I ... hope you won't be too put out by my feedback.

Of course not! As I've said before, I'd rather my code be good than to be bad. I'm new to all of this and I would love to learn more and keep growing. Sure, I may have thought it was good, and I put a lot of time and effort into researching and designing how to do this, but that in no way makes it perfect.

I have been waiting for the time when someone would read and give me a proper review on my first, rather large, refactor. Now it's simply up to me to fix it. I'll have to think about this and properly understand your criticism and advice before moving forward, but for now I have a starting point.

Your criticism is of my code, and not of my person, so there's no way I can take offense. Please continue to criticism openly and freely.

EDIT: Oh and if you would like to hear some of the reasoning on why I implemented it the way I did, please contact me on Discord. I'm always set to offline, but if you message me, I will respond when I see it.

[lheckemann]

Re discord: I don't need another proprietary communication service in my life. You can, however, find me on:

• IRC; I'm sphalerite on freenode and a few other networks
• Matrix, I'm @sphalerite:matrix.org
• Mumble at lugn.sphalerite.org or another server of your choice (with prior arrangement, I'm not usually online)
• Telegram (semi-proprietary :( ) https://telegram.me/sphalerite
• Steam (very proprietary D:) as sphalerite

[addictgamer]

I do know that addictgamer does not want the usage of this

this should not be used liberally and without meaningful effect.
Where this is beneficial and improves code readability and maintainability, it is ok. Though if you have to indicate a variable is not a local or global variable but a class member, then there's something wrong with your design (read: there is. Barony's previous C paradigm makes heavy use of the global namespace, and classes were added posthumously. So there are sections in the code that can use this->, and there are even sections that have to)

Uint8 for determining type; an enum would be much better for this, as it's semantically clearer and type safe (to the extent that C/C++ get type safety…)

Enums might be a good idea for the long run. There are many areas that currently use constants to name variable states.

[crkellen]

Ah, very good. I remember seeing you state that you wanted `this` removed from certain commits, and I think the style guidelines mentions them as well. Thanks for the clarification. null

[lheckemann]

Though if you have to indicate a variable is not a local or global variable but a class member, then there's something wrong with your design

I don't know, I find having a visual distinction between members (state) and locals (intermediate values) very useful myself, regardless of the design. But you do you :)

[addictgamer]

Ah, very good. I remember seeing you state that you wanted this removed from certain commits, and I think the style guidelines mentions them as well.

Yes, there were certain commits that may or may not have been unnecessarily overusing this...

[crkellen]

I will be re-reviewing this sometime hopefully soon.
A good amount of time has passed and I have a much better concept of things and what I should do. Along with that I've been reading Clean Code so I'll be taking that into consideration as well.

Due to my new schedule, I likely wont get to it this weekend, but perhaps the next.
If there are any new comments or requests, feel free to describe them here so I can evaluate them with the rest of it when I start this again.