Skip to content

Fix multi-events for better metatable compatibility

llamazing requested to merge llamazing/solarus-free-resource-pack:multi-events_fix into dev

Issue

Addresses #30 (closed)

Resolution

  • Adds a unit test for #30 (closed)
  • Adds a fix for multi-events to handle registering events with metatables where __index provides a separate table to store the meta events
Scenario Before After
1. metatables where __index is function to store data in the metatable itself (i.e. solarus metatables) ✅ ✅
2. metatables where __index is another table to store data ❌ ✅
3. metatables where __index is function with other behavior ❌ ❌

Scenario 3 proved too difficult to implement without complicating things too much. The fix to add support for scenario 2 covers most use-cases. I figure if someone actually needs multi-events to work with scenario 3, then they can modify the script to suit their needs. Since the __index function would be a custom application anyway, it is difficult to anticipate all use-cases.

As implemented, multi-events under scenario 3 will work normally with the regular object, but meta events will not be chained as if they never existed.

Unit Test

The unit test added is data/maps/tests/bugs/test_30_multi-events_custom_metatables.lua

It tests the following usage:

local multi_events = require"scripts/multi_events"

local mt = {}
multi_events:enable(mt)
mt:register_event("on_started", function() --[[blah]] end)

local menu = setmetatable({}, {__index=mt})
menu:register_event("on_started", function() --[[blah]] end)

sol.menu.start(map, menu)

--mt on_started callback must be triggered followed by menu on_started callback for successful test

Upon uploading the new unit test, the pipeline failed. Only the menu on_started callback was triggered as expected.

The pipeline passes with the fix (as well as all previous multi-events unit tests).

Other Remarks

The old version of the multi-events script (before I inadvertently broke it in MR !43 (merged)) was compatible with scenario 2, so this fix restores backwards compatibility. Technically the old version of the script was compatible with scenario 3 as well, but it didn't implement it correctly 🤷

Merge request reports