Jump to content

Photo

Script bug and optimization questions

papyrus scripting bug

  • Please log in to reply
19 replies to this topic

#1
xWilburCobbx

xWilburCobbx

    Enthusiast

  • Members
  • PipPip
  • 173 posts

My mod is nearing completion so now I am doing some script touch ups before I go all out, place it around the game world, and ship it. I have a couple of unrelated problems, answers to ANY of them would be appreciated.

 

The entire script:

 

Spoiler

 

(Problem 1: Bug)

 

I can't reproduce the bug, but just encase, I updated the script. The trigger originally only disabled it self on line 39:

if aiButton == 0
                self.Disable() ;Stops player from spamming activate
		PlayerREF.RemoveItem(mCoin, fCost as int)

This was sufficient, but then the bug happened and I don't know why! So now I just disable the trigger immediately and manually re-enable it when necessary.

 

The bug only happened once and I don't know if anyone is familiar with it. The script is ran via an activatable trigger, once activated it shows either a Y/N purchasing menu, or a menu that just tells you that you can't afford it (It does this by comparing a value set in the properties). I was messing around and spamming wait, I would wait an hour at a time (Its important for the mod. It gives money every hr on update) till I had enough money to get the purchase message.

 

I immediately activated one of the triggers as soon as I finished waiting, and it activated the purchase message twice in a row! The first one made the purchase and took the money required for it, the second one took a completely random amount of money (it was supposed to be 5,000 gold as defined in the script properties, it only took 499) and the rest of the values set in the properties processed normally.

 

Did I perform the proper fix for this strange bug? Is this just a fluke that happens in the papyrus engine every once in a while? Or is there an issue with my script that I just don't see?

 

(Problem 2: Functions)

 

I am getting familiar with using functions, I don't typically make them unless I need to run/reference a particular piece of the script multiple times, or remote call a specific set of processes from another script.

 

"Function DeleteAll()" is the only one that has a process that runs once and doesn't need to be remote called for any reason. Is it a good idea to split up the script into blocks of functions that do specific things? Or should custom functions only be used for re-runs/remote calls?

 

I feel like splitting them up is cleaner then just having a stack that runs on and on like this:

Spoiler

Edited by smashballsx88, 31 July 2020 - 06:42 AM.


#2
testiger2

testiger2

    Fan

  • Members
  • PipPipPip
  • 361 posts

instead of self.Disable() use a boolean toggle or use states

something like this would work

boolean lock example:

bool bLocked

Event OnActivate()
	if !bLocked
		bLocked = true ; lock OnActivate() to prevent spamming
		
		; rest of code goes here
		; use bLocked = false to unlock
	endif
EndEvent

About the function question:

 

Of course using functions has its pros and cons but most of the time it is perfectly fine to use functions even when they are only called once in your script.

 

A few situations come to mind where you might want to avoid them:

- you need to keep the string count down (every function increases the string count in the save) should be of no concern in most cases unless you add hundres or throusands of functions with your scripts

 

- avoid overhead

there may be times where you have resource intensive tasks (while loops for example) where you are better off not using functions.

Generally speaking function statements increase the amount of instructions the cpu has to process (start/exit adress, function parameters etc. google function overhead if you want an in depth explanation)

This is normally not a problem for bigger functions but on small functions the overhead may even outweigh the actual function and you are better of just using a direct statement if speed is of concern (keyword micro-optimization)

 

The Papyrus engine is also fps-linked. That means that per thread a maximum of 1 function call per frame can be made.

 

 

So to sum it up:

Your example should be fine using functions.

You can avoid functions where speed is priority but in speed is that much of a priority you most likely will find other bottlenecks in your code before this one.

And don't forget that you also have to read your code or may want others to read your code. One function line is often easier to read that big a block of text (in your script for example)

 

 

 



#3
xWilburCobbx

xWilburCobbx

    Enthusiast

  • Members
  • PipPip
  • 173 posts
Thank you for taking the time to answer my questions!

1)
I can switch to using a bool, seems like it would be faster and cleaner.

My only concern is I don't know if setting a variable outside a function will set it for every instance of that script ran by the specific object.

So lets say the first run sets bLock to true, but it bugs out and runs OnActivate twice. Will the second one read true and stop?

2)
I went ahead and just eleminated functions that weren't necessary, and just space out and comment on blocks of code to organize it. I decided that unecessary functions don't provide much benefit for its added slowness, and my new method is just as organized, if not more so.

Edited by smashballsx88, 01 August 2020 - 10:10 AM.


#4
testiger2

testiger2

    Fan

  • Members
  • PipPipPip
  • 361 posts

1) You already have Auto Properties in your script. You Can treat Auto Properties as variables with outside accsess. Meaning they provide the same functionality as variables but can be accessed by other scripts or the CK.

 

I think you misunderstand the way the instancing works

When you attach a script to an object it receives one instance of the script but not multiple.

When you create multiple instances of said object they each get a independant instance of the script.

 

When you happen to spam the activate button there are not multiple instances beeing called but the same instance calls the OnActivate event multiple times. This is why you want to use a lock variable

 

- user spams button

- 1st iteration of OnActivate gets called, the following are queued

- 1st iteration sets lock = true and proceeds

- 2nd, 3rd, etc iteration enters OnActivate and sees lock == true -> skips locked code and leaves ( does nothing)

- 1st iteration finishes and sets locked = false

 

2) The examples where you want to eliminate functions i gave you are very specific cases and may have been a bit misleading.

It is best-practice to keep your code in small logical functions that have a specific task. Your BuyMenu and Deletall functions were totally fine in that regard.

When i mentioned the overhead on small functions i meant the ratio between the function statement and the actual code.

So while very short functions may produce more overhead in relative terms, very large functions still produce more overhead in absolute terms.

 

Again this falls under the case of mirco-optimization and you should worry about the functionality of your code befor this.

 

There is a phrase in programming saying: premature optimization is the root of all evil



#5
maxarturo

maxarturo

    Faithful poster

  • Members
  • PipPipPipPip
  • 1,855 posts
Just to elaborate on testiger2 post, that is well explained.
 
"So lets say the first run sets bLock to true, but it bugs out and runs OnActivate twice. Will the second one read true and stop?"
There are circumstances where the PC may be too busy and / or it will fire twice something (the infamous weird game engines issues).
 
One example which i have personally experienced is with a 'Global Variable' not changing because the system was way too busy at the time of the call and it just jumped it / skipped it, this f**** up everything, the whole quest / mod !!!.
* Just one Gloval Variable...
 
To eliminate any of those possibilities been caused by the game engine / PC system, you need to use 'States', since changing a script's 'State' is one of the fastest functions there are, and they are the most reliables.
 
Using testiger2 script example i'm adjusting to states.
Auto State WaitingPlayer
Event OnActivate()
      GoToState("Activated") ; lock OnActivate() to prevent spamming
 
      ; rest of code goes here

      GoToState("WaitingPlayer")  ; to unlock OnActivate()
endif
EndEvent
EndState
 
State Activated
Event OnActivate()
      ; Do Nothing
EndEvent
EndState

Even if the 'OnActivate()' fires twice the second OnActivate it will ALWAYS fire in the "Empty State".


Edited by maxarturo, 01 August 2020 - 08:12 PM.


#6
xWilburCobbx

xWilburCobbx

    Enthusiast

  • Members
  • PipPip
  • 173 posts
1)
Yeah I was suspicious of that. I wasn't sure if the script was just 1 instance that can run multiple times, or if it just re-initiated everytime it ran.

Now I know to use that to my advantage, and using a bool makes a lot more since to me.

2)
I see, guess I jumped the gun on that one. As you can probably tell, I am no programmer, but toddlers can make papyrus scripts so I am comfortable with it.

I am just now learning some basic concepts of programming, and my biggest concern after many projects I tossed, is going beyond just functionality.

I want to make sure my scripts wont slow the game down too much, especially when other mods are doing stuff. I am trying to make everything as clean and professional as possible, so in the future I have the skills to write good scripts that behave.

I have dumped a lot of research to ensure my current scripts do whay they need to do in the shortest way possible, and keep them from save bloating.

#7
xWilburCobbx

xWilburCobbx

    Enthusiast

  • Members
  • PipPip
  • 173 posts

Just to elaborate on testiger2 post, that is well explained.

This explains a lot! Yeah I am not too familiar with the bugs of the papyrus engine itself.

I can always make it use a state, I will read documentation on it and touch up my knowledge of states before I just start using them.

I do have portions of other scripts that take data from some globals. I am not on my PC right now, but it is a function that gets called by this very script.

IncQS.UpdStatVal()

It takes values from 2 globals, runs a math operation with them and stores the result in a 3rd global just meant for text replacement.

Should this go in a state? I feel like it would be pointless to switch that into a state, and extremely problematic as the script has other functions that need to be called when it updates.

Edited by smashballsx88, 01 August 2020 - 06:33 PM.


#8
maxarturo

maxarturo

    Faithful poster

  • Members
  • PipPipPipPip
  • 1,855 posts
"Should this go in a state? Or states mostly good for stopping the script from re-running something while its already running? I feel like this is already prevented by putting the OnActivate() in an empty state."
That depends on what your script does and how it does it, and how busy the system is at that time.
 
States can be use to "for stopping the script from re-running...", but they have multiple functions.
 
One of the primary uses / functions of a 'State' is to make a script (the same script / the same activator) do something else when you re-activate it.
Example:
Auto State Sequence01
Event OnActivate()
          ......Do something
          ......Do something else
 
Now we want the same script / activator to do something else the next time the player activates it
 
          GoToState("Sequence02")
EndEvent
EndState
 
 
 
Next time the player activates this it will run the bellow
 
State Sequence02
Event OnActivate()
          ......Do something
          ......Do something else
 
Now we want the same script / activator to do the functions on 'Sequence01' or jump to an other "SEQUENCE"
 
          GoToState("Sequence01")  ;Or the next sequence
EndEvent
EndState
 
If you need more info on this or in-game functionable scripts as examples, don't hesitate to ask for it.
 
Have a happy modding.

Edited by maxarturo, 01 August 2020 - 06:45 PM.


#9
xWilburCobbx

xWilburCobbx

    Enthusiast

  • Members
  • PipPip
  • 173 posts

That depends on what your script does and how busy the system is at that time.

Yeah I got a little excited and skimmed through the concept of a state on the wiki, I will make sure to read it thoroughly when I go off work for the next day or 2.

I can see so much potential for this! Other then preventing my strange bug, I don't have a use for it else where quiet yet. Especially since Function UpdStatVal() needs to be ran by this script and it's home script.

Luckily if UpdStatVal() is fumbled, its totally fine as long as it is ran again, its not serious.

gTotalPaid.Mod(fCost) is in the script I showed you, and its the absolute most important one that CAN NOT fail under any circumstances.

Edited by smashballsx88, 01 August 2020 - 07:04 PM.


#10
NexusComa2

NexusComa2

    Journeyman

  • Members
  • Pip
  • 43 posts

What bug? ... You never told us what it is saying or doing. You just keep saying "the bug".
 

As far as disabling that don't sound wise. Let me introduce you to one of my favorite flags.

It is called: First Time Flag. I'll use an int here to show you but you could also use a bool.
I like to use a int as it gives me the choice of using it for a multi flag. if > 0 ... if  > 1.
But the concept remains the same. This little flag is useful in so many ways.
One of them is being able to totally stop spamming.

 

Int ftf = 0

 

Event Whatever()
 If(ftf ==(0))

   ftf =(1)
 

   bla bla bla
 

   ftf =(0) 

 Endif
EndEvent

In your case: 
 
int ftf = 0

utility ;For the wait command. I usually don't use a wait with this but in this case it may be helpful.

Event OnActivate(ObjectReference akActionRef) ;Checks to see if the player has the money and shows the menus
 if ftf == 0
  ftf = 1
  if PlayerREF.GetGoldAmount() >= fCost
   BuyMenu()
   wait(2.0)
   ftf = 0
  else
   mMenuFailP.Show(fCost)
   wait(2.0)
   ftf = 0
  endif
 endif
EndEvent

 


Edited by NexusComa2, 02 August 2020 - 01:58 AM.






Also tagged with one or more of these keywords: papyrus, scripting, bug

Page loaded in: 1.135 seconds