[extension] Advanced Weapon Damage Extension

[MeAndUnique]

So looked at AWD, I use 'maxdice', so maybe CA is just spotting 'maxdice' as 'max' depending on how they are doing the match ?

I'll take a better look with some test cases in a campaign at some point, as just quick looks at code I have not updated for a while and my memory of these things. So not 100% sure on the exact interactions of what is going off with this.

I have verified that when using CA by itself in a campaign it does not conflate "maxdice" with "max".

[bratch9]

Thanks I appreciate it.

So I've looked into this some more, and its the same issue I had with the extension a long time ago.

Basically they 'store' an early version of the 'decodeDamageText' results in a local variable 'decodeResult' near the start of the damage process. This then 'holds' the pre-AWD values or any modifications for the damage stack that happen between then and the 'apply damage' section. Which they then pull back and use later in the damage process when they display the 'messagedamage' function.

So the issue is the CA extension is 'trying to avoid' doing a bunch of extra decode of damage type stuff and holding onto incorrect values that could change in the damage function call process. Causing the incorrect results to end up at the damage point.

See attached 'values.jpg', in the green box I have shown the results of AWD on its own. You can see the initial dice #13 get changed into the #4 result and been used correctly and passed to the 'messageDamage' function.

In the 'red' box with both extensions, you can see the #13 dice values been passed into the AWD side and reduced to the correct #4 values, which suddenly 'becomes #13' for the 'call messageDamage' because during the 'applyDamage' from the 5e ruleset it has to call the 'decodeDamageText' and this is when CA 'magic replaces' the damage line with the one they 'stole' at the start of the damage roll process. Basically replacing the correct valid adjusted at the correct points in the damage process values created by AWD, with the non-processed 'stashed' out of date values from the start of the damage roll.

I reported this as a problem when they started the extension, it seriously invalidates the damage roll process and all the calling systems for modifications. I do not understand 'why' they store this early value and re-inject it later in the damage. But its a total mess that needs to be replaced, and I said this to them last year ( or even the year before then. )

CA is seriously breaking the damage roll process with using the stored old decoded values, this is why it breaks. Infact it would break any valid extension that changes values in the damage process stack using the damage modification stack function provided by FG.

The CA extension needs re-writing without this 'decodeResult' local stashed value. I'm sure they have a good reason, maybe it holds some values they they need later. But they should not re-inject it back at a later stage. They should process out of the old stored value the items they need, and merge that with the 'current' version the damage stack is using at the time they want to modify.


-pete

[Leprekorn]

Thanks botha for looking at this.

[MeAndUnique]

So I've looked into this some more, and its the same issue I had with the extension a long time ago.

Basically they 'store' an early version of the 'decodeDamageText' results in a local variable 'decodeResult' near the start of the damage process. This then 'holds' the pre-AWD values or any modifications for the damage stack that happen between then and the 'apply damage' section. Which they then pull back and use later in the damage process when they display the 'messagedamage' function.

So the issue is the CA extension is 'trying to avoid' doing a bunch of extra decode of damage type stuff and holding onto incorrect values that could change in the damage function call process. Causing the incorrect results to end up at the damage point.

See attached 'values.jpg', in the green box I have shown the results of AWD on its own. You can see the initial dice #13 get changed into the #4 result and been used correctly and passed to the 'messageDamage' function.

In the 'red' box with both extensions, you can see the #13 dice values been passed into the AWD side and reduced to the correct #4 values, which suddenly 'becomes #13' for the 'call messageDamage' because during the 'applyDamage' from the 5e ruleset it has to call the 'decodeDamageText' and this is when CA 'magic replaces' the damage line with the one they 'stole' at the start of the damage roll process. Basically replacing the correct valid adjusted at the correct points in the damage process values created by AWD, with the non-processed 'stashed' out of date values from the start of the damage roll.

I reported this as a problem when they started the extension, it seriously invalidates the damage roll process and all the calling systems for modifications. I do not understand 'why' they store this early value and re-inject it later in the damage. But its a total mess that needs to be replaced, and I said this to them last year ( or even the year before then. )

CA is seriously breaking the damage roll process with using the stored old decoded values, this is why it breaks. Infact it would break any valid extension that changes values in the damage process stack using the damage modification stack function provided by FG.

The CA extension needs re-writing without this 'decodeResult' local stashed value. I'm sure they have a good reason, maybe it holds some values they they need later. But they should not re-inject it back at a later stage. They should process out of the old stored value the items they need, and merge that with the 'current' version the damage stack is using at the time they want to modify.


-pete

While I can't speak much to the process of testing here given that it all exists behind closed doors, if one takes a look at the CA code it will be seen that indeed CA does hold on to the decode results from the ruleset. However, the assertion the old values are re-injected is simply inaccurate as there is no re-injection, merely inspection. The conclusion that CA breaks the damage modification stack is also, unfortunately, demonstrably false; when using CA alone with the 5e ruleset all CA damage types combine perfectly with all damage modifiers. I'd be happy to discuss in more detail the motivations for the use of a local variable for value inspection, though at a high level this is simply the best way to use the result of the decode calculations later in the ruleset damage application logic without overwriting, or re-inventing, ruleset logic. The alternatives would be highly likely to conflict, as "doing a bunch of extra decode of damage type stuff" drastically reduces compatibility with other extensions that are designed against the ruleset itself.

Sadly, I get the feeling given the flow of the conversation thus far that finding a solution is of secondary priority to assignment of fault. As such I will be backing out of this thread going forward, and earnestly await a favorable decision to seek a resolution for this conflict.

[Leprekorn]

@bratch9 I'm sure there is a way this could be resolved without so much finger pointing both of these extensions provide excellent functionality how can we move this forward?

[bratch9]

@bratch9 I'm sure there is a way this could be resolved without so much finger pointing both of these extensions provide excellent functionality how can we move this forward?

Not with the local variable, because they do back force it. Even if they think they do not.

function decodeDamageText(nDamage, sDamageDesc)
if not decodeResult then
decodeResult = decodeDamageTextOriginal(nDamage, sDamageDesc);
if string.match(sDamageDesc, "%[HEAL") and string.match(sDamageDesc, "%[MAX%]") then
decodeResult.sTypeOutput = "Maximum hit points";
end
end
return decodeResult;
end

Basically the 'local decodeResult' start off as 'nil', and so on the 'first' call to decodeDamageText, the 'if not decodeResult' will flow into the 'if' and capture the decodeDamageTextOriginal values. From this point on ANY call to 'decodeDamageText' used by the ruleset will return the original first decoded values held in 'decodeResult'. This includes the latter call to 'decodeDamageText' in the 'applyDamage' from the ruleset which is why my extensions modified values get overridden. Instead of correctly decoding the current on the fly values in the code flow, the ruleset if 'forced' to use the old stale value. ( Because decodeResult now has a value, it just skips over the decode and returns the old values when the ruleset code asks to re-decode the current values. )

Only when the 'applyDamage' ruleset function later calls the 'messageDamage' call is the decodeResult set back to nil and releases the held values. ( And waits for the next 'decodeDamageText', to store the values. )

Its clear from the image I provided that the CA 'stored' value is getting back into the results, and is why the correct values AWD generates get 'lost'. If this is not supposed to be the case, then its still a bug in CA.

Ive shown than my extension changes the values during the process, which it is allowed to do. I've shown that multiple calls to 'decodeDamageText' happens because CA captures it at the start and holds on to his until the message happens. Which happens in the 'applyDamage' after further 'decodeDamageText' call so the ruleset expects this structure to 'change' over time in the damage processing. ( Otherwise the ruleset would 'hold' the value as well, instead of multiple decodeDamageText calls. )

My extension does not hold onto the values, it can not 'magic' back the wrong values into the results. What you are seeing when the values do not return as expected is the net result of the 'stored' values in 'decodeResult' getting re-applied when the ruleset 'applyDamage' function later calls 'decodeDamageText' to process the 'Current' state, but in fact CA returns the old stored pre-decoded values. Which is how the wrong values gets re-injected back into the 'applyDamage' and the incorrect values end up as a result.

CA needs to remove the 'local decodeResult' and assume the structure been passed around into functions designed to change them, actually changes them, it can not and should not 'store' the values.

Clearly from the 'response' above, CA team does not 'expect' the values to get re-injected... they seem shocked that I'm telling that is the reason. So maybe they need to look into this 'bug'.

Its very clear in the 'red box' in the image I provided that when the CA extension is active the stored #13 decode results gets re-used in the later 'applyDamage' ruleset function. ( And you see it in game with the incorrect values been applied. ) As I've output the values at different stages of the damage function flow.

Finding a solution is why we are talking about this, the solution is to stop the bug that is re-injecting old values caused by the CA code.

I'm very willing to take on fault, when its in my extension, the fact the fault is in CA and they are not accepting the issue.

CA say 'assertion the old values are re-injected is simply inaccurate as there is no re-injection', but this is clearly shown to be happening in the images, and I've very clearly stated which parts of the CA code is causing this and how it happens.

I dont know how I'm getting painted as the 'bad guy', I'm going out of my way to debug an extension I do not own, I'm providing the feedback and reasons and how this issue is been caused.

This is not my bug, its an issue in CA.

[Leprekorn]

To be clear I'm not painting anyone as the bad guy. Inferring tone is very difficult online.

[bratch9]

To be clear I'm not painting anyone as the bad guy. Inferring tone is very difficult online.

I was not referring to you. ( But to CA,

Sadly, I get the feeling given the flow of the conversation thus far that finding a solution is of secondary priority to assignment of fault.

, which imply they think its a bug in my code and its more important for me to blame them for the bug... But it is their bug, and yep I'm pointing the issue at them as their bug. )

If you take a look of this image with just CA loaded, you can see 2x calls to 'decodeDamageText'.

ca_only.JPG

This is a normal weapon roll, you get 2 calls to decodeDamageText. In this case 'nothing changed' between the early and later calls, and all the results match up.


-------------------------------------------------------------------------------

With CA and AWD, and an hew hit, the 2x decodeDamageText is clear.

ca.JPG


I've marked on the issues on the second call. In the early call that CA 'store' the #13 damage, the later call with the modified results of AWD shows that the function is called with a request for '#4' damage, and the dynamic decodeDamageText call shows the original ruleset decodeDamageText decoded results 'nVal=#4', because that is the value that is requested in the 'applyDamage' 5e function that calls the second decodeDamageText, with the values AWD passes in.

I've marked with the 'red box' the point CA returns the 'old stale' values from the first call, and this causes the 're-inject' of the old values back into 'applyDamage' and it then applies '#13' damage instead of the '#4' that is called for in the function.

I've attached the changes to the CA 'manager_action_damage_ca.lua' ( renamed txt to make it upload ), to show the shown outputs.

-------------------------------------------------------------------------------

I hope this makes it clear that CA is holding and re-injecting 'old out of date' decodeDamageText, and it is totally the fault of this that is causing the incorrect damage result you are seeing.

The ruleset calls the decodeDamageText twice, once in the early stages of the damage functions, then the 'modification' can be applied, then it calls it again in the 'applyDamage' function to resolve the requested damage. It is totally within the ruleset for AWD to 'change' the initial early values, with the more specific per-NPC before the call to 'applyDamage'.

The fact CA are 'holding' and 're-injecting' the old early values is not compliant with the ruleset. They should not hold this value, and the ARE re-injecting it into later results as shown. ( Which they claim they are not. )

[MeAndUnique]

The ruleset calls the decodeDamageText twice, once in the early stages of the damage functions, then the 'modification' can be applied, then it calls it again in the 'applyDamage' function to resolve the requested damage.

Except it doesn't. The text "decodeDamageText" exists exactly twice in the entirety of the ruleset code: once to declare the function, and one call made in applyDamage.

It is totally within the ruleset for AWD to 'change' the initial early values, with the more specific per-NPC before the call to 'applyDamage'.

Per above, it totally isn't. While I wouldn't call the flow necessarily unreasonable, it is notably outside the assumed flow of the ruleset, and therefore would be a high risk of conflict with other extensions.

If you take a look of this image with just CA loaded, you can see 2x calls to 'decodeDamageText'.

Yes, CA itself does make an additional call to decodeDamageText prior to applyDamage to pre-calculate healing "damage" that should increase the target's maximum hit points. And due to the fact the the ruleset only makes this call once, CA ensures that the precalculated value is used during the ruleset call as well. While I grant that "pre-calculation" and "re-injection" may be considered semantic differences from one point of view, a vast amount of software development is intrinsically defined by semantic distinction.

I reported this as a problem when they started the extension, it seriously invalidates the damage roll process and all the calling systems for modifications. I do not understand 'why' they store this early value and re-inject it later in the damage. But its a total mess that needs to be replaced, and I said this to them last year ( or even the year before then. )

We did indeed have a conversation about compatibility in January. At which time I explained the rationale and invited feedback for alternatives. The last I heard, the conflict had been resolved on the AWDE side of things...

But do not be upset with my extension, CA is at fault for this one.

And then out of the blue this is stated quite publicly and pre-emptive of any real investigation.

I'm very willing to take on fault, when its in my extension, the fact the fault is in CA and they are not accepting the issue.

This is sorta the crux of the issue, and there are two key points here.
I'll start with the less important one: when two extensions work when on their own and conflict when together, assignment of fault could not be so trivial. Neither extension has bugs of its own. Both extensions experience a bug together. It is a shared thing. Either extension would be capable of resolving the issue.
The really important part though is that fault simply does not matter. The overwhelming majority of extension developers in the community recognize this. We work together to make the best experience we can for the users. And in the event that we deem the effort of compatibility too great to justify, we don't waste anyone's time assigning blame or putting each other down. We simply own the fact that sometimes extensions are incompatible in their goals.

Personally, I do not think that CA and AWDE have hit the point of concluding incompatibility. However, given that CA works on its own and with roughly 100 other extensions of varying capabilities including additions to the modifier stack and dammage application, if I am to be able to address the present conflict in CA I require either more constructive guidance than being advised to rewrite it, or a better understanding of exactly how AWDE deviates from the ruleset.

[bratch9]

Ok fixed your extension for you... And yes I changed your decodeDamageText which was causing the issue and injecting incorrect values. Now it does not.

I've not checked any of your 'CA function' changes, so maybe the CA weapon features have some issues. But I dont think this will be the case.

The only 'issue' I can think that conflicts with AWD is the 'transfer' option when you swap the source/target. As AWD will do things like effect 'size/type' checks which will not be against the source and not against the target. So with 'transfer' it will 'turn' the AWD specific damage selections onto the source. So if your character was a 'plant' and the weapon had '1d6 slashing,type(plant)' as part of the AWD then that 1d6 extra would be applied to 'you' as the 'you'/source is now the target. This is 'probably' what you want with the 'transfer' option anyway. ( basic flip the weapon on its owner ? )

Because of the way 'transfer' is checked, if it was on an AWD line with a check ie '1d6 slashing, type(plant), transfer'. You extension would always do the swap for the 'transfer' but in an interaction with AWD it probably should only apply the 'transfer' if the target was 'type(plant)'. But this would be complex to sort out and probably such a low chance not worth the worry. But you might want to note that 'transfer' use when interacting with AWD options, should not have 'transfer' damage type on any 'special' AWD lines that are basically 'optional' applied by AWD. It depends on how you want to think about the process of what it is trying to do.

-pete