Allow targeting non-unique actors with StartScript. Fixes #2311 #2648
Conversation
|
From what I can tell, target can be a non-actor object. In this case this PR does not help, so it really does not fix the bug, right? A first step to fix the issue would be to check how Morrowind stores global script targets in saves. |
akortunov
added
the
Saved game format
|
I guess RefNum should be stored instead like in Morrowind. |
|
Ah, yeah, it should use refnum. That's going to be an interesting change. |
|
I think this is more or less done. (Squashing and changelog entry aside.) Could someone label this as "needs testing"? |
|
Are... are you sure such low-level changes in the scripting system are essential for getting the point of this PR to work? |
|
You mean changing every instruction to ignore missing implicit references? It needs to happen one way or the other to prevent scripts that have lost their reference after loading a save due to not having a refnum from crashing. It's invasive but replicates vanilla behaviour. As for the change to move reference caching to globalscriptdesc, that seems like the place to put it since contexts don't live long enough. I still want to move enable and disable and such out of components and into apps/openmw, but that's not for this pr. |
|
So now that 0.46 has been branched off... anyone have any thoughts? @zinnschlag maybe? |
|
Its been an awful long time since I looked at this part of the code base. It is entirely possible that there have been changes that I missed or that I simply misremember. But from what I do remember RefNums are not unique across cells. That would make storing the RefNum without storing the CellID alongside it a non-workable solution. The sweeping changes to the scripting engine are somewhat troubling. If we had still been on the old roadmap (expanding the MW-scripting language) that would be a resounding no from me. With the new plan (Lua) this is less of a problem. But we will get into trouble again once the Lua implementation is underway. We are only kicking the can down the road. I am still not convinced that it is a good idea to fix this bug at all. Seems to cause more trouble than it is worth. |
|
A cursory check of all three ESMs suggests RefNums are unique. (Assuming the below is correct) I'm also not particularly happy with the number of changes, unfortunately I don't see a way of solving the problem more elegantly. Regardless, saving aside, the current way OpenMW deals with references is pretty broken. This PR only exists because I ran into the issue while designing a quest. |
|
I see. This is true for static reference (from esm/esp). So the problem are the references created during gameplay. Maybe it is worth looking into that. Presumably this could allow for a fix to this issue that is less disruptive. |
|
I would like to have a proper way of tracking references (if only for future scripting) for sure. I don't think this is the place for it however, because using refnums (and thus losing targets on loading a save) replicates vanilla behaviour... and I rather suspect someone has made a mod rely on that particular quirk. Still, if we're willing to ignore that, wouldn't a system using proper references for dynamic objects run into the same problem if reference added by a mod got targeted by a script (from another source) and the mod then got unloaded? Hmm, might not be worth worrying about since the workaround would just be to save the game and then restart OpenMW. |
I would be okay with not replicating such behaviour. I know we original stated that OpenMW should be able to run all original content that doesn't use third party binaries. But have abandoned that goal for some corner cases already (scripts so broken that we simply could not replicate the broken compiler behaviour without breaking other stuff). Given the choice I would rather ensure a solid future for OpenMW than to support some niche bug-using plugin.
That's also something we should look into eventually. Some kind of automated cleanup-phase when a save is loaded with a different content file configuration? |
Mhm. Though if we can have that solid future in Lua and inane compatibility in mwscript, that'd get my vote.
Some of it already happens doesn't it? Might be good to get a write-up on what's already there and what's missing. Not today however. It's weekend time for me. To recap though, do you think it'd be worthwhile to implement something akin to actorid for all records in this PR and just forgo refnums? |
The (very) long term goal is still to replace the mwscript engine with a translation tool that turns old scripts into lua. Maintaining two scripting engines (and especially two virtual machines) would not be beneficial for OpenMW in the long run.
+1
Not really. I am kinda regretting having introduced actor IDs by now (fairly but not entirely sure that was my doing). Having multiple systems that do the same thing is not good for the quality of the code base. |
Agreed, but translating specific mwscript instructions to fairly complex Lua scripts shouldn't be much of an issue right? Wasn't the plan to do the same with mechanics?
What would you do differently now? (Also I'd think a hypothetical "refid" system could replace actor IDs.) |
You think it is too late to back out without a major refactor effort? Do we just role with it or perhaps a post 1.0 goal? |
We need to cover all the workarounds for the buggy scripts resulting from the original vanilla script compiler. This will be a major pain in the arse.
Exactly. Again, having two systems that do the same (or partially the same) is a bad idea. |
Post 1.0 the earliest. We already have too many disruptive changes ahead of us. Better not to add more at this stage. |
For sure. But that shouldn't surprise anyone. Anyway, if we're not getting a better reference system pre 1.0, can this just get merged? |
|
A imrpvoed reference system before 1.0 is not entirely of the table. That is only true for merging the actor ID system into it. Since I have stepped down from my BDFL position its not up to me anymore to make the final call. But I am still not in favour of this merge. IMO a better approach would be to first improve the reference system and then attempt to improve the targetted script situation in a less invasive manner. But if it isn't possible then so may it be. My original statement on the issue stands. This may be one of the few situations were we are better of with just accepting an incompatibility with vanilla MW. |
|
I don't mind implementing a better reference system first (as time permits, of course) and I'm not exactly opposed to not dropping references on load. It's just that I don't see how that reduces the invasiveness since global scripts that lose their reference still need to keep running, and having them throw a reference not found exception means they can't even be restarted until OpenMW is relaunched. |
|
Hm, interesting point. Let's think this through. Do global scripts still need to run after they lost their reference? The thing they run on is gone. So there is really no point in keeping them running. Most likely it wouldn't work anyway. The only issue I see is that the script should be able to be restarted on another reference. Currently we mark a script as to be ignored if either If we were to differentiate between these two, we could allow restarting targeted scripts in case b by simply removing the mark first when starting a targeted script. |
|
Yes, that could work. If we further differentiate by creating some sort of "missing implicit target exception" we can even continue marking scripts that will never work as ignored. The only issue then would be mods actually relying on the "default to 0" behaviour, but it's probably better to just ignore that possibility until someone actually raises an issue. (I'd be interested in the use case too.) |
|
That dropped the number of files changed by 7. 🎉 |
|
That looks interesting. Will need a bit more time to review it. I will get back to you next week. |
|
That was quite an extensive rewrite. And yay for visitor pattern. I still think it would have been better to take on the ref num problem first and then implement a (relatively) clean fix for this issue here on top of it. But since we now have the code we can as well roll with it. There are two points I want to comment about:
|
|
Conclusion: It's a bit odd, but this branch gets pretty close to the vanilla behaviour. Close enough I'd say. Running a script on a damaged axe I placed via ESP (i.e. has a refnum and is different from a regular copy) works like normal. I got it to report its distance to the player, its disabled status, and its health (unfortunately GetHealth returns max health for weapons and armor). I also made it disable/enable every time it reported all of that. So I had a nice, blinking axe. Then I went and picked it up and the script simply kept reporting its last know location and the enabled/disabled status also kept updating properly - despite there being no axe. So I saved and reloaded to see if that'd do anything. The script still worked, the only thing that'd changed was the location of the item: it'd moved to 0,0,0. (I checked, it wasn't actually physically there.) When I tried the same thing with this branch I got pretty much the same result. Only difference being that the axe didn't get "moved" to 0,0,0 after reloading. |
|
Okay. Sounds reasonable enough. I have nothing more to add. Other than the merge conflicts and apparently CI failing I don't see anything that should stop us from merging. |
|
Actually, there is one minor improvement that could be made. The "unresolved vs resolved to nothing" isn't very intuitive. I think this could use a line of comments or two. |
|
Fixed the "conflict" and added a few comments. Even looks like CI likes it again now. |
|
Does it require a savegame format change now? At least, this PR does not increment a savegame version. |
|
Also it would be nice to squash commits. |
It changes the format so yes. I didn't increment the number because of the other format changing PRs - if you merge a couple at the same time you'd only need to increment once after all. Want me to increment it now? |
Yes, if Zini wants to merge this PR. |
|
All squashed up and ready to go. |
But you did not squash a commit name. I suppose it should be named something like this: |
|
Ah right. This is a bit shorter than before. |
|
@zinnschlag, @Capostrophic, what do you think? |
|
Thanks @Assumeru ! |
Fix a regression caused by #2648 (fixes #5513)
Note that this makes new saves incompatible with older OpenMW versions.