magical_attributes poison damage

[mgamperl]

Hi,

wanted to start the discussion on how to fix the following in attribute_enhancer.ts

if (prop.s === "poisonmindam") {
        //TODO: this is wrong
        property.values[2] = Math.floor(property.values[2] / 25);
        property.values[0] = Math.floor(
          property.values[0] / property.values[2]
        );
        property.values[1] = Math.floor(
          property.values[1] / property.values[2]
        );
      }

During my usage of this api I discovered that poison damage is incorrect and that reading and writing i.e charms with poison damage results in 0 poison damage.

I have not yet took a deep dive into the problem, but I thought I'd raise it here to start the discussion.

[mgamperl]

I now analyzed the problem and the explanation at https://user.xmission.com/~trevin/DiabloIIv1.09_Magic_Properties.shtml seems to be pretty accurate on how poison damage has to be calculated. At least my tests with 8 different value/second combinations have been successful.

Right now the transformation is done in _enhanceAttributeDescription which is problematic because

• it is done twice for magical_attributes and combined_magical_attributes
• it is not calculated back when saving the values with the write method

My proposal would be to treat those transformations as other transformations of values in the respective read and write method.

export function _readMagicProperties(reader: BinaryReader, start: number, offset: number, constants: types.IConstantData) {
...
//Transform poison damage values
      if (id === 57) { //poisonmindam
        let min = Math.floor((values[0] * values[2]) / 256);
        let max = Math.floor((values[1] * values[2]) / 256);
        let seconds = Math.floor(values[2] / 25);
        values = [min, max, seconds];
      }

      magic_attributes.push({
          id: id,
          values: values,
          name: constants.magical_properties[id].s
      } as types.IMagicProperty)
...

export function _writeMagicProperties(writer: BinaryWriter, properties: types.IMagicProperty[], start: number, offset: number, constants: types.IConstantData): number {
  ....
        let num_of_properties = constants.magical_properties[property!.id].np || 1;
          
        //Transform poison damage values
        if (property.id === 57) { //poisonmindam
          let seconds = Math.ceil(property.values[2] * 25);
          let min = Math.ceil((property.values[0] / seconds) * 256);
          let max = Math.ceil((property.values[1] / seconds) * 256);
          
          property.values = [min, max, seconds];
        }
...

Pull request will be ready soon, please let me know your thoughts. thanks

[mgamperl]

@dschu012 I would like to start the discussion here to talk about #17. Especially what you have said about the background of the current implementation and that it is implemented to be compatible with https://github.com/nokka/d2s.

If I understand correctly your main goal is to keep it in a way that it can read json data produced by nokka's implementation and generate d2s files from that. It also seems that nokka's implementation does no transformation on the values at all, although I have not verified that.

What I am trying to do is to make sure that when a .d2s file from the game is read and then the resulting json is written back to a .d2s file it should basically produce the same result because that would mean that the library is consistent in itself and I see a lot of potential to have such an api as this would allow lots of interesting use cases with the upcoming d2r release. Also this process of reading / writing should be repeatable (so the json should not be "destroyed" / "altered" in the progress). I think this would be important to use this library for other use cases.

I see the following things to discuss here:

1. I would propose to use the userConfig object and add a switch like enablePropertyValueTransformations or the like to toggle between the logic that is compatible with nokka's implementation and the logic that would produce consistent results. This would allow you to use your logic as it is and people who are using the api in my context could just enable the toggle. In the future if there would be more such cases I could even imagine to have a toggle like nokkaCombatibility = true / false which would make it more consistent overall, but of course this is your decision.

2. the current transformation that is implemented to be compatible with nokka's implementation is executed once for magical_attributes and then a second time for combined_magical_attributes, this leads to some values beeing null because the division is done twice also the values in combined_magical_attributes are just wrong (even in the sense of nokka's implementation). So I would propose to parameterize the function _enhanceAttributeDescription to disable the second transformation for the combined attributes. From my point of view this should also be neccessary for your current implementation, as I can not imagine how that could be correct.

3. the function _writeMagicProperties does alter the values array that means that after writing the data to a d2s file the json can not be used anymore, I would propose to do a deep copy there so that the json object can be used again after writing. Another question if you agree on that would be: Do you support to bring lodash as a dependency to the project? I assume there will be some more deep copies neccessary and I think it would be a cleaner way than to do the JSON.parse(JSON.stringify()) thing.

4. I also discovered that for skill charges on items there is a discrepancy between the values read and the values written. It seems that it correctly reads 2/11 charges but then it writes 11/2 charges to the d2s file. The question is if this is also intentional to align it with nokka's implementation or not. I still have found more things like that and I will create different issues for those topics but just wanted to mention it here because it might help on the decision about the above mentioned userConfig toggle.

Open to discuss those things in more detail, as I really would like to contribute to that project and I'm glad that you already accepted some of my proposals.

[dschu012]

It's been awhile since I've actually touch most of this code a lot but I 100% agree w/ you on "reading / writing should be repeatable (so the json should not be "destroyed" / "altered" in the progress)".

1-2. I think the main reason I wrote the attribute_enhancer was to basically do these transformations to stats that nokkas library didn't do to put the stats in a more presentable format for consuming by some UI. Without the attribute_enhancer the library is compatible with nokkas json format. So, I feel like transformation for the psn dmg stat could happen in attribute_enhancer and just be stored on another field in IMagicProperty, like displayed_values: any[], and thinking about it attribute_enhancer really shouldn't of ever touched the fields that are used for writing back the save (values).

3. We could probably just use a variable to keep track of the index we're working with instead of doing property.values.shift() which is altering the array. i.e property.values[idx++] everytime we access it should likely work. I kind of wanted to have as little dependencies on other libraries as possible. I'm trying to keep the library and dependencies pretty small, hence all the minified naming conventions on the txt data.

4. Ya, I've seen this in the past. I imagine the reading or writing of prop.e == 3 needs to be flipped around, just never got around to fixing it.

case 3:
                  values.push((v >> 8) & 0xff);  //charges
                  values.push(v & 0xff);  //charges
                  break;

Feel free to make these changes, or suggest other alternatives. I'd like to myself but probably won't have time to spend on this library for a bit.

[mgamperl]

add 1-2 thank you for the explanation, I will take a look into this as soon as I have more time and submit my proposal.
add 3 that is what I assumed by seeing the current dependency list ;) so I will also take a look into fixing the function
add 4 yep you are right it is while reading the data and the two .push have to be flipped around, created an issue for that.

I have created the issues #20 and #21 to keep track of the two issues which are not directly related to the poison damage thing.

[mgamperl]

I also found another thing in this regard:

attributeEnhancer sorts the magical properties which might result in a different output when the json gets written to a d2s file

I can think of the following possible ways to solve this:

1. add the sort order (.soproperty) to the attributes without altering the order itself -> would introduce possible changes to your plugin as you would have to add the sorting there if this is a requirement
2. add the original order to the attributes before sorting them to have this available at the time of writing the attributes to the d2s file
3. not only use displayed_values for those kind of transformations but use something like displayed_attributes to do all kind of transformation there and keep the magical_attributes completely untouched -> would also introduce changes to your plugin but could be the most elegant solution

The reason why I'm really forcing this consistency between reading and writing is that this opens up a very easy way to create test cases which basically read an original game file (*.d2s, *.d2i, *.sss) convert it to json, convert it back to a game file and then the result can be compared with the original file. If this is guaranteed to work then this library can safely be used for all kind of purposes and it would also be very easy to have very extensive unit tests. I'm currently investigating some more differences but I think poison damage and attribute sorting are the two biggest things left so far.

Curious to hear your thoughts on that 😇

[dschu012]

Both 1 and 3 sound like good solutions to me. So whatever route you wanna take on this sounds good.

[mgamperl]

Option 3 is implemented with #31