[RFC] SLua event handling proposal #3
Conversation
|
The proposal looks good on an initial skim! If I'm reading it right, there's support multiple timers in one script (awesome). What about |
It might, but we don't have plans for it at present. The implementation for We'll have follow-on proposals for other things like sensors and async task management. |
|
Edit: I'll add these comments inline on the PR snipJust a check, the concept implementation makes use of If the actual implementation makes use of this, does that mean that Afaik all the beta grid SLua sims are running on one machine, so it's not possible for me to test if it remains consistent. The luau documentation states that For my own slua multi timer implementation I used |
| function LLTimers:on(seconds: number, handler: EventHandler): LLTimersProto | ||
| assert(seconds > 0) | ||
| table.insert(self._timers, { | ||
| nextRun=os.clock() + seconds, |
There was a problem hiding this comment.
Is os.clock consistent across hosts? Luau doesn't define a baseline for the time it returns only that it increments in seconds.
This would cause problems if it is for instance based on host uptime or something similar, and timers are being set on different hosts as the script travels/teleports from region to region.
This is of course only a problem if os.clock isn't cross host consistent, and the actual implementation uses it.
There was a problem hiding this comment.
The actual implementation won't rely on os.clock() being the same across hosts (though it should,) see https://github.com/secondlife/issues/pull/3/files#diff-03b63dc9a26d2a3d82b27fea35bfd7acb64b7681fdf1a5d877a096db5dadea3cR174-R176 . In the actual implementation, when the script state gets serialized we'll turn it into a timestamp relative to the current os.clock(), then the far end will treat the timestamps as relative to the current time at the time of deserialization. Timers only tick down while objects are in-world, not when they're in transit or inventory, so we have to do it that way.
There was a problem hiding this comment.
Ahh... it does help if I carefully read it all completely, sorry for wasting your time, in my head relative to os.clock was still a problem.
There was a problem hiding this comment.
No, absolutely not! Thanks for thinking about these things!
There was a problem hiding this comment.
You bring up a good point, actually, with regards to ll.GetTime(), since that's the natural / correct way to write this.
The reason I try to avoid ll.GetTime() as much as possible is because of its poor output precision (32-bit float) and the fact that it can experience significant drift in as little as 4 hours due to the precision of the underlying timer. I only used os.clock() because there isn't a timer for seconds that a script has been "alive" like ll.GetTime() that doesn't suffer from those drift problems. I'll see if we can come up with a better API for that.
There was a problem hiding this comment.
In general a global table for "meta" functions/values would probably be useful in the future. Something that can contain metadata about the script itself and meta functions.
type llscript = {
compileTime: number, -- Unix timestamp of script compile
startTime: number, -- Unix Timestamp of script start (resets with ll.ResetScript)
getUpTime: () -> number. -- Like ll.GetTime (without the issues and no reset function, except ll.ResetScript)
bytecodeSize: number, --- The size of the bytecode
...
}Just some ideas
| -- Event handlers which stack multiple events together in LSL via an integer `num_detected` parameter | ||
| -- TODO: Pass in a special wrapper object for these functions and loop through `num_detected` rather | ||
| -- than actually pass in `num_detected`? Seems like almost nobody uses these functions as | ||
| -- intended since it's annoying to have to write the loop, everyone just does `llDetectedWhatever(0)` | ||
| -- and silently drops the rest of the queued events of that type on the floor. |
There was a problem hiding this comment.
Could this be elaborated on some?
In general I agree that the grouping of some events, is an unintuitive pattern, that is likely the result of resource management from the past.
But
Does this TODO: Pass in a special wrapper object for these functions mean that it would ALWAYS be done for the scripter, or is it more of a TODO: implement supporting this?
If this were to be implemented as the new standard it would need careful documenting else we will end up with people still doing ll.DetectedKey(0) but now having it run 10 times or something.
It would also need to pass in the current "event index" to allow scripters to use the correct data via ll.Detected...
Or a rework of how the ll.Detected... functions work, so as to not need an index, and pretend that that system doesn't exist. Where in reality something equivalent to the code below is executing, but the scripter only sees the bit after USER CODE themselves.
local LL = ll;
ll = setmetatable({}, {__index=ll});
(function()
local detectedIndex = nil
ll.SetDetectedIndex = function(index:number) detectedIndex = index end
ll.DetectedKey = function(num:number)
return LL.DetectedKey(num or detectedIndex)
end
end)()
function touch_start(events : number)
for i=0,events-1 do
ll.SetDetectedIndex(i)
touch_handler()
end
end
-- USER CODE
touch_handler = function()
ll.OwnerSay(`Touched By: {ll.DetectedKey()}`)
endA "clearer" alternative might be to add new "pseudo" events, named something like each_touch_start or each_touch etc but that smells in other ways.
There was a problem hiding this comment.
is it more of a TODO: implement supporting this?
Sorry about that, yeah, all TODOs are things that need to be fleshed out (or decided against) for this RFC.
Or a rework of how the ll.Detected... functions work, so as to not need an index, and pretend that that system doesn't exist. [...]
Yeah, we definitely want to get rid of ll.Detected*() if we can, since the usability story isn't great there.
I guess we're worried about the usability angle. It's not obvious that ll.DetectedKey() and friends are functions that you should think about calling in that context, or that something like ll.DetectedVel() is something that will work in some contexts but not others, and that their output is going to be nonsense if you try to use them in a coroutine once execution is no longer happening under the touch_start() handler.
Then you have events like sensor where you want want to handle all those detected items as a single batch, since you might want to select the closest object matching certain criteria or something.
My thinking was that there could be wrapper object that store an index internally, and that have methods like event:getKey() or event:getVel() and friends that get marked as invalid when execution leaves the relevant event handler.
These event objects would be inexpensive memory-wise (is_valid bool + event number index) so no resource worries there, just not sure what makes sense. Passing in a table of these event objects?
There was a problem hiding this comment.
One idea could be returning results in tables for events like sensor and collision where there are routinely going to be multiple results.
The various Detected types could be stored in the list as a way to get rid of the Detected functions. Then llDetectedKey(0) becomes something like result[1].key.
This would not be as memory-efficient per event object though, so it has its drawbacks. Ease of use versus resource consumption.
There was a problem hiding this comment.
Beware, some scripts call llDetected*(0) without a proper loop when they really should be looped (discarding incidents that built up during a sleep or forced delay), while some scripts deliberately accept only the first input and then discard the rest. If touch*, collision*, sensor and other events that provide values for the llDetected* functions are restructured to call a user-defined function (handler) once for every incident, then SLua must not pay additional forced delay time for each time the UDF is called within what would be a batch in LSL. The script should drop further UDF calls if removed from the event before a batch is finished.
Scripters can always implement their own wrappers in SLua. Time will tell whether this is actually necessary. Some will do it out of habit, but I think most will take the path of least resistance. I have never considered it annoying to wrap llDetected* functions in a loop; batching reduces the number of events in the queue, and fewer events in the queue means a more responsive script.
There was a problem hiding this comment.
Yeh the more I think about this, the more it feels like this should be a new event, and the old events should be marked as "deprecated".
I also like Kristy's idea of "clicked"
Maybe we can have something like this instead
local handler = function(event: TouchedEvent)
ll.OwnerSay(lljson.encode(event)) -- {"type":"start","key":"<uuid>","link":1,"uv":"<0.5,0.5,0.5>",...}
end
LLEvents:on("touched", handler)type should probably be a number/enum, but I used a string for clarity
This could later be expanded to allow for filtering...
LLEvents:on("touched", handler, {type="end", key=ll.GetOwner()})
| end | ||
|
|
||
| function LLTimers:on(seconds: number, handler: EventHandler): LLTimersProto | ||
| assert(seconds > 0) |
There was a problem hiding this comment.
I think this (and also the once version) should not assert and instead trigger the off behaviour to match what llSetTimerEvent(0) does.
This way we can use
llTimers:on(CalcMyNextTimer(), myHandler)instead of:
local next = CalcMyNextTimer()
if next > 0 then
llTimers:on(next, myHandler)
else
llTimers:off(myHandler)
endThere was a problem hiding this comment.
We probably won't, it would be surprising for new scripters to have a method called :on() that sometimes unsubscribes from an event. This would also be in keeping with the EventEmitter semantics that people are probably used to.
is adding a In my scripts I use Having a simple Though, that only covers the touch part of things, collisions are different beast... Side note: maybe SLua having few extra handy events, compared to LSL, would encourage more creators to update their scripts or adopt new stuff faster to take advantage of the new features. |
|
Some ideas... Could we have states?
"num_detected" events
LLEvents:off / LLTimers:off
LLEvents:once and LLEvents:off
LLEvents:listeners
And two questions to know how to teach the changes to the students:
Depending on it i will teach the changes to the students before LLEvents arrives (so the students know what to do when their scripts stop working) or after LLEvents arrives (if the scripts will go on working for some time, better for explaining and testing). Thanks Harold, great project! |
We're not planning to provide this ourselves during the first effort. We're trying to only build out the things that people can't efficiently write themselves in SLua first, then adding wrappers around common usage patterns as we see them crop up in real code. Mostly because there's only a handful of folks working on SLua, and we still have a bunch of "must-haves" to implement :P States could be pretty easily done in user code by having states represented as a table of
Yeah, that seems sensible. It seems like the existing event batching is something people want, but I think we can do it with slightly better usability. We're going to look at writing a wrapper around the existing
This was caused by me not paying attention, I'll fix it :)
It's mainly for debugging. With LSL you always know what handlers are active since all handlers are grouped by state and you can't dynamically add or remove them, but that's not the case in SLua.
They'll be removed immediately, it'll be a breaking API change once it's implemented
Likely not. The |
|
To script states in LSL-style, could we have functions like:
Is this related to this project? Or do I write a canny request for new LL functions? |
I believe you'll get much more targeted event dropping under the new scheme. Removing all event handlers for a particular event clears the queue of all events of that type once this is actually implemented in C++. Is there another usecase that
We're going to follow up with an |
|
@HaroldCindy I'm not sure, but I think @Suzanna-Linn's A means to drop/cleanup all registered event handlers at once, could be useful. |
|
In one of my scripts I use state swapping to clear all the listener handles because very very rarely, my script ends up running out of listeners, and I can't figure out how to reproduce that issue, as far as I can see all my code paths create and remove listeners just fine. I have since rewritten the script in SLua, so hopefully I won't run into that issue. But something like |
|
Makes sense to me! Once the event API is finalized / merged we can look at more utilities for clearing the sim-side script event queue if they're still necessary, I don't think any of this work would block that. |
Good idea, clearing the pending events of a specific type when all its handlers have been removed is perfect.
Yes, that would be sufficient if it includes a function to remove all listeners without needing to know their individual handles. This would be useful for simulating a state change, as it would avoid the need to wrap ll.Listen() to store the handles. Although a ll.ListenRemoveAll() would have a more general use, also in LSL, so lazy scripters (like me) wouldn't leave listeners active so often. |
Actually, I was referring to removing all the listeners activated with ll.Listen(), which would allow for simulating a state change in an LSL-style. Although having a parameterless LLEvents:off() to remove all events would be good too, and the same goes for LLTimers:off(). |
We don't want to do too much special logic with missing arguments (since they're just filled with for i, event_name in LLEvents:eventNames() do
LLEvents:off(event_name)
end |
| -- Find the handler first, then remove it | ||
| local found_index = nil | ||
| for i = 1, #handlers do | ||
| if handlers[i] == handler then |
There was a problem hiding this comment.
In the case where a scripter does something like this.
LLEvents:on("touch", function()
ll.OwnerSay("Touched")
end)It becomes impossible to off that anonymous function, without offing all handlers. (Same of LLTimers too)
Options are either
- Document and explain to scripters in advance that they will face this "problem"
- Return some other type of "handle" value from
onthat can be used to unsubscribe (a la JSconst timer = setInterval(() => clearInterval(timer));)
Both are obviously valid choices, just think it should be addressed as a conscious decision rather than just an assumed "default"
There was a problem hiding this comment.
I like the documentation approach more. It feels more natural to name the function before on'ing it, if it has to be off'ed, than having two ways to off (by function name or by returned handle).
There was a problem hiding this comment.
Oh I think the current method should keep functioning as is too, just wanted to address the anonymous function case specifically, either to document it as Intended, or see if it want's handling.
There was a problem hiding this comment.
Indeed, setInterval() returning an opaque handle from JS is a pretty old API before people were thinking too much about API symmetry. We'll go with the documentation approach.
@HaroldCindy does this system allow for custom event handlers or is this out of the scope of the proposal |
|
Custom event handlers are out of scope for this since the underlying event system SL is using doesn't support them |
This contains the proposed API for event handling and timer management in SLua. Please add any suggestions inline on the code, or as a comment on this PR!
Particularly interested in how people think events like
touch_startthat pass anum_detectedforll.Detected*()function calls. Right now those events continue to receive anum_detected, but is there something better we should be doing there?