Jump to content
Sclerocephalus

God dammit Bethesda, or: why the Lakeview Manor weapon rack can't be fixed

Recommended Posts

my opinion for order to work on would be in the exact order you just listed bookshelf and traps would be the biggest priority imo

  • he bookshelf script
  • The trap scripts
  • The DLC1_NPCMentalModelScript (see: http://afkmods.iguan...almodelscript/)
  • The CWScript (sic, this has been found to throw various errors lately)
  • The Neloth scripts

 

The main reason id do bookshelves first is just because its the most useful one since it stores and displays. 

 

I am really hoping a complete rewrite is not needed since your herculian rewrite for the weapon racks

Share this post


Link to post
Share on other sites

 

Found a much better one:

Imagine you have an object reference 'Spawner' and need to choose a pair of random x/y coordinates at an arbitrary z angle and within a distance equal to or less than 'fLeashLength' from 'Spawner'. How do you calculate this ?

 

This is a function from the vanilla CritterDragonfly script, where someone is aimlessly trying to accomplish this:

Function PickRandomPoint()

	Float fLength = fLeashLength * 2.0

	Int Count
	Int MaxCount = 10


	fTargetX = 0.0
	fTargetY = 0.0

	While (fLength > fLeashLength) && (Count < MaxCount)
		fTargetX = RandomFloat (-fLeashLength, fLeashLength)
		fTargetY = RandomFloat (-fLeashLength, fLeashLength)
		fLength = Math.Sqrt (fTargetX * fTargetX + fTargetY * fTargetY)
		Count += 1
	EndWhile

	fTargetX = fTargetX + Spawner.X
	fTargetY = fTargetY + Spawner.Y
	
	; Now pick a random Z
	fTargetZ = RandomFloat (0.0, fHeight)
	fTargetZ = Spawner.Z + fTargetZ
	
	; Pick random target angle
	fTargetAngleZ = RandomFloat (-180.0, 180.0)
	fTargetAngleX = 0.0
	
EndFunction

Of course, he could have chosen a random z angle first (quickly rewritten by myself):

Function PickRandomPoint()

	Float fLength = RandomFloat (0.0, fLeashLength)

	fTargetAngleZ = RandomFloat (-180.0, 180.0)
	fTargetAngleX = 0.0

	fTargetX = Spawner.X + fLength * Math.Cos (fTargetAngleZ)
	fTargetY = Spawner.Y + fLength * Math.Sin (fTargetAngleZ)
	fTargetZ = Spawner.Z + RandomFloat (0.0, fHeight)

EndFunction

could it be a speed reason between the squared and sin/cos functions?

http://www.gamedev.net/topic/214140-cossin-or-sqrt-/

c++, but i guess basic math functions are handeled similarly in most languages.

 

+ it looks like ftargetangleZ is used outside the function to (probably) orient the object (dragonfly?) in the world (why else would it be calculated at the end of the function, with no further math involved?). in your function, isn't the variable then used to position and orient the dragonfly, making the spawn less random? (fixed orientation of the object relative to the orientation of the target point to the spawn point?)

 

and i can see the original scripter taking measure to not put function calls within additions. don't know why, but could be papyrus-stability related (judging by the pure fact of how easy papyrus breaks and throws stack overflows in modded games)

Share this post


Link to post
Share on other sites

If there really is a priority list of things to fix, I nominate the critter scripts simply because their spam is hugely annoying and strongly suggestive of continuing save game bloat being generated because they're not cleaning up properly.

Share this post


Link to post
Share on other sites

@Gruftlord:

 

The point here is that the task is mathematically very clearly defined, and there are simple relationships that yield exactly the result you look for, even with a randomization.  If you search for a random point on a circular plane with radius R, mathematics tell you that the conditions are fulfilled for all points with 0 <= alpha <= 2 pi, and 0 <= r <= R, where alpha and r are the respective point's polar coordinates in a two-dimensional system. Consequently, you select a random alpha and a random r between 0 and R and then calculate the Cartesian coordinates (x and y) by a simple transformation (the sine-cosine stuff).

 

It has nothing to do with mathematics when you select random x and y values between -R and +R (note that the conditions are inappropriate), then calculate r and repeat the procedure until either x and y meet the required conditions by chance or your count of unsuccessful trials has piled up to a certain limit. A total of 10 square root calculations and 20 randomizations, all with the risk of still not obtaining a valid result after all, do not save any performance compared to one sine and one cosine calculation.

 

Whoever wrote the script had simply no glimpse of an idea how to do this calculation properly. From this point of view, his result isn't that bad after all. What's bad, however, is that Beth leaves the scripting tasks to people who are everything else but experienced programmers.

Share this post


Link to post
Share on other sites

In my personal priority list, the trap and critter scripts have top priority. While the critters produce more spam, the traps have a more significant impact on gameplay. On position three are the Neloth scripts, mainly because they are performance killers (trying to do stuff by scripts that is better handled by a couple of game settings). There also is no real scripting task involved; we actually want to get rid of them, but it takes some work to find out in how many scripts the nonsensical code has been spread (there are at least three).

 

The Serana script has been left unfinished by Beth anyway, and fixing the remaining bugs will not account for the missing code which is impossible to reconstruct, so will have to live with the fact that we won't have to expect any significant improvements (but who knows, perhaps a few lines of her unheard dialogue could be brought to light nonetheless).

 

The bookshelves have lowest priority. While the bugs are significant, they don't occur too often, and people seem to have learned to handle them, as there are few complaints in forums.

Share this post


Link to post
Share on other sites

 

Yes, that's out of the question, but it doesn't matter when sine and cosine yield the expected result while the square root does not. There's a fundamental mathematical principle that relates this task with angular functions. You can't simply replace the terms in a formula and expect that you still get the same result. You don't.

Share this post


Link to post
Share on other sites

but sometimes a beautiful, precise script may not be the fastest. this game had to run on pretty old hardware (consoles)

if it would have been me i would have ditched the whole circle part of the idea and made it a square :-D

 

i just gave chrome a test with this. funny thing is: i dropped from 270m operations per second for both calculations to 13m for Sqrt and 5m for sine/cosine. guess chrome isn't the fastest browser for each task still :-D.

and: milage may vary heavily depending on the conditions (hardware, software). i'm just trying to defend the poor little scripter a bit, he probably had to work under more constrained conditions than we imagine.

Share this post


Link to post
Share on other sites

I'm by no means attacking the scripter. He tried to make the best out of what he knew. I'm attacking Bethesda.

 

You've based your comparison on the wrong assumptions: the goal isn't to perform the operations a given number of times; it is to obtain a valid result. To do this, my script version needs to run only once, since it is based on a mathematical-analytical solution, which means that I have derived the function to describe the relationship (believe me, there's nothing more elegant than logics). The vanilla script has to run a variable number of times; in the worst case it takes ten loops (but only because it is stopped there deliberately). Even then however, there's no guarantee that the result will be valid. If you want speed, replace the sine function in my script version by 1- cos^2, so you have only one angular function left. If you find a programming language that can handle virtual numbers, it might even be possible to improve it further.

 

Go one step further and have a look at the critter fish script, where our poor programmer tried to find a random point within a cone with his stratgey, but fails. In fact, his results can be within the cone only by chance, because his approach does not resolve to any relationship with less than two unknowns.

 

You can actually try yourself to solve the task with the square root approach, but this won't allow for a full randomization:

Every point on a circle with radius r can be defined unambiguously in any system of coordinates, but every system will require specific parameters. In practice, it's the application that determines which system is best suited. In a Cartesian system, the problem resolves to the following relationship: x^2 + y^2 = r^2. There is no unambiguous solution, because you have two unknowns, and the only thing you can do is choosing a random value for one parameter and calculating the other parameter from the resulting formula. However, our poor programmer chose to vary both parameters, although it is predictable that this equation won't converge.

Share this post


Link to post
Share on other sites

*sigh*

[10/12/2013 - 02:10:19PM] Error: Unable to call GetItemCount - no native object bound to the script object, or object is of incorrect type
stack:
    [None].TrainerGoldScript.GetItemCount() - "<native>" Line ?
    [None].TrainerGoldScript.CheckGold() - "TrainerGoldScript.psc" Line 22
    [None].TrainerGoldScript.OnDetachedFromCell() - "TrainerGoldScript.psc" Line 33
[10/12/2013 - 02:10:19PM] Warning: Assigning None to a non-object variable named "::temp3"
stack:
    [None].TrainerGoldScript.CheckGold() - "TrainerGoldScript.psc" Line 22
    [None].TrainerGoldScript.OnDetachedFromCell() - "TrainerGoldScript.psc" Line 33

Why is this none stuff becoming such a problem lately? And WTF, even trainer scripts are doing it now?

Share this post


Link to post
Share on other sites

Why does the script run on a "none" at all ?

 

I mean, it made its way through an OnCellDetach() event, then into the CheckGold() function and eventually, it gets aware that it's not attached to anything ?

 

EDIT:

Perhaps, Papyrus was finally replaced by something even more bizarre ...

Share this post


Link to post
Share on other sites
        elseif (!AelaCurrentQuest.IsActive)
            AelaCurrentQuest.Stop()
            AelaCurrentQuest = newRadiant
            AelaCurrentQuest.IsActive = True

:facepalm:

 

IsActive is a reserved native function name. They're using it here like it's a boolean variable. Tooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooodd!

 

Net result? Shit's not working the way this guy thinks it is, and the variable now needs to be renamed - with potentially far reaching consequences since it's buried in the core of the Companions radiant quest system.

 

The result is basically that the Companions radiant quests will cycle themselves up on every visit to Whiterun, and they have no regard for whether the quests are already running or not.

 

And hey, why not. Let's enshrine this one in a bug report: http://www.afkmods.com/index.php?/tracdown/issue/13958-companions-radiant-quests-all-cycle-without-regard-to-whether-theyre-running-or-not/

 

Cause CR03, CR06, CR07, CR10, and CR11 are all bloat bombs. 10 and 11 at least stop becoming available once the quest line is over, and 3 isn't available in vanilla, but 6 and 7 are, and they're as bloat happy as ever.

Share this post


Link to post
Share on other sites

While I appreciate the title "script sleuth", I think my rewrite is sufficient. I cleaned out the RegisterForUpdate(), replaced the OnUpdate() with OnUpdateGameTime(), removed what could be a potential perma-spinning error where the script's while loop required the game's time to be within certain parameters, and removed a number of spaces that made the script impossible to read for me.

 

That said, the lectures do work, but the old script seemed to have some problem with processing the function that would choose a scene too often, potentially resulting in a scene never being chosen. I think, though, a large portion of reports of folks not witnessing the lectures comes simply from misunderstanding: the script polls to choose a lecture between 12 PM and 2 PM, but then there will be a delay whilst the professor intended to give the lecture makes their way to the Hall of Attainment to actually begin speaking; the professor is not actually intended to begin walking to the lecture until 2 PM.

 

***

 

These none errors are odd, given that those properties are filled.

 

Sclero, I should like to address some of your concerns here:

 


After a look at that script, I decided to stop wondering. That's the biggest nonsense I have seen so far:
Scriptname DLC2NelothScript extends Actor 
ObjectReference Property DLC2TelMithrynTowerDoorRef  Auto 
Location Property DLC2TelMithrynTowerLocation  Auto 

Event OnEnterBleedout()
 StopCombat()
EndEvent
Event OnLocationChange(Location akOldLoc, Location akNewLoc)
 if akNewLoc == DLC2TelMithrynTowerLocation
;   debug.trace("NELOTH: unlocking door")
  DLC2TelMithrynTowerDoorRef.Lock(false)
 endIf
EndEvent
If they wanted him to stop combat when entering bleedout, why didn't they flag him as protected ? If they wanted him to be able to unlock the door to his own house, why didn't they give him a f***g key ?
 
He doesn't HAVE a key, he does not even have proper AI packages ... and while we're at it: why does he have a "JobFarmerKeyword" ?
 
And even more suprising: why did he change locations when his only package is a sandbox package at his editor location ? Are there any running quests that didn't properly terminate ?

 

Neloth does have proper AI packages, I don't know what you're talking about. He experiments on his apprentice and his sandbox package handles any mundane activities he might engage in (except sleeping, since according to Talvas, Neloth doesn't sleep).

 

Neloth leaves his tower on two occasions: to escort the player to and through the Dwemer ruin that houses the Black Book he's interested in, and when he examines the Earth Stone when the player first arrives on Solstheim.

 

Neloth is in the lock list for his tower, Tel Mithryn; when any NPC is the last NPC in that lock list to leave the cell, the doors are locked. Bethesda wanted to make sure that Neloth's leaving his tower wouldn't lock the player outside.

 

Neloth is an essential actor; essential actors fight until their health falls below 0, to a maximum of -30. From that point, their health will regenerate and, when it's positive, they will get up. Being KO'd does not prevent an essential NPC from being in combat, so this script makes sure that Neloth will drop out of combat if he is (somehow) reduced to no health. It has the added benefit of rapidly accelerating his recovery, as well, since essential NPCs have an additional health regeneration when not in combat.

Share this post


Link to post
Share on other sites
Neloth does have proper AI packages, I don't know what you're talking about. He experiments on his apprentice and his sandbox package handles any mundane activities he might engage in (except sleeping, since according to Talvas, Neloth doesn't sleep).

 

Since his sandbox package is linked to his editor location and thus would make him travel back to this location anyway once the package starts running, they should have bundled it with a "travel to" package with the "unlock doors on arrival" flag set. Then he would have a proper AI package, and any scripts to make him unlock his doors would become superfluous. Of course, he would need a key which he still hasn't (but what would be wrong with giving him a key to his own house ?) What can be handled at engine level (such as AI packages) runs much faster and with less impact on performance than any script.

 

Neloth is in the lock list for his tower, Tel Mithryn; when any NPC is the last NPC in that lock list to leave the cell, the doors are locked. Bethesda wanted to make sure that Neloth's leaving his tower wouldn't lock the player outside.

 

But those scripts don't handle Neloth leaving his tower. They handle him returning to Tel Mithryn. Thus, when he leaves, the player would still be locked outside until his return. What would make sense, by the way, if nobody else is in.

To make him unlock his door when he returns, just give him a key (he doesn't have one) and the engine will handle this (to make this double safe, see my reply above). Handling this from a script is a waste of performance (although it works).

 

Neloth is an essential actor; essential actors fight until their health falls below 0, to a maximum of -30. From that point, their health will regenerate and, when it's positive, they will get up. Being KO'd does not prevent an essential NPC from being in combat, so this script makes sure that Neloth will drop out of combat if he is (somehow) reduced to no health. It has the added benefit of rapidly accelerating his recovery, as well, since essential NPCs have an additional health regeneration when not in combat.

 

That's how all essential NPCs will behave, without scripts running on them. When the essential flag is set, the engine will handle this, and it will take them out of combat temporarily until they have recovered to a certain point. Whether or not they resume combat after that depends on the particular situation.

Running "StopCombat" on an actor does not really change this behaviour:

 

"The actor will stop fighting. However, if the actor is still aggressive and has low disposition towards any nearby actors, it may immediately go back into combat. This is most useful for stopping combat which was started artificially using StartCombat." (from the CK Wiki)

 

Thus, unless they wanted him to completely disengage from combat once his health dropped below a certain point (which would have required more than just running "StopCombat" on him), this script command is superfluous and a waste of performance.

Share this post


Link to post
Share on other sites
        elseif (!AelaCurrentQuest.IsActive)
            AelaCurrentQuest.Stop()
            AelaCurrentQuest = newRadiant
            AelaCurrentQuest.IsActive = True

:facepalm:

 

IsActive is a reserved native function name. They're using it here like it's a boolean variable.

 

Remains the question how they made it that it got compiled. To call the function, you must use the empty parentheses syntax, like so: "IsActive()".

Share this post


Link to post
Share on other sites

Remains the question how they made it that it got compiled. To call the function, you must use the empty parentheses syntax, like so: "IsActive()".

Well they weren't testing to see if it was active in the UI.

 

Seems as though Papyrus is fine with it because when I switched it to its own variable name to test, the script behaved exactly the same way (and then disrupted dozens of dialogue options because of the new name). In the end though I found a solution to the bloat bomb. A bit forced, but it works without disrupting whatever the hell is going on in that mess of a script.

Share this post


Link to post
Share on other sites

I assume by "that mess of a script" you're talking about the one where the dev has a comment saying that what they're doing is a mess and they shouldn't be doing that, but they're going to anyway? Looks like a bunch of cut and paste code?  Stuff like that makes me wonder what their QA procedures are.  Surely they're making enough money to do better...

Share this post


Link to post
Share on other sites

Pretty much that, yes. Stuff of legend:

 

; just in case some idjit (i.e. me) unblocked a dead questgiver

 

    ; lord, so much duplicated code here... this grew "organically" and is now hideous.
    ;  do not look here for examples of good tidy code.

 

            ; no current quest for whatever effed up reason, but got something in the
            ;  chamber, so fire.

 

            ; I don't even know why this is here anymore, but it is.

 

CompanionsHousekeepingScript is full of things like that. It's horrible.

Share this post


Link to post
Share on other sites

Remains the question how they made it that it got compiled. To call the function, you must use the empty parentheses syntax, like so: "IsActive()".

The compiler does little or no error checking. We learned that with value(less) returns in value returning functions. My guess is this is a relatively smart guy, who had heard of recursive descent compilers, but never formally took a class.

 

(OK, admittedly I didn't take the class either. In my case, because in my second summer I was hired on a project that required me to write a compiler, and compiler class was 4th year. So I basically skipped a couple of years. Besides, I was Honors College, Electrical Engineering, and Music Performance, so I wasn't required to take Computer Science prerequisites. But I have written compilers, plural.)

 

As far as reserved words, in creationkit.com they link to wikipedia for the definition. That is, they don't have a formally defined list of reserved words!

 

So IsActive can be a variable name. It can be different functions in different scripts. Etc. Context sensitive.

 

Confusing, but perfectly legal. The comments there are hilarious, in a macabre sort of way.

Share this post


Link to post
Share on other sites

The important thing at this point is that I defused another bloat bomb. Now we just need to deal with the next one that comes along. :P

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!

×
×
  • Create New...