Implement misc spells for OpenMW by Assumeru · Pull Request #33 · TD-Addon/TD_Lua_Addons

Assumeru · 2026-04-29T15:04:58Z

Includes #32.

trav5555 · 2026-06-18T20:45:13Z

+local appliedOnce = {}
+for _, effect in pairs(core.magic.effects.records) do
+    if effect.isAppliedOnce then
+        appliedOnce[effect.id] = true
+    end
+end


Should we gather all information on all effects right at the start, or should we perhaps only cache the data for the ones that have been queried? Something like:

local appliedOnce = {} local function isSpellEffectAppliedOnce(effectId) if appliedOnce[effectId] == nil then appliedOnce[effectId] = core.magic.effects.records[effectId].isAppliedOnce end return appliedOnce[effectId] end

then the only caller down could just call if isSpellEffectAppliedOnce(effect.id) then

Or is the memory gain not really worth it? I hold no big opinion on this

I'm not so sure you'd gain memory. Indeed if you assume the worst (a session in which every effect has been encountered) that way would use more memory.

You'd definitely lose out in execution time. Though I wouldn't expect the difference to be noticeable.

You're likely right. Not needed probably

trav5555 · 2026-06-18T20:50:19Z

+    for _, spell in pairs(activeSpells) do
+        local effects = {}
+        for _, effect in pairs(spell.effects) do
+            effects[effect.index] = STATE_IGNORE


Does this line mean that in the following scenario:

A new actor appears (on new cell entry I suppose - but not on game start/game load)

That actor has a pre-placed (like: in CS, maybe from worn equipment) custom effect on them

then that custom effect gets initialized and marked as STATE_IGNORE, so I understand not triggering anything?

I haven't tested this thought if it's correct in any way. I'm parsing this code part very slowly. I'm not even sure there's currently a custom effect that could be used for such testing (constant effect armor resartus? just guessing)

Any effects that this code applies to started before we could detect them. We could pretend that means they started just then, but that might be wrong.

Note that we discard an actor's state when it becomes inactive and only has STATE_IGNORE effects (that we're not interested in tracking.)

Now realistically inactive actors don't have active effects so odds are it's all moot anyway.

Restartus is an applied once effect btw. It doesn't make much sense as a constant effect. (I assume this was done because Morrowind.exe doesn't allow condition to be incremented by a floating point value. And I'm not sure enchantment charge would allow them in OpenMW.)

Acknowledged. Is there btw any openmw merge request/issue that would replace this code altogether eventually, or not yet?

Don't think so. Foal's MR could be used for durationless effects, but can't replace any of the others.

trav5555 · 2026-06-20T14:47:42Z

+    requiredLuaApi = 129,
+    settingsPlayerSectionStorageId = "Settings_TamrielData_page01Main_group02Magic",
+    settingsEnabledByDefault = true,
+    settingsKey = "Settings_TamrielData_page01Main_group02Magic_miscBlinkIndicator"


Would be good if this setting were to be placed after the miscSpells setting. (perhaps it could be made to be toggleable only if the miscSpells is enabled, but not worth the effort imo) To achieve that this key needs a change. My suggestion:
Settings_TamrielData_page01Main_group02Magic_miscSpellsBlinkIndicator

I think all these keys are obnoxiously long and we should be using the order property to order them.

trav5555 · 2026-06-21T09:32:57Z

+return {
+    eventHandlers = {
+        T_Passwall_Cast = magic_passwall.onCastPasswall
+    },


Organizational suggestion -- we don't need a player_magic.lua file anymore. These three lines is the only relevant functionality preventing this file from becoming a player_magic_blink.lua. My original design with a master loop of course required a centralized control point, but now it's not there anymore.

My suggestion:

move this event handler to player_magic_passwall.lua (it would replace the return PSW in that file -- onCastPasswall wouldn't need to be accessible from the outside anymore)

make player_magic_passwall.lua a PLAYER script in omwscripts

change this file to player_magic_blink.lua (change the effect record id in line 3 to a blink effect), keep it as a PLAYER script in omwscripts

I don't like the big "all spells in one lua" file approach very much and believe splitting the scriptfiles would be useful for maintenance. (ex. the Lua profiler would track it more granularly).
Unless there's a disadvantage to splitting?

With regard to Passwall, I did sort of mean to keep this minimal in the name of reviewability (hence also the two PRs) but that might have been too optimistic.

Anyway splitting files isn't free. It's mostly free when it comes to player and global scripts that don't need to track state. (So it'd be fine here.) And there's obviously advantages to maintainability or at least discoverability. But there is a price to pay in load and execution time. There's probably an argument to be made for splitting things into utility files that all get required by the same script... except you'd either end up with quite a lot of very small files or you'd end up consolidating them somehow which seems like it'd defeat the purpose.

We definitely want to keep the number of "actor" scripts down.

I see. If the execution/load and number of actor scripts is something we aim to have in low numbers, then ofc this current approach is acceptable. I won't pursue that avenue then :)

trav5555 · 2026-06-21T09:47:54Z

IMO if a file is not a PLAYER/ACTOR/GLOBAL/MENU script in omwscripts, then it shouldn't have a player/actor/global/menu prefix in its naming. And this - in its current form - is a utility file with a function used by both the actor and player blink scripting. I suggest one of these options:

a rename - to magic_blink.lua

Or - if we'll have more of magic utils files like that (I don't see any more yet) - maybe even make a new directory magic for magic utilities and put this file in. I'd call it blink_utils.lua in that case

or move it to utils directory, although I think utils should include framework stuff only, not gameplay stuff

EDIT: On the other hand, even if the script is not marked directly in omwscripts as a PLAYER/ACTOR, it may be written in a way that makes it usable only by player/actor scripts (ex. it calls functions only accessible within that scope). One could say that such approach should warrant even utility files to have a prefix in their name. Maybe also a valid point. Will think on it, unless you have opinions. Not a gamebreaking thing, really

I have no strong feelings on naming. I tried to match what was there mostly. Adding additional subdirectories could be worthwhile (whether magic or say global/menu/local.)

The only thing that really matters is that we're essentially locked in once we release. For any script that saves state anyway. So ideally we'd get it right the first time 😛

trav5555 · 2026-06-21T13:13:43Z

+            core.sound.playSound3d('mysticism hit', self) -- TODO !3029
+        end,
+        T_Blink = function(magnitude)
+            -- TODO: check if levitation is disabled


I made a ticket for that in https://gitlab.com/OpenMW/openmw/-/work_items/9163

trav5555 · 2026-06-21T15:37:35Z

+
+    local function getCasterPenalty(candidate)
+        if not casterPos then
+            return 0


I feel like with the convention this function is doing (i.e. if I'm not misunderstanding this, then: it's not strictly a penalty, it's a bonus, but the bonus is lower the closer the candidate point is to the player 🤓) returning math.huge here would work better. But maybe it's a matter of no consequence. No strong feeling

Returning math.huge ruins the math because it'd all just turn to infinity.

You're right about the name, though.

trav5555 · 2026-06-21T15:53:32Z

+                state = data
+            end
+        end,
+        onUpdate = function(dt)


Expanding my organizational comment in player_magic.lua https://github.com/TD-Addon/TD_Lua_Addons/pull/33/changes#r3448199617 I suggest the same about this actor_magic - that it may be better if it was split into files per spell/effect. Because for instance if I understand correctly, this onUpdate is purely for Distract?

Right now it's only for Distract, yes. Hard to say if it'll stay that way of course.

trav5555 · 2026-06-23T21:01:03Z

+            if record.enchant then
+                local enchantment = core.magic.enchantments.records[record.enchant]
+                if enchantment then
+                    -- FIXME: this is incorrect for autocalc


How so? What's missing?

trav5555 · 2026-06-23T21:12:22Z

+    end
+    local equipment = actor.type.getEquipment(actor)
+    local toFix = {}
+    for slot, item in pairs(equipment) do


slot is unused, so can be _

trav5555 · 2026-06-24T20:49:41Z

+                local mult = effect.magnitudeThisFrame / 100
+                reflectedHealth = reflectedHealth + health * mult
+                health = health * (1 - mult)
+                reflectedFatigue = reflectedFatigue + fatigue * mult
+                fatigue = fatigue * (1 - mult)


Not sure if there's a difference, but to ensure the same number is accumulated and subtracted, I'd have written this part like that:

local mult = effect.magnitudeThisFrame / 100 local healthChange = health * mult local fatigueChange = fatigue * mult reflectedHealth = reflectedHealth + healthChange health = health - healthChange reflectedFatigue = reflectedFatigue + fatigueChange fatigue = fatigue - fatigueChange

but maybe it's of no consequence. I'm always paranoid about floating points

trav5555 · 2026-06-24T22:33:27Z

+    end
+    local items = {}
+    for _, item in pairs(actor.type.inventory(actor):getAll()) do
+        -- TODO: filter items. Don't copy MWSE, it doesn't account for script added items


What do you mean by that, exactly? You mean the getAll doesn;t really get all things, or it gets too much? I recall some 'invisible' light-emitting objects (welkynd enemies? aurorans? not sure) which were problematic for pwn's QuickLoot (you could pick them up). But are they problematic here, considering the new container is still using the engine backend for opening it? Assuming that's what you meant?

Assumeru added 30 commits April 9, 2026 20:27

Implement summon effects in OpenMW

6f074c9

Fix positioning, vfx, hostility, and update effects upon going inactive

67c1c52

Avoid spawning summons behind walls

b37d919

Use MWSE data

6517136

Improve performance

c0308a1

Add setting

e4eff0f

Support hypothetical multiplayer

0d154e2

Implement enchantment, potion, ingredient, and item changing

e8cbe1e

Account for load order changes

5a56df1

Delete summons instead of disabling them

1fc3ed0

Switch to nicer syntax

658504a

Fix typo

f63aa31

Reduce code duplication in setting up MWSE misc effects

18f79bd

Turn Passwall into a real effect

0feef41

Implement Reflect Damage

b716f4b

Implement Fortify Casting

dfca7ee

Implement Resartus

7524d92

Adapt hyacinth's Distract

fdc993a

Implement Summon Bouquet

8feed06

Implement Corruption

b35cd20

Implement Wabbajack

352562a

Mostly implement Banish

ba0268a

Finish Banish for now

492ac47

Implement Blink

7bfa24a

Implement Blink indicator

7bfe18f

Don't send Min-Tal's sigil into the ceiling

2a4c468

Improve Blink destination

88090bf

Improve Blink some more

e0252c1

Account for load order fuckery

bc810ad

Fix loading artifact data

7cac36b

trav5555 reviewed Jun 21, 2026

View reviewed changes

Address preliminary feedback

4c0e7f1

trav5555 reviewed Jun 23, 2026

View reviewed changes

trav5555 reviewed Jun 24, 2026

View reviewed changes

Uh oh!

Conversation

Assumeru commented Apr 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants