PFRPG Ammunition Manager

[bmos]

Hello Bmos, hello B9,
I have discovered a compatibility issue in-between B9's Advanced Weapon Damage and Bmos' Ammunition Manager extension. Nothing causing a script error, just an inconsistent UI.
Tested with a fresh 5e campaign and only those two extensions (plus B9 Core, so actually three) loaded.
When both are loaded, the "load" button is visible even for melee weapons. You can load and unload them (with corresponding chat notification), and both extensions work, so it's just cosmetics.

Likely this is because B9 is overriding onDataChanged in char_weapon without calling previous copies via super.onDataChanged.
I don't know this for sure, but that's the most likely thing I can think of. If I'm correct, the hiding of the ammo checkboxes and linking of the ammo field when you link ammo should also not be working.
If overriding this is required for the features of that extension (since of course it's usually best to call the prior function when you override it), it shouldn't be hard to add some compatibility code to the replaced function based on this.

[bratch9]

Likely this is because B9 is overriding onDataChanged in char_weapon without calling previous copies via super.onDataChanged.
I don't know this for sure, but that's the most likely thing I can think of. If I'm correct, the hiding of the ammo checkboxes and linking of the ammo field when you link ammo should also not be working.
If overriding this is required for the features of that extension (since of course it's usually best to call the prior function when you override it), it shouldn't be hard to add some compatibility code to the replaced function based on this.

I've released v2.7, which adds calls to the super onInit/onClose which clear up the loading/unloading icons on my tests... but not sure if the 'DB.addHandler(DB.getPath(nodeWeapon), 'onChildUpdate', onDataChanged)' in my onInit and in yours will both be called due to them been on the same node...

As I'm not sure on the exact setup and usage for 5e to have these show up or not and work etc.. can some one check with the v2.7 and see if this has resolved the issues. If not we can talk about now to deal with DB.addhandler calls from both extensions.

-pete

[bratch9]

Likely this is because B9 is overriding onDataChanged in char_weapon without calling previous copies via super.onDataChanged.
I don't know this for sure, but that's the most likely thing I can think of. If I'm correct, the hiding of the ammo checkboxes and linking of the ammo field when you link ammo should also not be working.
If overriding this is required for the features of that extension (since of course it's usually best to call the prior function when you override it), it shouldn't be hard to add some compatibility code to the replaced function based on this.

So I did a check with the hotfix version and my version against the 'onDataChange' call back registered by both extensions. It does seem with debug.chat tests in both extensions that both get called..

So you dont need the refactor your 'onDataChange', as calling the super is not required. And also you would need to pass down the 'nodeWeapon' into the 'super.onDataChanged(nodeWeapon)'.

But feel free to have a test against v2.7 to see if other conflicts exist, but you should not need that slight refactor as both are getting called.

It probably was just the onInit/onClose in my extension not calling the super, which was skipping your onInit/onClose which registers you onDataChange etc... all getting lost with the error in my code.

-pete

[bmos]

So I did a check with the hotfix version and my version against the 'onDataChange' call back registered by both extensions. It does seem with debug.chat tests in both extensions that both get called..

So you dont need the refactor your 'onDataChange', as calling the super is not required. And also you would need to pass down the 'nodeWeapon' into the 'super.onDataChanged(nodeWeapon)'.

But feel free to have a test against v2.7 to see if other conflicts exist, but you should not need that slight refactor as both are getting called.

It probably was just the onInit/onClose in my extension not calling the super, which was skipping your onInit/onClose which registers you onDataChange etc... all getting lost with the error in my code.

-pete

I think onChildUpdate actually provides nodeWeapon as an argument there anyway.
But I look forward to confirmation that it's sorted, thanks!

[Ratupper]

Hello, can this be extended to pathfinder 2.0? SoxMax created a ammo manager for starfinder https://forge.fantasygrounds.com/shop/items/1128/view and the ruyles as far as I know aren't any different but if you need any info I am happy to help. Your extensions are amazing you are very talented. You have made my GM job much easier.

[bratch9]

I think onChildUpdate actually provides nodeWeapon as an argument there anyway.
But I look forward to confirmation that it's sorted, thanks!

Yep the callback does provide the node...

but you line

if super and super.onDamage then super.onDamage() end

does not pass it down... I was point out the bug that it should have been...

if super and super.onDamage then super.onDamage(NODE) end

but I assume you have removed this code as both extensions have registered handlers and both handlers get call... so no need to try and call a super function.

-pete

[bmos]

you line

if super and super.onDamage then super.onDamage() end

does not pass it down... I was point out the bug that it should have been...

if super and super.onDamage then super.onDamage(NODE) end

Ah, I see what you mean! Indeed that is a mistake on my part.
I have pushed a new version out with this fix in case there will be an extension with a lower load order that misses that call. Thanks for pointing that out and explaining it so clearly!

[Arnagus]

Testing with B9 - Advanced Weapon Damage 2.7, Common Core v0.1a & Bmos Ammunition Manager v4.0-hotfix.3, the issue persists
Screenshot 2023-03-28 204348.jpg

[bratch9]

Testing with B9 - Advanced Weapon Damage 2.7, Common Core v0.1a & Bmos Ammunition Manager v4.0-hotfix.3, the issue persists
Screenshot 2023-03-28 204348.jpg

Because AWD replaces some of the onDataChange, onAction,onDamage as it has to evaluate damage line clauses and reject or include depending on if things like 'slashing,type(undead) in the weapon damage line, it would be best if Ammo manager could load after AWD, so after loadorder 110. I'm not sure if Ammo manager can move from its 34, which seems a very specific number so maybe locked to a low value due to other extension conflicts. I dont know.

But since Ammo manager only seems to not call the super version in cases when no ammo exists and should not process, it make more sense for a load order after mine.

( I'm also not convinced that some of the addhandler calls should also call the supers as it assumes only one addhandler will fire, when they all fire from each level... so you end you with multiple calls as each addhandler fires and then the supers call extra versions on top of the addhandler version of other extensions.. it gets a mess. I need to look into what the original ruleset code is registering and what is been called. )

At the moment the self.onDataChange in the ammo manager oninit, is causing the AWD onDataChange function to be called, which does not call down to the super and hence does not call the ammo manager onDataChange. But if you edit and cause an update action then both extensions onDataChange get called from the addhandlers registered from each extension and it should sort the 'load'/'unload' visibility. But if we both have super calls in these then they will nest recursive which could cause data to be process multiple times which might cause issues... Specially on the onDamage and onAction versions of these functions.. we dont want to end up doing multiple 'performRoll' action calls which would happen if both extension 'onDamageAction' were called and hence can not and should not have super calls. In these cases the resolution is to basically have one extension that is loaded last have to 'process' all other extensions.. This can be seen in both our code as we both have to 'have special' code for 'AdvancedEffects' extensions as our call cause us to replace the lower order loaded 'AdvancedEffects' onDamageAction changes... so it propagates code from other extensions into each of ours. With the current load order, I'd have to include and process Ammo managers 'code' for onDamageAction in my onDamageAction to resolve all extensions and end up with just a single 'preformRoll' call.

So its not just a case of load order, and super calls... this gets very messy very quick due to how the ruleset was written and how things merge.

( Sorry if i have to edit this in the morning to have it make sense, its late night for me )

-pete

[Arnagus]

Sounds like a lot of effort, likely too much for a cosmetical issue - I have advised my players to ignore the "load" button on melee weapons. Functionality was not affected (at least when I reported the issue), so when they "load" a sword it's funny one or twice then no more