Jump to content

Recommended Posts

Problem is, other than occasional notes, Sclero didn't post his code revisions. I suppose I could start over. Is this the next big thing we need?

 

EDIT: and I have a very different view of how things should be done. I'm interested in fixing things that are broken and spamming the logs. I'm not interested in looking at the whole thing and deciding to re-architect, as he wrote he wanted to do earlier. I'm a firm believer that's beyond the scope of USKP.

Share this post


Link to post
Share on other sites

I don't know if you could call it a vote, but yes, critter scripts need a whole bunch of attention to solve various bugs that may or may not still be leading to save bloat. Not to mention they generate a sea of Papyrus errors all on their own that dwarfs everything else the game does.

 

Bugfixing is obviously the goal, but I am not entirely against rearchitecting if that's what it takes to get there.

Share this post


Link to post
Share on other sites

First and foremost, I've compared the 1.9 scripts with the current state. For some odd reason, there are already some fixes for bird and fish -- but the exact same fixes are needed for DragonFly and Moth!

 

Secondly, there was a change from OnLoad to OnCellAttach. That's good and much more reliable. But means that a lot of places need to be fixed -- no guarantee the object 3D is loaded yet! That's going to be the biggest job.

 

Share this post


Link to post
Share on other sites

Looking at previous USKP attempts to fix FXfakeCritterScript, I see:

		if myContainerRef
			myContainerRef.disable()
			myContainerRef.delete() ; Added by USKP 1.3.0 - This was a PlaceAtMe object.
		endif

You've marked it for deletion, but deletion won't happen. The Ref hasn't been cleared, so it is temporarily persistent -- until the next time the critter is collected (which may be never)! So the bloat continues. You need:

			myContainerRef.disable()
			myContainerRef.delete() ; Added by USKP 1.3.0 - This was a PlaceAtMe object.
			myContainerRef = None; USKP fix persistent reference to deleted object

Second of all, this only happens after a long waitGameTime(hoursBeforeReset), and only a later OnCellAttach() -- which may never happen, because the player may never revisit the cell. So the bloat continues.
 
As I mentioned, BlueDanieru's clever fix for the bloat uses OnUpdateGameTime() instead. So it should clear without the player re-visiting the cell. But it's not backward compatible. Old references will still be waiting. Also, his fix has the same temporarily persistent problem.
 

In his Flora Respawn Fix, BlueDanieru also changed critters, added a switch from wait to an updategametime. I've looked at it and thought it wasn't done in a very backwardly compatible manner. But I'll include it here for you to gander:


To fully fix this problem is complex. We need to be backward compatible, handling OnCellAttach(). We need to clean out temporarily persistent variables. And we need to transition to OnUpdate behavior. Fortunately, events are callable:

;/*** USKP
;*	reduce save bloat
;*	onCellAttach pending readyToReset on old saves
;*	onCellAttach may not occur, as player may never re-visit
;*	credit BlueDanieru
/;
EVENT OnUpdateGameTime()
;~ 	debug.trace(self + "OnUpdateGameTime()")
	readyToReset = TRUE
	onCellAttach()
endEVENT

To clean out old disabled or deleted references onCellAttach:

    elseif !isDisabled()
        if myContainerRef && myContainerRef.isDisabled()
            ; USKP fix persistent reference to deleted/disabled object
            myContainerRef.delete()
            myContainerRef = None
        endif
        if myFloraRef && myFloraRef.isDisabled()
            ; USKP fix persistent reference to deleted/disabled object
            myFloraRef.delete()
            myFloraRef = None
        endif
        if myIngredientRef && myIngredientRef.isDisabled()
            ; USKP fix persistent reference to deleted/disabled object
            myIngredientRef.delete()
            myIngredientRef = None
        endif

Finally, to clean any existing Refs before adding new ones:

				if myContainer 
					if myContainerRef
						; USKP fix persistent reference to previous object
						myContainerRef.delete()
					endif
					myContainerRef = self.placeAtMe(myContainer, 1, false, true)

Share this post


Link to post
Share on other sites

FXfakeCritterScript test, put this in your Source (after backing up the vanilla), and compile. It seems to work for me, but I don't have any good bloated saves....

[old attachment deleted]

Share this post


Link to post
Share on other sites

Thanks for the heads up. FYI (although I guess DayDreamer at least has already seen this):

 

http://www.creationkit.com/Persistence_%28Papyrus%29

 

I thought using OnUpdateGameTime events for this was going to be considered harmful? Not by me, that is, since I use it in FloraRespawnFix...

 

You've marked it for deletion, but deletion won't happen. The Ref hasn't been cleared, so it is temporarily persistent -- until the next time the critter is collected (which may be never)! So the bloat continues.

Strictly speaking, I think it is better to say that "deletion" does happen, whatever that even means, but garbage collection doesn't, since there are still references pointing to it. Although it's really splitting hairs, since it seems like we've been using delete() as though it was free() which apparently isn't the case.

 

It's a pretty interesting thing. Usually a garbage collector just frees up objects if they don't have any references pointing to them, but in this case just because it isn't reachable from Papyrus, doesn't mean Skyrim.exe isn't using the object anymore. So, it seems what delete() really does is tell the executable to let go of the object, but then the Papyrus VM can still hold on to it as well, so you have to clear that out too. At least, that's my working theory - I may have missed something.

Share this post


Link to post
Share on other sites

Oh geez. So the reference REALLY can't be purged from the game without nulling it out first? That's cringeworthy, but as we don't have a cringe emote, these will have to suffice: :facepalm: :scared:

 

How many other scripts are lurking out there with this same kind of problem that we've assumed deletion was good enough for?

 

Oh, and I have some nice ripe old saves from 2011 that are no doubt loaded with bloat that could be thrown to the wolves for this one. Is the fixed script going to actually clear that bloat though if it can't deal with refs that already exist?

Share this post


Link to post
Share on other sites

Thanks for the heads up. FYI (although I guess DayDreamer at least has already seen this):

 

http://www.creationkit.com/Persistence_(Papyrus)

Bookmarked it a year ago, as I was working on my query thread over on the official forums about avoiding persistence.

It was essential to making Touring Carriages work and be removable. Had to actually clear the variables, and in the correct order, and worry about inter-script thread blocking -- or the driver/horse/wagon could mutually lock each other in memory. Good old dining philosophers....

I'm still not sure I've correctly solved all such problems there. It's a knotty multi-body situation. Much worse than here.

 

I thought using OnUpdateGameTime events for this was going to be considered harmful? Not by me, that is, since I use it in FloraRespawnFix...

Not by me! I think it's clever, as it cleans up the savegame even without the player re-visiting the cell site.

And in this case, it's great because the only thing the update makes temporarily persistent is the original disabled object, while cleaning up the temporary ingredients, etc. Much better than the vanilla, which uses long wait, holding the script in memory (and savegame) and making the ingredient variables persist.

 

Strictly speaking, I think it is better to say that "deletion" does happen, whatever that even means, but garbage collection doesn't, since there are still references pointing to it. Although it's really splitting hairs, since it seems like we've been using delete() as though it was free() which apparently isn't the case.

Oh geez. So the reference REALLY can't be purged from the game without nulling it out first? That's cringeworthy, but as we don't have a cringe emote, these will have to suffice: :facepalm: :scared:

How many other scripts are lurking out there with this same kind of problem that we've assumed deletion was good enough for?

I don't know, but I'm sure it's one reason we're getting so many Nones and missing forms. These are put into inventories and containers, where the original object reference is lost/unused. Then given another new reference as it's taken out again. But the old reference ("pointer" value) remains in these variables.

This is similar to the problem Talenden diagnosed with scripted objects and weapon racks.

 

Oh, and I have some nice ripe old saves from 2011 that are no doubt loaded with bloat that could be thrown to the wolves for this one. Is the fixed script going to actually clear that bloat though if it can't deal with refs that already exist?

Thanks. Sadly, some kinds are stuck:

(A) picking the same fake-critter twice over time. The 1st reference is lost, and the 1st disabled non-deleted object and its script are in the savegame forever. That's what's fixed in "Finally" (above), where I make sure it's marked for deletion before re-using the Ref variable. It doesn't complain about calling delete() twice.

(B) you never visit the cell again. The revised script still needs you to OnCellAttach to clear it. Only new pickings use OnUpdateGameTime.

Share this post


Link to post
Share on other sites

This is similar to the problem Talenden diagnosed with scripted objects and weapon racks.

Aha! That made me think of something else.... If the ingredient has been sold or consumed, it will have the deleted flag set already, and won't have a parent cell or 3D.

 

You know, I don't think the developers were worried about persistence and savegame bloat. It made the code so much simpler.

 

EDIT: Why did some earlier version of USKP stop doing ingredients? I couldn't find a match in the changelog?

Share this post


Link to post
Share on other sites

EDIT: Why did some earlier version of USKP stop doing ingredients? I couldn't find a match in the changelog?

What do you mean?

Share this post


Link to post
Share on other sites

BTW, that FXfakeCritterScript file isn't the main problem here. This file is used by the hawks and stuff. The critter problems being mulled over are for the insect spawners all over the world.

 

I don't have any significant bloat with hawks and salmon because I never spent a great deal of time caring about either one. I think I've shot down maybe a dozen, and half of that was testing our previous iterations of this script.

Share this post


Link to post
Share on other sites

What do you mean?

				if myIngredient 
					myIngredientRef = self.placeAtMe(myIngredient)
					myIngredientRef.MoveToNode(self, myLocationOffset)	
				endIf

They are commented out in USKP version. line-by-line in 2.0.0, as a block in mine. Wondering why?

 

But the Ref is still checked everywhere else.

 

 

BTW, that FXfakeCritterScript file isn't the main problem here. This file is used by the hawks and stuff. The critter problems being mulled over are for the insect spawners all over the world.

 

I don't have any significant bloat with hawks and salmon because I never spent a great deal of time caring about either one. I think I've shot down maybe a dozen, and half of that was testing our previous iterations of this script.

OK. Well, it's the first I looked at, and the problems were obvious. I've never shot a hawk, but I do harvest salmon in the early game.

Share this post


Link to post
Share on other sites

First and foremost, I've compared the 1.9 scripts with the current state. For some odd reason, there are already some fixes for bird and fish -- but the exact same fixes are needed for DragonFly and Moth!

 

Secondly, there was a change from OnLoad to OnCellAttach. That's good and much more reliable. But means that a lot of places need to be fixed -- no guarantee the object 3D is loaded yet! That's going to be the biggest job.

OK. I've compared them all line by line and duplicated the fixes from bird and fish and firefly into DragonFly and Moth. Ran around Riften and didn't get any errors this time.

 

The main thing I did was make 3 standardized testing functions and use them in all the same places.

 

Still haven't looked at any of the other issues that Sclero raised -- like bad code.

 

Are there other script fixes in the upcoming USKP pool that need to be merged with these?

 

[old code deleted]

Share this post


Link to post
Share on other sites

So how do birds start? critterBirds is the only one that doesn't have an enable() in its onStart() code.

 

UPDATE: the answer is the Critter KickOffOnStart OnUpdate has an enable at the end, after the OnStart() call. But that enable is done after Motion_Keyframed, which in general requires 3D to work.

 

For all other critters, the critter enable is redundant.

Share this post


Link to post
Share on other sites

Trying to debug http://www.afkmods.com/index.php?/tracdown/issue/14602-critterfishonupdate-assigning-none-as-mudcrab-is-killed/

 

Each fish is pointing at another to emulate schooling. If the leader is killed and deleted, the pointer isn't good anymore (but still there as a deleted object), and tosses a pile of exceptions every time it's touched.

 

This bad design is prevalent in many critters. Birds flock. Moths congregate around flora that can be harvested.

 

What we need is a dependent critter property and a generic object pointer property. If something is pointing at another, it should register with the other. Then, the other's DisableAndDelete function can clear the dependent's property.

 

However, this would allow only 1 dependent. Hopefully schooling and flocking will work as a chain, instead of follow the random leader. Otherwise, it will have to be an array of dependents. But I'll try 1 dependent first!

 

UPDATE: It seems to work for Fish to Fish. And FireFly and Moth, which both use plants (I don't yet have the flora knowing about it and cleaning up). Birds don't really flock, they use the player as a focus. DragonFlies use any actor, without saving the reference.

 

It won't fix existing saves, but it's only a warning message....

Share this post


Link to post
Share on other sites

Next problem, easily tested by walking outside Honeyside and back inside. A huge pile of errors, leaving at least 2 non-deletable bloat objects for every critter. This happens for every onCellDetach():

[01/23/2014 - 02:10:15PM] Error: Unable to call Delete - no native object bound to the script object, or object is of incorrect type
stack:
    [None].ObjectReference.Delete() - "<native>" Line ?
    [ (FF000DC0)].critterMoth.DisableAndDelete() - "Critter.psc" Line 339
    [ (FF000DC0)].critterMoth.OnCellDetach() - "critterMoth.psc" Line 320
[01/23/2014 - 02:10:15PM] Error: Unable to call Delete - no native object bound to the script object, or object is of incorrect type
stack:
    [None].ObjectReference.Delete() - "<native>" Line ?
    [ (FF000DC0)].critterMoth.DisableAndDelete() - "Critter.psc" Line 343
    [ (FF000DC0)].critterMoth.OnCellDetach() - "critterMoth.psc" Line 320

These are the landingMarker and dummyMarker, using in the Translate routines. Sure enough, the code just before it doesn't stop translations in exactly this case (going inside, where the parentCell detaches):

    if (GetParentCell())
        StopTranslation()
    endIf

According to notes at http://www.creationkit.com/TranslateTo_-_ObjectReference "deleting havok objects or changing their motion type mid-translation may cause a CTD."  So nothing will delete (they are in use by the translation code), and probably causes a fair number of our unexplained CTD!

:wallbash:

 

The answer I'm testing gets rid of the OnCellDetach(). So far, I'm finding that DragonFlies gradually delete themselves after their translate routines complete.

 

"USKP 1.3.3 added this because Fireflies never delete once cells detach, potentially building up for ages." Obviously, I'm going to test that issue. But I think it was the wrong solution.

Share this post


Link to post
Share on other sites

has all the stuff from steve40 been looked into allready?

 

http://forums.nexusmods.com/index.php?/topic/981701-critter-script-bugfix

Thanks, I wondered where that came from.... Yes, there are plenty of lines indicating STEVE40. But the dates are older than his most recent. So I'll take a look and compare.

 

UPDATE: Looks like I've been duplicating some of his work. I'd already discovered that USKP hadn't done anything to DragonFly and Moth, and I've applied the same techniques to them. He mentioned that problem in: http://forums.nexusmods.com/index.php?/topic/981701-critter-script-bugfix/page-5#entry8498833

Share this post


Link to post
Share on other sites
				if myIngredient 
					myIngredientRef = self.placeAtMe(myIngredient)
					myIngredientRef.MoveToNode(self, myLocationOffset)	
				endIf

They are commented out in USKP version. line-by-line in 2.0.0, as a block in mine. Wondering why?

 

IIRC that's never actually used in the game. Maybe that's why, although it seems better to keep it in, IMO.

Share this post


Link to post
Share on other sites

I don't actually know why that was done. Probably a holdover from before I joined the project. Could be it somehow interferes with normal flora procedures and was blocked because of that.

Share this post


Link to post
Share on other sites

I don't actually know why that was done. Probably a holdover from before I joined the project. Could be it somehow interferes with normal flora procedures and was blocked because of that.

Wasn't done as of 1-0-5c, or even 1-2-6, was done in 1-2-7 -- the explanation doesn't match:

 

FXfakeCritterScript.psc does not reset its doOnce variable when the scripted object resets itself. (Bug #9882)

Share this post


Link to post
Share on other sites

The changelog entry for bug 9882 tells you what *WE* did to it, not what someone before us did to it. Blue says it's not used. Perhaps Kivan thought the same. If it's not going to interfere with something then I'd just as soon uncomment it and put it back into service but I'd like to be sure it isn't going to cause problems related to flora harvesting first.

Share this post


Link to post
Share on other sites

Actually, I was wrong. It looks like it is used by Hearthfire for the waterfall salmon, for the roe. But, I don't think anything should be done with ingredients in the OnHit() event handler, only OnActivate(). For OnHit(), the script will place the dead salmon (DeadFXSalmon0X) which already give the roe. The vanilla script seems to actually place the ingredient directly in the world, in addition to the flora object (i.e. DeadFXSalmon0X), which shouldn't be happening. So, the MyIngredient property is used in the OnActivate() handler, and needs to be used; and MyIngredientRef is used in vanilla in the OnHit() handler, but should not be, since those are added directly to player inventory (via the flora or container object placed in the OnHit() handler).

 

Here's what I'm going to put in FloraRespawnFix in a few days:

Scriptname FXfakeCritterScript extends ObjectReference  
{Make fake critters shootable}

container Property myContainer Auto
Flora Property myFlora Auto
ingredient Property myIngredient Auto
potion Property myFood Auto
string Property myLocationOffset Auto
string Property myFakeForceExplosionOffset Auto
Explosion Property myExplosion Auto
Explosion Property myFakeForceExplosion Auto
objectReference myContainerRef
objectReference myIngredientRef
objectReference myExplosionRef
objectReference myFakeForceExplosionRef
objectReference myFloraRef
int Property numberOfIngredientsOnCatch Auto
int property hoursBeforeReset = 72 auto

Event OnActivate(ObjectReference akActionRef)
	self.disable()
	if myFood 
		game.getplayer().additem(myFood, numberOfIngredientsOnCatch)
	endif
	if  myIngredient
		game.getplayer().additem(myIngredient, numberOfIngredientsOnCatch)
	endif
	RegisterForSingleUpdateGameTime(hoursBeforeReset)
EndEvent

Event OnHit(ObjectReference akAggressor, Form akSource, Projectile akProjectile, bool abPowerAttack, bool abSneakAttack, bool abBashAttack, bool abHitBlocked)
  if akAggressor == game.getplayer()
    if myContainer 
      myContainerRef = self.placeAtMe(myContainer, 1, false, true)
      utility.wait(0.01)
      myContainerRef.enable(false)
      myContainerRef.MoveToNode(self, myLocationOffset)	
    endIf
    if myFlora 
      myFloraRef = self.placeAtMe(myFlora, 1, false, true)
      utility.wait(0.01)
      myFloraRef.enable(false)
      myFloraRef.MoveToNode(self, myLocationOffset)	
    endIf
    utility.wait(0.1)
    self.disable()
    RegisterForSingleUpdateGameTime(hoursBeforeReset)
  endIf
EndEvent

Event OnUpdateGameTime()
  enable()
  if myContainerRef
    myContainerRef.disable()
    utility.wait(0.1)
    myContainerRef.delete()
    myContainerRef = None
  endif
  if myFloraRef
    myFloraRef.disable()
    utility.wait(0.1)
    myFloraRef.delete()
    myFloraRef = None
  endif
endEvent

I've taken out the cleanup for MyIngredientRef from the update event handler, since that's never used anyway, and nulled references to any previously-placed objects. Otherwise, same as before.

 

I did notice another thing though, when looking at this, in the USKP and UHFP. For the waterfall salmon, in the unplugged game (without Hearthfire even) the meat is added via script when killing then harvesting. USKP changes this to adding via the ingredient field for FLOR objects. This is fine, although AFAICT it doesn't make much difference either way. For the UHFP, the ingredient field is changed to BYOHSalmonRoe01, so the player gets the roe instead. However, both the meat and the roe are already added via script (and, indeed, they need to be added via script, since the player is supposed to get two items and you can add only one by using that field). So, if you kill waterfall salmon and then harvest them, you'll get an extra roe - once from the script, and once from the field. That field should be put back as None in UHFP, as it is in vanilla Hearthfire. Then the player will get one of each.

 

If you directly harvest the salmon instead, without killing first, everything appears to work fine I think, because the DeadFXSalmon0X objects never gets placed by fxfakecritterscript anyway.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Support us on Patreon!

×