Don't stack scripted items (bug #2969) #2059
Conversation
|
Everyone cool with this? :) |
|
I'm not sure if I agree with this. One of my mods add scripts to items that can stack (scrolls), and while this should not affect it overmuch it'll probably make vendors annoying. Not to mention other mods like the explosive arrows, the 'auto equip arrow stacks' etc. Also stacks of scrolls in the world... Also how does this 'break' exactly? Seems to work fine around here, you 'just' have to extend dangerous functions such as 'delete' and such to only affect one of the stack. Aren't you over-exaggerating the 'danger'? Also about the '1 script per object instance'. Well, that should be bleeding obvious. All objects in the same stack are the same kind of object so they have the same kind of script, but shouldn't be the same instance (though i imagine this might get completely bad if someone, say adds 200K scrolls to the inventory for testing). Maybe in a stack the only 'active' script should be one of them as the others treated as if the script was suspended? Also you should really add a way to remove scripts from not global scripts, but we all know your stance on MWScript so i'll not hold my breath about things like 'no useless memory leaks on scripts'. |
|
Morrowind doesn't stack scripted items at all. I'm trying to replicate that. A stack of items is technically a single reference which has a single script. So if you equip one item from the stack, all carried instances of the item have OnPCEquip set to 1 when it is equipped. Similar thing about other flags (OnActivate, OnPCDrop, OnPCRepair etc.). This causes various kinds of issues with scripts that expect the scripted item to be "alone".
There aren't any "others". It's a single reference. Don't, I repeat, don't long-term rely on behavior that is subject to change.
Take it easy with passive-aggressiveness, it's not my personal stance, mwscript untouchability is something that was decided upon when Zini first agreed to a future Lua-based scripting system. |
Ok. but this is not necessarily a problem if you treat special actions on a stack as first separating one unit (cloning the object with a stack value of 1) of it and then using it, which was what i was thinking you were doing anyway. Though i guess that does not work as a rule for onpcdrop, which is supposed to apply to all of the stack, but even then i don't really see the harm of the script running on the stack. A stack is represented as a single object in the world, and if some dumb script wants to burn it up or something, that's not very surprising. Even my mod that uses OnPcEquip has zero problems dropping a stack to learn a spell from the scroll, because there is just one script running (and if there were more they wouldn't run all at once) and i need a global variable to 'spend' the script for the whole class of items (though i suppose if i had to use a local variable, problems could arise when i separated the stacks). The problem with the 'cloning' idea now that i think about it is that it shouldn't be clone but use the default values (for local variables), and handling the merging and separation would get tricky (with non-default local variables overwriting the 'new' instance). Like, do you seriously want scripted arrows to turn into 200 separate instances on the inventory all running a script (or scrolls etc). Isn't this a Bethesda mod? |
If that's what Morrowind does (and it does), sure.
Arrows from Area Effect Arrows are enchanted, not scripted. As Hrnchamd has said some time ago:
|
|
I cannot say i approve of this because it makes my own mod kind of useless. But it's your habit to chase MW behavior instead of extending it i'm pretty sure that even if i had a great idea, you'd just go ahead. The thing is if no mod except mine is using this as you assert, then you can make the rules. Your rule seems to be 'no doesn't happen' and that's by farrr the worst way possible. My current 'best' ideas is: extend MWScript to have a 'stack' iterator function similar to while but automatic with a 'reference' that holds OnPcEquip etc. Won't happen for obvious reasons. Or warn it on the editor/runtime and leave it to people who know what they're doing like my own mod that works fine and doesn't use local variables badly. |
for your information, is the universally agreed goal of 1.0.
Don't make OpenMW-specific mods intended for running with Morrowind assets. Make mods compatible with Morrowind engine so OpenMW can work with them too (or will work with them at some point). |
Let me laugh at that, hah fucking hah. First the mod could work because i build the records manually instead of on cs, though i didn't try because i can't run the original. Second your own fileformat extension discourages running mods on morrowind so don't be sanctimonious about this. I wonder if i could 'prove' if the mod worked the same way in morrowind you'd leave what you consider broken behavior there. https://github.com/i30817/raremagic4openmw it's the scroll component. edit: thinking about it, don't even bother because MW requires the compiled script record unlike openmw that only uses the script text, so the mod is basically impossible in MW anyway for unrelated reasons to the behavior. |
Okay, this doesn't really track with the link at https://gitlab.com/OpenMW/openmw/issues/2969 If it's a single instance, how is it printing twice in that script that guards against that? Isn't the main problem that the script does get cloned, but that the stack separation is too late and thus variables like OnPCDrop get turned on for all of them? edit: ugh, nvm about that - he resets the 's_has_spell' variable always - although the following idea is still a valid way to handle this as long a separating the stacks always 'reset' into a clone with default variables for the script. Though of course, this gets more complicated with items that are already in stacks in the world which i saw plenty of times in vendor houses or Tamriel Rebuilt tombs for scrolls. What's 'OnActivate' for a single stack? This is why i suggested that there is no 'real' issue in treating a stack script as a single item... until they get separated, upon which they should clone the script (from a prototype or from the currently running script i'm not sure, though i'd suggest 'prototype' - that would take care of the error in the bug report), and upon merging, in which case they should only merge if the local variables are the same on the script (maybe complete object 'deep equality' if you manage to implement that). So in the example given the amulet would show the message and then when unequiped no longer stack until the other amulets were equipped (because it changed the 's_has_spell' local variable), which seems sane at first look. |
|
Allow me to make the behavior closer to Morrowind behavior to improve Morrowind mod compatibility in peace please, without overcomplicating things trying to adapt a different behavior to act the same. Edit: to reconsider, any possible improvements are not out of question at some point in future (no, not necessarily post-1.0), but I firmly believe we need something working properly first, some baseline, and those are not in the scope of this PR. |
|
@i30817 your frustration is understandable, but don't attack developers for it. Calm it down please. Until OpenMW 1.0 has be released, don't assume that your OpenMW specific mods will keep working. The only 'official' API is the one that Morrowind itself has and the behaviour of MWScript. OpenMW has never made an implicit nor explicit promise of compatibility beyond MWScript. This is to enforce mod compatibility between OpenMW and Morrowind. This will change once we have 'newscript' in place. |
|
Again, you didn't explain what happens with your 'separate scripts instances are different items' with stacks on the world already (from multiple mods). Does the script run for all of them (lol), or does it run '1' and when you pick them up they get cloned? If you're going to be 'okkkay this behavior causes problems' later on your own 'fix', might as well pull the plug now and tell me to my face 'your mod is going to stop working because we're going to forbid scripts in stackable items, not 'turn stackable items with scripts into non-stackable items' ' and i'll write off openmw for about 2 years to forever. Also the notion of 'enforcing compatibility' on a thing that doen't actually exist on the original... Good luck automatically converting all the old Bethesda copyrighted scripts to 'newscript' without being sued, you'll need it to turn frame by frame scripts into event driven scripts. |
Excuse me? I have already stated Morrowind doesn't stack scripted items for a very good reason.
OpenMW has already been converting Morrowind scripts into bytecode automatically for years and somehow Bethesda hasn't freed an army of lawyers on anyone from the AUTHORS.md file yet. It's not going to be greatly different if and when mwscript scripts are converted into Lua scripts first. Like, it's not actually necessary to redistribute anything. |
|
@i30817 Please, try to work with us instead of being sarcastic all the time. Right now you're being a dick and at the same time, you want the devs to do it your way. Want to get some influence in the project? Pro tip: Start with being polite. I'm not here to debate anything, just state my opinion in this. |
A transpiler without any modification is ofc fine (until lawyers decide to butt in with a new insanity). Editing the scripts so they work on a listener model of scripting is another thing entirely. Just look at the trouble scummvm goes through to fix SCI and others scripting languages. It's done with runtime patching of the compiled machine code (not even decompiled sources). Your project is lucky that cs includes source of every script into esps, but even that won't make manual modification unnecessary if you really want to change model. And manual source modification is illegal unless by runtime patch 'that doesn't distribute original code', which is pretty much a diff hostile legality.
So... what's going to happen with stacks placed in the game world or vendor inventories? Will i suddenly see 200.000 scripts running if i place a 200.000 stack on a cell on openmw-cs and apply my mod? Or vendors showing 200 separate arrows all running scripts if a mod decides to add scripts to them? For that matter, some vendor inventories are collected from items in the world, what happens if that 'item' is a stack with a script? Will the world-object even get items subtracted if I buy a few inventory-objects? I believe there is a very good reason scrawl diverged from morrowind, and if 'reverting' will just mean that eventually you'll decide 'no, every stack item won't have scripts', so you might as well think of that 'hypothetical' right here and now instead of a future bug report. If you want to go on in the traditional 'MW did nothing wrong' route, well, my mod - and every future mod scripting stackables - is broken and you might as well remove the option. Meanwhile i gave you a alternative option: perform stacking only when the object instance deep equals match (make script equals only if their instances are of the same source and their local variables are equal), separating the stack creates a new object with a deep clone (and you need to make sure that script variables like 'OnPCEquip' are only set after performing a stack separation when you drop a stack on the paperdoll or a repair item on the stack). I think you shouldn't separate the stack on dropping items (OnPCDrop) for obvious reasons. OnActivate.... i think should just run on the stack like OnPcDrop. For activation on inventory, it can only be done by dropping on the inventory paperdoll, which would already clone/separate. But for world activation, well, you can't just spawn a 'separate' item on the world at the same location as another can you? And it's sort of what already happens in stacks on the world without scripts. You activate a stack of scrolls and the scroll text only opens once (not to mention the 'Take' button for books and scrolls in the world is visible only if you activate them). A minor inconsistency that probably deserves a open-cs warning when you script a stackable but i have no better solution and much better than throwing out the baby with the bathwater. |
We've actually discussed something like this and it's a vague possibility (and shouldn't be something we can't get done pre-1.0 if someone has time) but the critical distinction between this and that is that the implementation in this PR fixes a bug right away, but doing something more complicated leaves the bug in-place for a while as the new thing needs to be written and tested. Merging this now has to happen whether or not a new solution will be made later.
It's much more likely that he thought it was probably fine and did it without thinking through whether or not anything would break (as otherwise the change would have been accompanied by a bunch of discussion and someone would remember that). The guy was good, but only the pope has the superpower of infallibility (and I think he might be lying about that). |
|
Close bug, open (at least) 3? The second paragraph of my previous response still applies, and the rationale for pushing for a decision. Half measures here just make me think that you're inching to remove the ability altogether by making a decision that opens those bugs and makes mods that use scripts on stackable items unusable. |
Yes. |
|
Well, do what you want, i made my argument. |
|
The idea behind this PR is correct. Items with scripts can not stack. That is because each item with a script has state associated with it (the local script variables). Merging items with scripts will result in lost state. This is hopelessly irrecoverably broken. If I had caught this change by scrawl I would have vetoed it. Stacking only if the state matches is a possibility, as mentioned before but that is better left for post-1.0 development. I am not so sure about the implementation though. As noted the loop isn't great, especially since it works on all items, even those that do not have scripts. A simple approach would be to check if the item doesn't have a script and then use the loop-less implementation. Just to make sure do we know exactly how vanilla handles this: Does it actually unstack if you add a stack of items with a script? Another option would be to simple reduce the number of added items to 1 and print a warning message. But of course we should try to be as compatible as possible. |
Yes, when you try to use AddItem to add a stack of scripted items, you'll get multiple items in the inventory instead. And I'm not sure if it's specific to scripted items (it might apply to any items with the same ID that can't stack in general). Simply changing stacks() only makes it work with the items that have been picked up separately, since a new stack is created for these. I'll try something, though. Taking a closer look at the stacking check, it seems that the only way two items could be combined in a stack by add erroneously is if they were scripted, as any other parameters are the same for newly added items. |
Capostrophic
force-pushed
the
stacks
branch
2 times, most recently
from
8861fc9
to
f486c1a
Compare
|
Does anything else need attention? |
Capostrophic
changed the title
|
Seal of approval from @zinnschlag :) |
|
It's been two months and Zini is yet to reappear. Are there any objections? |
|
Executive decision, merging. |
Bug 2969.
Make scripted items not stack, ensure scripted items added with AddItem don't stack and optimize an item name recovery loop in RemoveItem.
Scripted items must not stack because every stack is effectively a single reference which can have non-1 count but still have 1 script, but every script must be per-instance and not per-stack (or shit breaks due to script states being merged). This is something scrawl changed a while ago seemingly for no reason. Even then, ContainerStore::add happily creates stacks of anything added even if the added items shouldn't stack (they simply won't stack with existing instances of the same item ID if they can't stack).
Using a loop for ContainerStore::add is obviously not ideal for performance, but I can't think of something else that doesn't horribly break stacking.