Extension functions not correctly called by the super class functions.(tested in 5e)

[bratch9]

I've noticed that overrides of functions only work from 'external' functions, while functions in the super class(es) still use the original super version of the functions and not the overridden version of the functions.. This forces you to search out all usages of these functions in the super class and also override all those functions and extra code to make it work....

( these are tested in FGC in ruleset 5E, but its the same in FGU. I dont think its ruleset specific, this is just the functions I used.. )



Attached is a zip with 3 campaigns and 3 extensions, and a 'patch' file for and unzipped '5E-rulesets' ( which adds debug chat output on 5 functions.. )

camp1,

uses ext1 which overrides,
onDamageAction,onDamageChanged,onInit,onClose

Open character 'bob', and swap to 'actions' tab. This will run the onInit sequence,

s'onInit start'
s'onInit start - ruleset'
s'onDataChanged start - ruleset'
s'onDamageChanged start - ruleset'
s'onDamageChanged end - ruleset'
s'onDataChanged end - ruleset'
s'onInit end - ruleset'
s'onInit end'

You can see the extension 'onInit' is called and this calls the super.onInit from the ' - ruleset' functions.

It also shows that ONLY the ruleset version of 'onDamageChanged' is called... which should have been overridden.


When you 'double click' on the 'longsword' damage box,
s'onDamageAction start'
s'onDamageAction start - ruleset'
s'onDamageAction end - ruleset'
s'onDamageAction end'
bob: [DAMAGE (M)] Longsword [TYPE: slashing (1d8=2)]



You can see that 'onDamageAction' from the extension correctly calls and flows into the super version in the ruleset it calls. ( Called from external to the scrips and rule set script.. )

Using the 'damage edit' button you will get this output,

s'onDataChanged start - ruleset'
s'onDamageChanged start - ruleset'
s'onDamageChanged end - ruleset'
s'onDataChanged end - ruleset'

Showing again that the extension version of 'onDamageChanged' was not called.


camp2, uses ext2 which add override for 'onDataChanged', as its clear from above that this is called by the ruleset to sequence the update.

If you do the damage action sequences above you will get exactly the same 3 output blocks as above as this extra 'onDataChanged' is also never called just like the 'onDamageChanged'


camp3, uses ext3 which brings the 'contents' of the 'super' function for 'onInit' and 'onDataChanged' into the extension from the 5e rule set code base,

On open of 'bob' and 'actions' tab,
s'onInit start'
s'onDataChanged start'
s'onDamageChanged start'
s'onDamageChanged start - ruleset'
s'onDamageChanged end - ruleset'
s'onDamageChanged end'
s'onDataChanged end'
s'onInit end'

You can see, now I dont call the 'super' version of the function but just used the 'contents' for these 2 functions, the 'onDamageChanged' in the extension is used and correctly call the super version.

Also when you use the 'damage edit' button you will now see,
s'onDataChanged start'
s'onDamageChanged start'
s'onDamageChanged start - ruleset'
s'onDamageChanged end - ruleset'
s'onDamageChanged end'
s'onDataChanged end'

You can see again that the extension version are now called by the correct 'DB.addHandler' in the exension 'onInit'


The issue with this is that all the extensions override 'onDamageChanged' but the ruleset 'super' still called ONLY the rule set version of this function.

You have to check in the original ruleset file and add all functions that also call 'onDamageChanged' to make the override work because the merge has not extended the base class functions.. It only does this for 'external' calls to these functions.

I would expect the ruleset ( or super ) version to call the new exteneded override function, and assume that if needed the new extended function would call the 'super' version of the function to pass down to the original ruleset code.

I dont expect to have to 'hunt' around in the code and end up having to override extra functions and move the code from those function into my extension to make this work.

This is not how inheritance/function overrides should work. ( Well in c++ you would virtual the 4 functions and the base class would call your inherited functions.. )

Thanks, Pete

[Moon Wizard]

I believe that you are running into how Lua scoping and assignment works.

While you are overriding any external reference to functions when attempting to replace functions, any internal reference to a function within a script will use the internal reference to the function. Also, whenever a function reference is registered, it's the exact point to that function that is registered; not a general reference to a function with that name.

That is part of the reason I've been adding so many callback registration type of functions to bypass that limitation of Lua scoping.

Regards,
JPG

[bratch9]

I believe that you are running into how Lua scoping and assignment works.

While you are overriding any external reference to functions when attempting to replace functions, any internal reference to a function within a script will use the internal reference to the function. Also, whenever a function reference is registered, it's the exact point to that function that is registered; not a general reference to a function with that name.

That is part of the reason I've been adding so many callback registration type of functions to bypass that limitation of Lua scoping.

Regards,
JPG

Thanks for the feedback, I wanted to make sure I was not missing something or that the code merge for scripts was not having further issues..

I guess when I want to change code in 'updatePowerWindowUses' in power_page.lua because that function touches the 'local aGroups = {};', and so I have to 100% replace the file to add my code ?

Or do you have a suggestion for a better solution. ( As its a pain to have to take 100% of the code and replace as it then causes further extension issues if the load order is not correct.. )

Thanks,
-pete

[Moon Wizard]

In most cases, 100% replacement is the only solution for making sure functions are overriden.

Regards,
JPG

[bratch9]

So I had a bit more of a dig in the code and documents....

adding 'self.onDamageChanged()' instead of just 'onDamageChanged()', in the 'onDataChanged' function allows the 'outermost' function to be called making camp1 top example work as I'd expect...


see 'self = Always refers to the outermost object with all layers applied, even if called from a lower layer.'

in,

https://www.fantasygrounds.com/wiki/...ts_-_Scripting


This sort of implies that when you call functions internally you should prefix them with 'self' to allow override ?

Adding it to the onInit and onClose seems to also work for the 'DB.addHandler' function pointer as well, making camp2 work as expected..

Maybe this needs to become a 'coding standard' and rulesets updated to allow for better compatibility for extensions...

( But I guess that is a massive change across all the rulesets........ )

-pete


gives me,

s'onInit start'
s'onInit start - ruleset'
s'onDataChanged start - ruleset'
s'onDamageChanged start'
s'onDamageChanged start - ruleset'
s'onDamageChanged end - ruleset'
s'onDamageChanged end'
s'onDataChanged end - ruleset'
s'onInit end - ruleset'
s'onInit end'


function onInit()
Debug.chat("onInit start - ruleset");
local node = getDatabaseNode();
DB.addHandler(node.getNodeName(), "onChildUpdate", self.onDataChanged);
DB.addHandler(DB.getPath(DB.getChild(node, "..."), "abilities.*.score"), "onUpdate", self.onDataChanged);
self.onDataChanged();
Debug.chat("onInit end - ruleset");
end

function onClose()
Debug.chat("onClose start - ruleset");
local node = getDatabaseNode();
DB.removeHandler(node.getNodeName(), "onChildUpdate", self.onDataChanged);
DB.removeHandler(DB.getPath(DB.getChild(node, "..."), "abilities.*.score"), "onUpdate", self.onDataChanged);
Debug.chat("onClose end - ruleset");
end

function onDataChanged()
Debug.chat("onDataChanged start - ruleset");
onLinkChanged();
onAttackChanged();
self.onDamageChanged();

local bRanged = (type.getValue() ~= 0);
label_ammo.setVisible(bRanged);
maxammo.setVisible(bRanged);
ammocounter.setVisible(bRanged);
Debug.chat("onDataChanged end - ruleset");
end

[Moon Wizard]

I almost mentioned that in my previous response as a possible approach. However, that would only work in some situations. (i.e. fixed calls)
Any function registrations are still fixed as the current function reference at the time the registration occurs.

Basically, it gets complicated fast, which is why I point people to replace the file, or layer on top of an existing file (for templates and window classes).

Regards,
JPG

[bratch9]

I guess only using it about 10 times outside character wizard code in 5e ( and the extra 15 or so in the character wizard ), shows that its not used much in the rule set.

I'm looking from the growing number of extensions side, and it probably should be used more in the rule sets. ( CoreRPG has a bunch, as do most of the standard rulesets.. )

I would guess that the rule sets pre-date some of these features and so have not been upgraded/re-written, specifically with respect to extensions.

( I know a bunch of the 5e ruleset, and can see a lot of functions that look to have been made more extension friendly .. Thanks for sorting these out for us.. )

I do think that,

super.foo() to call up the chain

and self.foo() and foo() should do the same thing..

and local function boo() and boo() should be private..


Maybe if the function registrations setup onto calling local function and these called a self.function to do the work, this would cover a lot of cases with less problems, but open up extensions to not copy chuncks ( or 100% of file ) from the ruleset..

As you say, you have wrapped a number of registered callback function which call an update function, maybe these calls to the update functions should be marked with self. so that extensions can grab and extend the update function at the outermost level while keeping away from the registered call back problem area...

You know much more than I do, my experience is limited in the code base, but maybe we should have a thread to allow extension coders to suggest adjustments that would expose areas cleaner and save a bunch of pain and make extensions more stack-able and flex-able ?

-pete