Overhauling the weapon rack scripts

[taleden]

OK, I can see that. It actually isn't very hard code.

 

However, I have a guess as to the reason for using GetEquippedItemType (1) and GetEquippedShield() instead of Keywords. The object is still in inventory, and not a real object. It doesn't test Keywords until the object has been removed from inventory.

 

Or as vanilla code says: "; Force the weapon to be dropped, and get it's reference" [sic]

 

Right now, it uses GetEquippedItemType() to decide if its allowed, then GetEquippedWeapon/Shield() to actually get the base item's form to drop. So we could just get the form first and see if its valid via keywords (before dropping it), the same way it's done for HandleStartingItem().

 

The only test that has to be done after dropping is the one for oversized forsworn staves, because that test relies on HasNode().

[Arthmoor]

OK, beauty then. Shouldn't we just adjust the plaques to be closer to the wall? The center rack doesn't have a reverse component, so it must be OK.

I presumed Bethesda did it so the useable rack wouldn't end up too close too the wall collision. They should have created a static rack for the purpose of using as a backplate though. Their interior building collisions are never terribly good so I'd check that before trying to get rid of anything.

 

Arthmoor, the two quests (Sclero and Talenden) are both SEQ and Run Once. SEQ is probably OK, but should they be Run Once? Might that be the cause of the problem integrating the two?

The "Run Once" flag tells it not to fire the OnInit() event twice. So I'd say those are probably fine as they are, especially if they're not intended to be shut down again.

[DayDreamer]

Right now, it uses GetEquippedItemType() to decide if its allowed, then GetEquippedWeapon/Shield() to actually get the base item's form to drop. So we could just get the form first and see if its valid via keywords (before dropping it), the same way it's done for HandleStartingItem().

 

The only test that has to be done after dropping is the one for oversized forsworn staves, because that test relies on HasNode().

As I wrote: "the object is still in inventory, and not a real object. It doesn't test Keywords until the object has been removed from inventory."

 

The starting item is not in inventory, and can be assessed directly.

 

I'm in favor of automating something that you've found to be confusing and inconvenient -- especially as it looks easy. I'm not in favor of re-writing code that works, just because it may be more symmetric with other code. It buys us nothing.

 

Can we concentrate on bug fixing first? Are you perfectly happy with the alias code now?

 

I presumed Bethesda did it so the useable rack wouldn't end up too close too the wall collision. They should have created a static rack for the purpose of using as a backplate though. Their interior building collisions are never terribly good so I'd check that before trying to get rid of anything.

I was merely trying to figure out how to get rid of some nonfunctional CoA racks that spam the debug logs a little. But there is a plaque of a similar shape without scripts -- that could be placed backwards instead. I'm not sure how big of a deal it is.... Is it worth pursuing?

 

The "Run Once" flag tells it not to fire the OnInit() event twice. So I'd say those are probably fine as they are, especially if they're not intended to be shut down again.

I was actually thinking that the quest could/should be started and stopped. After all, players don't place items on racks all that frequently. There's no dialogue, so they aren't SEQ file material, are they?

I'm off to see Ender's Game tonight. I'll be back again tomorrow.

 

[taleden]

'm in favor of automating something that you've found to be confusing and inconvenient -- especially as it looks easy. I'm not in favor of re-writing code that works, just because it may be more symmetric with other code. It buys us nothing.

 

Can we concentrate on bug fixing first?

 

If I had no other reason to touch that code, I'd agree. But the way it's currently designed, adding support for mounting from the left hand would require duplicating all of the rack/item compatibility checks a third time. Rather than do that, it seemed to me a fine opportunity to streamline the logic a little, and in the process make sure that we won't get weird bugs in the future if some item is discovered whose Anim Type and Keywords don't agree (presently, this could cause the item to be place-able as a starting item but not as a player item, or vice versa).

 

If everyone else agrees that having three separate copies of the rack/item compatibility logic is fine, and that bugs arising from Anim Type / Keyword mismatches are also fine, then I guess I don't mind reverting that piece and doing the left-hand support the ugly way. What say ye all?

[taleden]

Here are some tweaks I've been working on today, based on Sclerocephalus' v3.4 scripts that were packaged in USKP 2.0.0. (Sclero, the v3.5 download you posted only had meshes in it, so I'm assuming you haven't updated your scripts since release -- if there are newer versions I should work from, let me know!)

 

These scripts should support placing Ebony Blade, Ghostblade, Wuuthrad, Keening, Silver Sword, Red Eagle's Fury and anything else that used to fail (for example lots of otherwise generic iron/steel/dwarven/elven weapons have a few pre-placed copies with attached scripts, so those would fail even when other copies of the same items were fine).

 

Here's a quick summary of my changes in both scripts (compared to the USKP 2.0.0 release versions), roughly in the order they are in the file, so you can follow along in a diff:

 

Trigger script changes

• UpdateActivatorState()
  • don't call methods on the ObjectReference that came from OnTriggerLeave() since it will be broken for scripted items
  • validate ActivatorScript instead of ActivatorRef, since Script is what we need to get/set properties and it can stand in for Ref anyway
  • if the linked activator is missing, has the wrong script, or is enabled, then send trigger to ActivatorBusy state to ignore further OnTriggerLeave() events

Activator script changes

• new property USKPWRPlayerRefAlias which should eventually be filled in the CK, but for now (to avoid having to override every activator) is set dynamically in InitActivator()
• InitActivator()
  • validate that the linked trigger not only exists but has the right script, just like how the trigger validates the activator
  • set StartingItemHasBeenGrabbed if it IsDeleted() or GetRackItemError()
  • instead of calling HandleStartingItem(), just set PlacedItemInit and PlayersDroppedWeapon (which used to happen in PlaceItem() anyway); the starting item then gets placed below
  • if PlacedItemInit and PlayersDroppedWeapon, then PlaceItem() to re-position all mounted items (including starting items, from above)
• RunPlayerActivation()
  • if no valid item is found in the right hand, check the left hand too
  • removed rack/item compatibility logic from this function, that's now done by GetRackItemError()
• HandlePlayerItem()
  • removed the test for scripted items since they're allowed now
  • dropped reference is now captured by OnItemRemoved in USKPWRPlayerAliasScript, instead of DropObject which fails for scripted items
• HandleStartingItem()
  • deleted entirely, InitActivator() instead uses GetRackItemError() to decide if it's valid, and then PlaceItem() to place it
• GetRackItemError()
  • new function that replaces the rack/item compatibility validation that used to be duplicated in RunPlayerActivation() and also HandleStartingItem()
• CheckFor3D()
  • now checks on every frame draw for 5 times, then every 0.1s for 5 more, then every 1s for 5 more; so in the best case it will return immediately or after 1 frame, but if the engine is bogged down it can wait up to ~5.55s

The only issues I found so far in my testing are:

• two-handed swords mount in the wrong direction on the vertical wooden racks in Vlindrel Hall (the handle is in about the right place, but the blade points upward along the wall instead of down into the rack)
• items which are z-keyed and waved around before being mounted will end up offset from where they should be, usually hanging off the edge of the rack

The first issue maybe needs an edit to the nodes? I don't know much about how placement angle works. I was hoping the second could be solved with an extra MoveTo() with extra arguments to reset offsets to 0 and not match angle, but that didn't do it for me.

Taledens Weapon Rack Item Fix v4.7z

[taleden]

Any comments or criticisms on these edits? Sclero, I'm especially interested in your feedback since I don't want to fork another separate line of development, so if there's anything in here you don't like, let's discuss it.

 

I'd also love to hear from anyone who's tried mounting Ebony Blade etc with these revisions. All weapons should work normally now, so if anyone still has issues even after these changes, let us know.

[DayDreamer]

Downloading them Saturday evening (I see there was only 1 download so far), and flicked the Like button to let you know. But haven't actually looked at it, as I spent the weekend helping relatives winterize (moving tall ladders, cleaning gutters, raising storm windows, etc). And now I'm in pain. And I'm behind on all my email. And it's IETF week; although I'm not there I'll be watching remotely. So I'll try to look during "spare" time.

[DayDreamer]

Sorry, I haven't had any "spare" time. Getting into standards body mind set and writing security protocols has a tendency to crowd everything else out. I'm not a design multi-tasker -- the mind never stops working on the problems, and I awake in the middle of the night with ideas....

 

I see there have been 7 downloads now. I've spent the evening trying to grok the changes. And there were big changes! I'm not finished even doing all the comparisons....

 

This is really a major departure from your previous design. Could you explain the reasoning? It's a bit short on commentary!

 

You are still doing the player.UnequipItem(), but no longer catching the reference with an event.

 

Then you are player.DropObject, and using the OnItemRemoved to catch the reference. Are you doing both because something happens differently?

 

And this one eliminates all use of the USKP chest stuff, instead of being able to switch back and forth for comparison testing.

Any comments or criticisms on these edits?

Frame time is 0.016667, not 0.01, so your timing loops have a tendency to be too short on the early iterations! But the backoff is a great idea, so I'm going to just go with standard exponential backoff.

 

"extends ReferenceAlias" should already have a default "Event OnItemRemoved" and "" state null defaults been artificially sped up internally. So you shouldn't need one, and having it would just slow things down. Or was there a problem?

 

[taleden]

This is really a major departure from your previous design. Could you explain the reasoning? It's a bit short on commentary!

 

It's not that different really, just tightened up. My previous download had a lot of redundant stuff in it because I was trying to demonstrate two alternative methods for accomplishing the same thing, and both of them also included more reliable code for detecting scripted items so that they could be disallowed.

 

In this version I cut that down to just the one method which I think is cleaner, and removed all the code for detecting and disallowing scripted items, both of which simplified the code quite a bit. So don't bother diffing my v4 against my v3, just compare my v4 to the ones released in USKP 2.0.0. I could add some more code comments though, that's a fair point.

 

You are still doing the player.UnequipItem(), but no longer catching the reference with an event.

 

Then you are player.DropObject, and using the OnItemRemoved to catch the reference. Are you doing both because something happens differently?

 

Yes, something happens differently, that's the whole point of my contribution. Feel free to read back over my posts in the last 20 pages, but here's the summary: DropObject() returns a broken pointer if the dropped object has a script and a persistent reference at the time of dropping. This is what caused Ebony Blade etc. to fail on racks. OnItemRemoved does not have this problem, so I use DropObject to cause the drop, but OnItemRemoved to catch the reference.

 

The UnequipItem isn't strictly necessary, but without it, DropObject will prefer to drop non-equipped items. That means if you're wielding one Iron Sword and carrying another, then when you activate a rack, the one in inventory will be dropped and placed while the one in your hand remains there, which seems odd. UnequipItem improves this experience so that the player is never still holding the item they tried to mount.

 

But I don't bother catching the reference from the OnObjectUnequipped event because that was only needed as part of the scripted item detection. Since scripted items are supported in this version of the scripts, they don't need to be detected any more.

 

And this one eliminates all use of the USKP chest stuff, instead of being able to switch back and forth for comparison testing.

 

As I mentioned above, my v3 included two methods for getting the item reference, which I've trimmed down. I chose the non-chest version because it's simpler, and because if something should somehow go wrong, there is no chance of the item getting lost in some inaccessible place.

 

Frame time is 0.016667, not 0.01, so your timing loops have a tendency to be too short on the early iterations! But the backoff is a great idea, so I'm going to just go with standard exponential backoff.

 

I do know the result of 1/60, but chose 0.01 anyway because I was under the impression (according to the Notes in http://www.creationkit.com/Threading_Notes_(Papyrus)) that all native functions return after at least one frame, except for the non-delayed native functions (http://www.creationkit.com/Category:Non-delayed_Native_Function). Since Utility.Wait is not on the non-delayed list, I am assuming it will always wait for at least one frame, so the goal of using 0.01 is just to avoid waiting for more than one frame (unless framerate is >100). In my tests it only ever took one or maybe two cycles of that loop before proceeding (either waiting for OnItemRemoved to fire, or for Is3DLoaded to be true). If your testing shows that it very often takes many loops, then the interval could be raised.

[DayDreamer]

In this version I cut that down to just the one method which I think is cleaner, and removed all the code for detecting and disallowing scripted items, both of which simplified the code quite a bit. So don't bother diffing my v4 against my v3, just compare my v4 to the ones released in USKP 2.0.0. I could add some more code comments though, that's a fair point.

OK. I'll compare that instead. I agree this is a cleaner method. But the alias script is, shall we say, very compact (no comments at all).

 

The UnequipItem isn't strictly necessary, but without it, DropObject will prefer to drop non-equipped items. That means if you're wielding one Iron Sword and carrying another, then when you activate a rack, the one in inventory will be dropped and placed while the one in your hand remains there, which seems odd. UnequipItem improves this experience so that the player is never still holding the item they tried to mount.

Ah. I was confusing "remove" in OnItemRemoved() and RemoveItem(), which preferentially takes equipped items. Both DropObject and RemoveItem do seem to cause OnItemRemoved (not documented or even See Also).

Did you consider http://www.creationkit.com/AddInventoryEventFilter_-_ObjectReference instead of the if test?

We could dispense with the while loop entirely by setting a rack and having the object placed by a call from the event.

 

I chose the non-chest version because it's simpler, and because if something should somehow go wrong, there is no chance of the item getting lost in some inaccessible place.

Yes, hopefully a real improvement over the chest method. Although dropping things still sometimes loses them; but that's usually a mesh bug.

 

I do know the result of 1/60, but chose 0.01 anyway because I was under the impression (according to the Notes in http://www.creationkit.com/Threading_Notes_(Papyrus)) that all native functions return after at least one frame, except for the non-delayed native functions (http://www.creationkit.com/Category:Non-delayed_Native_Function). Since Utility.Wait is not on the non-delayed list, I am assuming it will always wait for at least one frame, so the goal of using 0.01 is just to avoid waiting for more than one frame (unless framerate is >100). In my tests it only ever took one or maybe two cycles of that loop before proceeding (either waiting for OnItemRemoved to fire, or for Is3DLoaded to be true). If your testing shows that it very often takes many loops, then the interval could be raised.

Hmmm, clever, but non-obvious. Hadn't seen this (0.01) argument before.... Yeah, folks do try 120Hz frame rates, but I've read there are problems.

Outside, Is3DLoaded always seems to take either 0 loops or many. That's why the backoff is such a good idea!

[BlackPete]

I received this papyrus error about a weapon rack script error.

[11/22/2013 - 05:10:37PM] error: (000FC865): does not have any 3d and so cannot be moved to.
stack:
[ (000FC86A)].ObjectReference.MoveToNode() - "" Line ?
[ (000FC863)].WeaponRackActivateScript.PlaceItem() - "WeaponRackActivateSCRIPT.psc" Line 435
[ (000FC863)].WeaponRackActivateScript.HandleStartingItem() - "WeaponRackActivateSCRIPT.psc" Line 395
[ (000FC863)].WeaponRackActivateScript.InitActivator() - "WeaponRackActivateSCRIPT.psc" Line 183
[ (000FC863)].WeaponRackActivateScript.OnUpdate() - "WeaponRackActivateSCRIPT.psc" Line ?

 

There is a similar one already in the tracker (issue #13882), but this one appears to be slightly different. Additionally, issue #13882 was supposedly fixed by 2.0.0. Should I add this one to the tracker as well?

 

Edit: issue #14209

[taleden]

I received this papyrus error about a weapon rack script error.

[11/22/2013 - 05:10:37PM] error: (000FC865): does not have any 3d and so cannot be moved to.

stack:

[ (000FC86A)].ObjectReference.MoveToNode() - "" Line ?

[ (000FC863)].WeaponRackActivateScript.PlaceItem() - "WeaponRackActivateSCRIPT.psc" Line 435

[ (000FC863)].WeaponRackActivateScript.HandleStartingItem() - "WeaponRackActivateSCRIPT.psc" Line 395

[ (000FC863)].WeaponRackActivateScript.InitActivator() - "WeaponRackActivateSCRIPT.psc" Line 183

[ (000FC863)].WeaponRackActivateScript.OnUpdate() - "WeaponRackActivateSCRIPT.psc" Line ?

 

There is a similar one already in the tracker (issue #13882), but this ones appears to be slightly different. Additionally, issue #13882 was supposedly fixed by 2.0.0. Should I add this one to the tracker as well?

 

Curious; there are numerous checks for the starting item to be fully loaded, but here it looks like it's the trigger marker that was not yet loaded. I suppose you could just add another CheckFor3D(), although I'm wondering why this hasn't been observed before -- or maybe it has and was just mixed up with the error about the item.

[taleden]

I just finished playing through Wyrmstooth and Falskaar (both are excellent, by the way), and discovered that the weapon racks in both mods' player houses (Fort Valus and Horndew Lodge, respectively) are wrongly configured: the activators are enable-parented to the room decorations X-markers, so the triggers cannot enable/disable them.

 

Of course the correct solution is for the authors to fix their racks, but I started tinkering with a workaround to cover these kinds of cases until they're fixed properly:

 

 

WeaponRackActivateSCRIPT.psc

Bool Property HasEnableParent = False Auto Hidden
Float Property MyEditorZ Auto Hidden


Function Disable(bool abFadeOut = false)
	If HasEnableParent == False
		Parent.Disable(abFadeOut)
		If IsDisabled()
			Return
		EndIf
		HasEnableParent = True
		MyEditorZ = Self.Z
		Debug.Trace("WARNING: "+Self+" failed to Disable() and probably has an Enable Parent set in the CK, which is a mistake! Tell the mod author!")
	EndIf
	SetPosition(Self.X, Self.Y, -32500.0)
EndFunction ; Disable()

Function Enable(bool abFadeIn = false)
	If HasEnableParent == False
		Parent.Enable(abFadeIn)
	Else
		SetPosition(Self.X, Self.Y, MyEditorZ)
	EndIf
EndFunction ; Enable()

Bool Function IsEffectivelyDisabled()
	Return IsDisabled() || (HasEnableParent && (Self.Z < -32000.0))
EndFunction ; IsEffectivelyDisabled()

WeaponRackTriggerSCRIPT.psc

	;If ActivatorScript && ActivatorScript.IsDisabled()
	If ActivatorScript && ActivatorScript.IsEffectivelyDisabled()

The idea is that, if the activator is not actually disabled after calling Disable(), then it probably has an enable parent; in this case, as an alternative to disabling it, just move it waaay down to the bottom of the coordinate space (what is the minimum possible coordinate, anyway?) where the player can't activate it. Then when it's time to re-enable, just move it back.

 

I originally used MoveToMyEditorLocation() rather than storing the original Z coordinate, but that didn't seem to work; I'm not sure why, unless maybe that method also reverts to the editor enabled-ness or something.

 

It's been working for me so far in the Wyrmstooth and Falskaar players homes, but of course there could be an edge case I haven't tested yet. The obvious failure mode is if some cell with a rack actually has player-accessible space below Z=-32000, which seems unlikely but is certainly possible. On the upside, even in that case, the activator would still (probably) be moved away from the placed weapon, so that it would still be possible for the player to retrieve that weapon; contrast to the current failure mode, where they lose access to the weapon because it's hidden behind the still-enabled activator.

 

So anyway, thoughts?

[Arthmoor]

So what happens if an author fixes their weapon racks but this script alteration has moved the activators?

 

I'm not sure if I'm comfortable with the idea of moving stuff like this around considering I've run into problems doing stuff like this and having Papyrus refuse to move the objects back to where they belong.

 

Also, if you do this in an outdoor cell, -32000 is far enough away to put it outside the rendering distance and may not register as being loaded when the player comes back again. You'd need to put in a safeguard to only let this happen in an interior.

[taleden]

So what happens if an author fixes their weapon racks but this script alteration has moved the activators?

 

If the rack is fixed, HasEnableParent would still be true so it would continue to use SetPosition() to "enable" and "disable" the activator; not ideal, but functional at least.

 

Edit: Note also that the activator cannot get stuck disabled, because HasEnableParent is only set while it is enabled (after failing to disable), and as soon as it's set, Disable() never again tries to call parent.Disable().

 

I'm not sure if I'm comfortable with the idea of moving stuff like this around considering I've run into problems doing stuff like this and having Papyrus refuse to move the objects back to where they belong.

 

Also, if you do this in an outdoor cell, -32000 is far enough away to put it outside the rendering distance and may not register as being loaded when the player comes back again. You'd need to put in a safeguard to only let this happen in an interior.

 

That's something I don't have much experience in, although I'm curious about the circumstances where a reference could be moved once but not back again; I would have guessed that it would fail the first time if it was going to at all.

 

And one could certainly put an interior check on the workaround, although I thought all player-activateable racks were in interiors anyway, so that wouldn't be much of a loss.

 

Also, if either of these things were to be a problem, I'm not sure it would actually be a worse problem than without this workaround; by default when the player uses a misconfigured rack, the activator stays enabled and masks the item so the player can never get it back without console help. If this workaround failed to move the activator back into place for any reason, at least the player would be able to retrieve their item, although they would never be able to place another one after that -- still seems like an improvement to me.

[Arthmoor]

Couldn't you set it up so that if it has an enable parent, it gets moved down, sets HasEnableParent, and then when it would get enabled again (moved back into position) set HasEnableParent to false again? That way if the rack is later fixed the shifting of positions isn't necessary anymore once it's been restored to its usual spot. Seems less risky that way if Papyrus decides it doesn't want to move it anymore.

[taleden]

Sure, I think that would work too. The tradeoff would be that the enable parent error and stack trace would appear in the log every time the player used the rack, instead of only the first time, but that's not necessarily a bad thing if it puts pressure on mod authors to fix their racks.

[DayDreamer]

First impression, very clever!

 

Second thought, next version of SKSE is going to have an enable parent test, so would be cleaner.

 

Third thought, I'm opposed. What we really want is fixes. So finding and making a patch is better, then giving the patch to the author.

 

Therefore, I'd say check for disabled success after disable. If not, then don't place the item. EDIT: instead, post a debug messagebox.

[Arthmoor]

Let's just cut out the SKSE option right now though - if the intention is to get this code into the USKP that's not an option. So while it would be cleaner and far more helpful, it would only be suitable for use in an external mod.

[DayDreamer]

Hey, it was you, Arthmoor, who asked for the SKSE feature. I assumed it would be for USKP.

[taleden]

Of course I agree that the preferred long-term solution to these problems is for authors to fix their racks. But I don't see a reason not to apply a workaround if it's possible to do so without negative side-effects, or at least without ones that are worse than the downside if no workaround is attempted, which is players losing access to their items.

 

Also, is enable parent data baked into saves? Or are we sure that as soon as an author fixes a rack, everyone's existing save will accept the change and let the rack work correctly?

[Arthmoor]

Hey, it was you, Arthmoor, who asked for the SKSE feature. I assumed it would be for USKP.

Don't assume

 

I asked for it after musing about how useful it would be for junk like the predator script. Even if the unofficials won't make use of it, someone surely will find it worthwhile to have.

 

Of course I agree that the preferred long-term solution to these problems is for authors to fix their racks. But I don't see a reason not to apply a workaround if it's possible to do so without negative side-effects, or at least without ones that are worse than the downside if no workaround is attempted, which is players losing access to their items.

 

Also, is enable parent data baked into saves? Or are we sure that as soon as an author fixes a rack, everyone's existing save will accept the change and let the rack work correctly?

Nope. Enable parent data isn't baked in. I fixed the Falskaar ones with a quick patch mod that I've sent ahead to Alex. Loaded up while standing inside the house and it worked fine. No more log errors.

 

It would have been a colossal pain if that stuff baked in because our fix to the predator script would have collapses on its face.

[asd]

[11/30/2013 - 11:48:40AM] Papyrus log opened (PC)
[11/30/2013 - 11:48:40AM] Update budget: 1.200000ms (Extra tasklet budget: 1.200000ms, Load screen budget: 2000.000000ms)
[11/30/2013 - 11:48:40AM] Memory page: 128 (min) 512 (max) 2000000000 (max total)
[11/30/2013 - 11:48:53AM] Cannot open store for class "dlc1scwispwallscript", missing file?
[11/30/2013 - 11:48:53AM] Cannot open store for class "DLC2BenthicLurkerFXSCRIPT", missing file?
[11/30/2013 - 11:49:00AM] warning: Property USKPWeaponRackExceptionList on script weaponrackactivatescript attached to (000FC871) cannot be initialized because the script no longer contains that property

 

It gives me that last line over 7000 times... ;c

[tyrel68]

got the exact same missing file bugs was kinda worried

[taleden]

Are you both using USKP 2.0.0a? Did you make double sure that it really is checked and enabled in your load order? Because that property sure looks present in the copy of the script I'm looking at, so I don't know why else the game would think it was gone. Even in the patch I posted to allow scripted items, I didn't remove that property, I just stopped using it for anything.