collinsmith / riiablo

Diablo II remade using Java and LibGDX
http://riiablo.com
Apache License 2.0
884 stars 101 forks source link

Validate Attribute maps behavior #36

Closed collinsmith closed 3 years ago

collinsmith commented 5 years ago

D2S.StatData isn't ever modified or used properly in conjunction with item stats. This data should be read into the Player entity's stat enum map. Once it's in a map, then items can append to it.

Some things I'm unsure about:

The concern for bullet 3: For things like modded strength/dexterity or stats in the save file it's not a big issue, however this becomes an issue when two items contain the same overwriteable stat and one is removed, which would remove the stat -- I can possibly just not care about this and deal with it later or put these sort of stats in a trailing array list. This would be a problem for say 2 items with a 30% chance to do some event. Technically this should be treated as 2x30% (60%) chance.

SomeFire commented 5 years ago

Technically this should be treated as 2x30% (60%) chance.

No, it is 2 different checks for the same event with 30% chance each. So, it is possible to have event twice with 9% chance (0.3*0.3=0.09) and higher chances for single event. When you work with these checks separately, you avoid issues when events are the same, but differs in numbers (e.g. 30% for critical damage x2 and 30% for critical damage x3).

collinsmith commented 5 years ago

The game already takes care of that. Different skill levels will change the properties hash, so it won't aggregate with properties even if they are the same skill/% chance/etc.

The problem is to work backwards from that, since the player entity will contain an accumulation of all stats and properties from all items/charms/etc they are carrying, I'm afraid some items will have their properties overwritten by other items (not just added). So when that item is removed, even though the player still might have another item with that property, it won't get added back correctly.

I'm going to investigate this further, it might not even be a problem, it's just a concern that it could be a problem.

collinsmith commented 5 years ago

I created Attributes which is a PropertyList that tracks modifications and maintains a reference to "base" stats. The idea is that Attributes will act as the base stats for an item or player that supports apply(PropertyList) which will generate a copy of the base stats and then add the specified PropertyList or perform the given operations on the base stats.

E.g., An armor has armorclass=10, +10 armorclass and +100 item_armor_percent. The base attributes are only 10, but then the properties are applied, first +10 is added, and then that entire value has +100% added for armorclass=40. This is still a WIP, but so far with the armors I've tested it's working. I just need to nail down which property lists are where and make copies where needed. The reason why Attributes only modifies the copy is because the base stats need to be retrievable, e.g., when the item is saved each property list and base stats should be discrete.

Some considerations I'm trying to make are that the original stats are accessible, and this should reasonably support removing stats on-the-fly, e.g., removing set bonuses if a set item is removed. Additionally, some items might not have any use for stats (e.g., attributes or fortitude runeword gives enhanced damage on an armor) -- in these cases the properties should get pushed up to the next level.

collinsmith commented 5 years ago

I made some changes to Attributes to make the different property lists more clear. I also added the ability to add property lists, which are multiplicative right now, and remainder property list which contains stats which the parent needs to handle (+damage on fortitude armor, or +skill for example -- neither of these do anything on the item itself and are meant for character)

collinsmith commented 5 years ago

I'm having an issue with the mutability of some stats when applying (some are unintentionally changing). For now I'm making probably more copies of the stats that necessary to avoid this, so I may want to explore some alternative options.

collinsmith commented 5 years ago

I had to make a small fix. op for energy and vitality are unique in that they should also modify the stat itself in addition to the opstat. vitality is unique in that it should only modify on opstat=maxhp to avoid a double increment.

collinsmith commented 4 years ago

This issue can probably be closed, but do all non-static entities need a stat map (Player, Monster, etc), i.e., does the Monster stat map function the same, or do special considerations need to be made. needs to be looked into.

collinsmith commented 3 years ago

I think not. I'm moving forward with simple adding an Attributes component to monster entities. This means that damage calculation will all use the same code.