BoolClub / ProjectCrusade

First project of RHS Software Development Club
0 stars 0 forks source link

Fork stable and ready for merge. #25

Closed AnonymousNix closed 8 years ago

AnonymousNix commented 8 years ago

Inventory rebuilt, item behaviors partially complete (waiting on player stats, etc.) , sprint function, etc. This is stable enough to merge to master, but test it yourselves first.

EDIT: Merging directly to master after unannounced commit to original dev branch made merging impossible. Bug fix from dev was committed to fork. This must be merged instead of the fluid fix or the Master will need to be manually rewritten.

christiancosgrove commented 8 years ago

@AnonymousNix A question before we go through with this merge (I was just in the process of resolving merge conflicts)--

We need to consider how we’re implementing items here. The work is great, but I believe the way we had implemented items before (creating an individual class for each item and using polymorphism to reduce duplicated code) will lead to more readable and less duplicative code in the future. A lot of the changes you suggested (such as including a max stack value with each item) are completely valid, but I just think the foundation of the existing system will be more extensible.

We can write an abstract Item class, an abstract ConsumableItem/FoodItem class, an abstract Sword class, etc. All actual items would derive from these templates, allowing us to reduce the amount of duplicated functionality and better organize the item system. Just as we currently have, we would still have a general ItemType enum that allows us to access a given item's sprite sheet source rectangle. It would also allow us to easily organize the inventory, e.g. by categorizing the inventory by item type (using C#'s is keyword). This system provides mostly organizational benefits; if we plan on adding a significant number of items, it would help us manage them.

If you have any criticisms, I’m perfectly open to them. Your approach may be preferable because of vtable overhead, but I don’t think performance is much of a concern for an inventory system of this scale (if there even is any difference).

Christian

AnonymousNix commented 8 years ago

(Pardon any lazyness in this reply, it's 12 o'clock and long story short I'm in Princeton.) One of the main purposes behind my approach is to make the items as flexible and easily modified as possible. Custom items can be made by the game itself without any action by a programmer. The item's actual behaviors are defined by a list of tuples and these lists interpreted by the behavior handler. While a class for each item ensures the best expandability of complex behaviors, if the behavior handler in my technique is designed well, it would allow easy creation of custom behaviors. For example, A sword that uses attack animation X, but with effect Y, and spell action Z because why not, spellswords are cool. This could be made very easily, and even made by the player in game via a forge, enchanter, etc. Existing items could also be tweaked without creating a new object via similar methods. It may be complicated to write at first, but after careful planning and maybe a few revisions, I believe we'll have a very powerful peice of code on our hands. I'm glad you see the promise in my approach, it's not too often something good is said about my code, as I tend to make monstrosities from pre-optimization (aka the root of all evil). The memory accesses and writes need to be more efficient, which can be accomplished by lightening the inventory code. That's all I can really think of to say at the moment... I may get back to you tomorrow with more info as I probably left some important details out.

christiancosgrove commented 8 years ago

I think this approach has its merits, especially if we can add a few things. Would we be able to use delegates/events for behaviors instead of a hardcoded switch statements? (This is the precise purpose of events in C#). In other words, you would create a custom delegate and a corresponding event for item use. Events allow arbitrarily many listeners, so your idea of having many behaviors would continue to work.

Another thing we should consider: not hardcoding items. If we go with your system, I think we should not hardcode item properties (it's easy enough to integrate with this current system). We could have an XML file that defines every item and its respective properties (including a list of all of its behaviors). This would allow for non-programmers to add new items to the game (XML is pretty self-explanatory), and although the behaviors themselves would be hard-coded, this would allow for a lot of creativity (if a spellsword is possible, think about how many other portmanteau-swords will be possible...). It would also allow modders (albeit minimally) to add new content to the game without compromising the source.

Otherwise, however, I'm still advocating for the inheritance system, as it leads to the natural organization of items (which, depending on their subtypes, would have explicit unique properties) and better, clearer, interoperability with the game code. You can sort all items based on whether they're FoodItems, you can access an FoodItem's heal value by casting it as a FoodItem and simply access item.HealValue, etc.

Let me know what you think about these changes.

Christian

christiancosgrove commented 8 years ago

Also, on an unrelated note, we decided in last game's meeting to return to procedural level generation (we would still design rooms in Tiled). This game style (probably less content-heavy than an RPG but still with some RPG elements) is important to consider when designing an inventory. I don't know if a Minecraft-like inventory system is suitable to this type of gameplay.

AnonymousNix commented 8 years ago

The changes you suggested to the system are absolutely acceptable, and we should definitely make them. Moving the item properties to an xml would be preferable to the current solution as it would (as you said) give easier access to the item database, and I'm completely open to changes in the behavior handling system. The whole switch statement part was completely hacked together anyway, so I would love to replace it with a more thought out solution. As for the inventory layout, I'd also agree. The minecraft style layout we have currently is not even close to what we should be aiming for. I was planning on proposing an equipment-slot and inventory style menu (just like what you'd normally see in an RPG,) and changing the consumables to a "right-click item -> click use" style usage with one or two assignable quick-use hotkey slots.

AnonymousNix commented 8 years ago

I'm changing the pull request to go to the dev branch now.