Workaround found for Aura Effect / Polymorph conflict

[Vam]

I recently purchased @SilentRuin's Polymorph extension, and I love it. Unfortunately I also use @bmos's Aura Effect, and the two do not get along. I've spent a few hours this morning troubleshooting it, and while I don't have a fix I do have a workaround. I'll start with the workaround for anyone who wants to use these together, and then share my other findings in case either SilentRuin or bmos can see anything I can't for a proper fix.

The workaround is to install Aura Effect manually from the github repo and remove the load order from the xml definition. You can download the 1.8 ext file from https://github.com/bmos/FG-Aura-Effe...eases/tag/v1.8. Unzip it into a "FG-Aura-Effect" folder in your extensions folder. Use your favorite text editor to load the extension.xml file and remove the line: "<loadorder>999</loadorder>". This is on line 8 in version 1.8. Start / restart FG, enable both extensions, and at least from my testing you're good to go.

You can stop reading now unless you want to learn more about the problem.

The crash happens in manager_combat.lua from CoreRPG in this code:

	for _,fCustomDelete in ipairs(aCustomDeleteCombatantEffectHandlers) do
		if fCustomDelete(nodeEffectList.getParent()) then
			return true;
		end
	end

The value for nodeEffectList changes while the loop is running. How? I have no idea. From my limited understanding of Lua this should not be possible.

I wanted to see if Aura Effect was doing anything funky. I found that if I commented out this line it didn't crash:

	CombatManager.setCustomDeleteCombatantEffectHandler(checkDeletedAuraEffects);

However if I commented out the entire contents of the checkDeletedAuraEffects function it still crashed. Again, that doesn't seem to make sense. I removed everything from the script other than the one line above in the onInit function, and an empty version of checkDeletedAuraEffects, and it STILL crashed. I deleted all the other files other than the one lua script and the xml definition. It still crashed. I removed everything not required from the XML and it worked! I restored the full version and removed a chunk at a time from the xml to find the loadorder workaround.

Next I looked at it from the other side. What could Polymorph be doing to cause this? I found a call to CombatManager.setCustomDeleteCombatantEffectHandle r and commented it out. It fixed the crash at the expense of functionality. I commented out the contents of the onEffectsChildDeleted function which was the handler added. It fixed the crash. Remember, when I did this in Aura Effect it did NOT fix the crash. Interesting. I got it narrowed down to a single line in the deleted handler:

		NotifyHostPolymorphismActivated(nodeCT, true, nLastHP);

Removing this line fixes the crash, but it breaks the Polymorph extension. I think the next step would be to see if I can narrow down what happens in the OOB_MSGTYPE_POLYMORPHACTIVATED handler, but I didn't go any further with that.

Instead I went back to the CoreRPG code that is actually crashing. I added some debug statements to it:

function onDeleteCombatantEffectEvent(nodeEffectList)
    Debug.console('start loop', nodeEffectList)
	for _,fCustomDelete in ipairs(aCustomDeleteCombatantEffectHandlers) do
        Debug.console('in loop', nodeEffectList)
		if fCustomDelete(nodeEffectList.getParent()) then
			return true;
		end
	end
    Debug.console('end loop', nodeEffectList)
	return false;
end

Aha, it is recursing and somehow this is blowing away the nodeEffectList value! Why? I don't know. But I think that if Polymorph could avoid this recursion it would *probably* solve the problem. Here's the logs from above when ending polymorph concentration:

[2/21/2022 9:37:55 AM] s'start loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'start loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'end loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | userdata
[2/21/2022 9:37:55 AM] [ERROR]  Handler error: [string "scripts/manager_combat.lua"]:100: attempt to index local 'nodeEffectList' (a userdata value)

I think that the change in load order fixes it because the loop then ends without using the corrupted value, so the recursion doesn't cause the crash.

I started trying to narrow things down in the OOB_MSGTYPE_POLYMORPHACTIVATED handler, but gave up. I think the PolymorphActivated function is causing the unwanted recursion somewhere. I also think there's something wonky in the Lua interpreter that's allowing the value in the first loop to be corrupted. Maybe someone with more extension / Lua experience will spot something I missed.

[SilentRuin]

I recently purchased @SilentRuin's Polymorph extension, and I love it. Unfortunately I also use @bmos's Aura Effect, and the two do not get along. I've spent a few hours this morning troubleshooting it, and while I don't have a fix I do have a workaround. I'll start with the workaround for anyone who wants to use these together, and then share my other findings in case either SilentRuin or bmos can see anything I can't for a proper fix.

The workaround is to install Aura Effect manually from the github repo and remove the load order from the xml definition. You can download the 1.8 ext file from https://github.com/bmos/FG-Aura-Effe...eases/tag/v1.8. Unzip it into a "FG-Aura-Effect" folder in your extensions folder. Use your favorite text editor to load the extension.xml file and remove the line: "<loadorder>999</loadorder>". This is on line 8 in version 1.8. Start / restart FG, enable both extensions, and at least from my testing you're good to go.

You can stop reading now unless you want to learn more about the problem.

The crash happens in manager_combat.lua from CoreRPG in this code:

	for _,fCustomDelete in ipairs(aCustomDeleteCombatantEffectHandlers) do
		if fCustomDelete(nodeEffectList.getParent()) then
			return true;
		end
	end

The value for nodeEffectList changes while the loop is running. How? I have no idea. From my limited understanding of Lua this should not be possible.

I wanted to see if Aura Effect was doing anything funky. I found that if I commented out this line it didn't crash:

	CombatManager.setCustomDeleteCombatantEffectHandler(checkDeletedAuraEffects);

However if I commented out the entire contents of the checkDeletedAuraEffects function it still crashed. Again, that doesn't seem to make sense. I removed everything from the script other than the one line above in the onInit function, and an empty version of checkDeletedAuraEffects, and it STILL crashed. I deleted all the other files other than the one lua script and the xml definition. It still crashed. I removed everything not required from the XML and it worked! I restored the full version and removed a chunk at a time from the xml to find the loadorder workaround.

Next I looked at it from the other side. What could Polymorph be doing to cause this? I found a call to CombatManager.setCustomDeleteCombatantEffectHandle r and commented it out. It fixed the crash at the expense of functionality. I commented out the contents of the onEffectsChildDeleted function which was the handler added. It fixed the crash. Remember, when I did this in Aura Effect it did NOT fix the crash. Interesting. I got it narrowed down to a single line in the deleted handler:

		NotifyHostPolymorphismActivated(nodeCT, true, nLastHP);

Removing this line fixes the crash, but it breaks the Polymorph extension. I think the next step would be to see if I can narrow down what happens in the OOB_MSGTYPE_POLYMORPHACTIVATED handler, but I didn't go any further with that.

Instead I went back to the CoreRPG code that is actually crashing. I added some debug statements to it:

function onDeleteCombatantEffectEvent(nodeEffectList)
    Debug.console('start loop', nodeEffectList)
	for _,fCustomDelete in ipairs(aCustomDeleteCombatantEffectHandlers) do
        Debug.console('in loop', nodeEffectList)
		if fCustomDelete(nodeEffectList.getParent()) then
			return true;
		end
	end
    Debug.console('end loop', nodeEffectList)
	return false;
end

Aha, it is recursing and somehow this is blowing away the nodeEffectList value! Why? I don't know. But I think that if Polymorph could avoid this recursion it would *probably* solve the problem. Here's the logs from above when ending polymorph concentration:

[2/21/2022 9:37:55 AM] s'start loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'start loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'end loop' | databasenode = { combattracker.list.id-00008.effects }
[2/21/2022 9:37:55 AM] s'in loop' | userdata
[2/21/2022 9:37:55 AM] [ERROR]  Handler error: [string "scripts/manager_combat.lua"]:100: attempt to index local 'nodeEffectList' (a userdata value)

I think that the change in load order fixes it because the loop then ends without using the corrupted value, so the recursion doesn't cause the crash.

I started trying to narrow things down in the OOB_MSGTYPE_POLYMORPHACTIVATED handler, but gave up. I think the PolymorphActivated function is causing the unwanted recursion somewhere. I also think there's something wonky in the Lua interpreter that's allowing the value in the first loop to be corrupted. Maybe someone with more extension / Lua experience will spot something I missed.

As I said to you earlier - its likely timing. Polymorphism uses effects to determine if it is in a polymorphed stated and when you stop concentration it will mark those effects "expired". This causes them to get removed and removing them causes the polymorphism to revert back to its original state (which removes the current state). This of course works everywhere unless there is a "timing" conflict where something tries to store and hold the effects after they are actually removed. Next time they try to use that "list" it will fail because the underlying stuff that the list belonged to no longer exists.

If load order removes the error that is simply because it alters the timing of when that "list" is being used. I'm not sure how anyone can have an effects "list" that is not retrieved at time of use - but if there is something out there that "stores" a list tied to a CT entry then yea... that's looking for trouble. As I said - I have no issue with this anywhere else and some things are simply not compatible. Whether this list is not being retrieved at time it is needed and stored somewhere (where it can disappear between time it is retrieved and time it is used) I have no idea. I for sure don't hold onto lists or anything that can be altered during runtime and get things immediately when I need them. THIS does not mean that I don't have some issue - its just that I don't see it hence I can't "fix" it. And if order solves this then that indicates a conflict and some things - just conflict. I have no plans to take this further as I have no idea where I could take it. I don't have any pointers to anything that can be "removed" anywhere that I'm aware of. If anyone else does - FGU or another extension - that would be a recipe for ... well.... this.

Anyway that is what my limited "look" tells me here. Sorry - but I have no plans to proceed any further on this unless someone can show me how I'm not doing a generic sequence of operations that should not always work. Which appear to always work - in my world. Some things just conflict.

[Vam]

Here's a better example to make my point that the problem isn't Aura Effect. A very simple extension that should do absolutely nothing, but causes the exact same crash as Aura.

extension.xml:

<?xml version="1.0" encoding="iso-8859-1"?>
<root version="3.0">
	<properties>
		<name>Foo</name>
		<version>0.1</version>
		<loadorder>101</loadorder>
		<description>
			<text>Conflicts with Polymorph extension</text>
			<author>Vam</author>
		</description>
		<ruleset>
			<name>5E</name>
		</ruleset>
	</properties>
	<base>
		<script name="Foo" file="foo.lua" />
	</base>
</root>

foo.lua:

function empty()
end

function onInit()
	CombatManager.setCustomDeleteCombatantEffectHandler(empty)
end

This causes the exact same problem, and the reason is that it loads after Polymorph and it registers a callback with CombatManager.setCustomDeleteCombatantEffectHandle r. Any extension that does this will cause this crash.

SilentRuin, at the end of the day I really like your extension and I'm excited about using it. I'm happy that I can work around the problem now. I'm not saying that the problem is your fault. I really think the problem is in the interpreter. I see no reason for the local variable in onDeleteCombatantEffectEvent to get trashed.

[SilentRuin]

Here's a better example to make my point that the problem isn't Aura Effect. A very simple extension that should do absolutely nothing, but causes the exact same crash as Aura.

extension.xml:

<?xml version="1.0" encoding="iso-8859-1"?>
<root version="3.0">
	<properties>
		<name>Foo</name>
		<version>0.1</version>
		<loadorder>101</loadorder>
		<description>
			<text>Conflicts with Polymorph extension</text>
			<author>Vam</author>
		</description>
		<ruleset>
			<name>5E</name>
		</ruleset>
	</properties>
	<base>
		<script name="Foo" file="foo.lua" />
	</base>
</root>

foo.lua:

function empty()
end

function onInit()
	CombatManager.setCustomDeleteCombatantEffectHandler(empty)
end

This causes the exact same problem, and the reason is that it loads after Polymorph and it registers a callback with CombatManager.setCustomDeleteCombatantEffectHandle r. Any extension that does this will cause this crash.

SilentRuin, at the end of the day I really like your extension and I'm excited about using it. I'm happy that I can work around the problem now. I'm not saying that the problem is your fault. I really think the problem is in the interpreter. I see no reason for the local variable in onDeleteCombatantEffectEvent to get trashed.

The reason is as I said - something holds that list pointer and allows the underlying data to be removed in between the time the list is stored and the time it is used. Your example shows this is likely the callback (engine) allowing this gap between storage of pointer and use of it.

[SilentRuin]

To be super explicit the loop for

for _,fCustomDelete in ipairs(aCustomDeleteCombatantEffectHandlers) do

will have one of those trigger a removal of the node with the effect list and cause any following calls to fail on that same list. Not sure why they are not doing this by node or doing a check to make sure it is still valid but that is the reason of the failure. As the engine is doing this callback there is no real way to insure the list is processed when it still exists except by load order insuring the last one is the one that removes the list.

In the end, its a catch 22 conflict where FGU is not designed for the node to go away during these callbacks.

[Vam]

Thanks SilentRuin. I appreciate you looking into it. I wrote another small extension that blocked recursion in onDeleteCombatantEffectEvent, but it didn't prevent the problem, so I think recursion was a red-herring for me. Maybe one day I'll come back to this and see if I can find another way to achieve the effect you need without breaking other plugins, but today is not that day.

[bmos]

I stopped using setCustomDeleteCombatantEffectHandler.
https://github.com/bmos/FG-Aura-Effe...b654d659a3a9cb

The script error from Polymorph that occurs without Aura Effects loaded at all is still there, but the extra error that only occurs with Aura Effects and Polymorph loaded is no longer present.
Not sure that's a big win, but gets it off of my plate :P

The fix will not be live until the big ruleset updates are released soon (tm).

[Vam]

That's amazing bmos, thank you!

I think the other Polymorph crash was from removing an item from the combat tracker while polymorphed. I can live with that one, I just have to remember not to do that.

@SilentRuin, bmos came across this when I showed him how to use Polymorph on discord. I didn't bring it up here, but there you are if you feel like looking at it.

[bmos]

@SilentRuin, bmos came across this when I showed him how to use Polymorph on discord. I didn't bring it up here, but there you are if you feel like looking at it.

To clarify, this is a different issue than the subject of this thread.
Steps to reproduce:
1. Add Shapeshifting NPC to combat tracker
2. Shapeshift the NPC
3. Delete the shapeshifted NPC (which impressively reverts it to the original NPC)
4. Console opens with script error from line 40 of ct_effectlist.lua complaining about v being a userdata value.

[SilentRuin]

I've also just delivered a fix (in DMsG not in Forge yet) - DO NOT get used to me fixing FGU issues that are not robust enough to handle legit operations (like removing nodes in CT), as nothing annoys me more than doing this sort thing... but as its my operations (legit as they are) causing this I'll put in this guard.

-- prevent conflict crash
function onDeleteCombatantEffectEvent(nodeEffectList)
	if type(nodeEffectList) ~= "databasenode" then
		return false;
	end
	return saveonDeleteCombatantEffectEvent(nodeEffectList);
end

Not sure when Grim will deliver it to forge but will be sometime today likely.