-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add MPI parsing to login screen #674
Conversation
This allows future dbref sysparms that use NOTYPE to work properly.
This switches the previous ad-hoc list read over to the same functions used for MPI, which reduces code duplication and allows us to be handle more potential list formats. It also puts them through a common output mechanism.
This adds MPI parsing to the login screen, for the welcome proplist and welcome.txt. New @Tunes: do_welcome_parsing: enables this behavior (only works if do_mpi_parsing is true) welcome_mpi_who: effective "me" (defaults to One) welcome_mpi_what: effective "this" (defaults to Room Zero) MPI variables: cmd: viewer's descriptor arg: viewer's hostname These are set here because it's the only information we actually have about the user at this point, so we might as well.
I would say this actually completes #620 because you can always use the muf macro to run a MUF via MPI. I'm not sure there's a benefit to separate MUF support in addition to MPI, so I think when we merge this, we close #620 as complete. That said, I wanna test some of the weirder MPI prims (like aforementioned muf, delay, and a few other oddballs) to make sure this doesn't have any weird edge cases that could cause problems. Some of these prims might not 'work right' and that is okay, but I want to make sure that a) nothing causes the MUCK to crash and b) as many oddball things are documented and understood as odd as we can. I'd think most things would work okay, though. I'll find some time tonight or tomorrow to put this through the paces. |
Agreed, this looks very good. I haven't looked at do_parse_mesg in a while, but I'm wondering if its possible (or worth it) to optionally pass in an additional environment so variables could be set, Maybe a corner case though. |
I've seen implementations of this where - on the MUF side - instead of "player", muf prims would use a #define that basically was "player, but OWNER(program) if player isn't valid), The solution proposed here might be more robust. |
There's potential value in this, because if we make additional versions of parseprop (parseprop_ex I think would fit in with other similar prims) and the other MPI parsing prims, we can allow MUF to pass additional variables to MPI, which could be useful. That said, I'd say that would be a separate issue, I wouldn't tack it on to this PR. I think @Loonesbury 's use of cmd and arg make reasonable sense. I agree it's "janky" but I also think its in line with how MPI does stuff so I have no issue with the existing approach. So I think test this thoroughly, merge this PR, and then maybe add a new issue to add variable passing to do_parse_mesg and related MUF prims. |
After some thought, I must begrudgingly agree -- MPI alone is more than adequate for and the only reason you'd want MUF is interactivity, like MOO's $do_login_command(), but you'd basically have to restructure or duplicate the entire READ event system to allow for per-descriptor input and per-player input (unless you get rid of multiple connections and do everything on a per-descriptor basis) and that... does not seem worth the number of practical use cases for an interactive login screen. |
Just letting you guys know I haven't forgotten about this -- I had a SURPRSIE! Wednesday demo for something that I wound up basically working all my waking hours to prepare for work :) So I haven't had much free time for this. I probably won't get to testing it until the weekend. Just letting you guys know because that's outside of my original plan! @wyld-sw if you want to give it a test -- I'm particularly concerned about the MPI muf macro but delay, tell, etc. might also be weird -- and you have the time, then feel free to hit it as well. I see a lot of ways this thing could potentially crash the MUCK server by 'things not being in a state that we assume it's in' kind of problems. |
I'm guessing prim_toadplayer used the 'result' temp var in the past, and when it was removed, this check wasn't updated - so it was just checking against whatever 'result' was set to by another primitive.
Finally starting to test these, let you guys know how it goes. |
It's been a busy end-of-year, but I can help test this weekend; just let me know what I can do. I'm wondering if there are cases where we need to store the fact that a MUF call came from the login screen and have certain prims act differently (or fail gracefully) if so. I also wonder if there's need of a {descrtell:{descr},} function for some cases in the future. |
What other tunes aren't showing up? They're appearing fine on my system with minimal.db:
|
Deleting my bug -- that was actually my fault. My restart script was using a hard coded path and not a relative path, so I was picking up an old fbmuck version. |
@Loonesbury beat me to it, though I did also notice that the welcome tunes don't show up in MAN SYSPARM, but that could be a help re-generation issue. |
@Loonesbury Is there anything special I need to do to get MPI parsing to work? I have do_welcome_parsing on:
And I just put some basic MPI in my welcome screen but I'm seeing it unparsed:
|
Oh interesting. If I put hte MPI at the start of the file, it works:
|
It works on random lines and not other random lines... for instance:
results in:
|
|
That's it -- I took out the ASCII art and it all came to life. |
So some preliminary observations. If you put invalid MPI in there, you get the default welcome screen and One (or presumably whomever is
However, if you leave something on the stack, it does work, i.e.
I am starting to suspect it will be unlikely to crash the MUCK with this, but there will probably be a lot of weirdness. I'll stop spamming comments now |
And I didn't mean to close it, I just hit enter and apparently github decided that was good enough to close it. :) Anyway, back to more testing, I'll try to be less noisy. |
Github thinks my shoddy code is flawless! I've fooled them all!
I'm guessing this is because MUF TELLs will go directly to One, since we're pretending One is the triggering player, but {muf} passes the program's return value back to do_parse_mesg and it gets displayed on the login screen. Using DESCRNOTIFY should work fine, though. I never really thought about using {muf} so I didn't bother coming up with a prettier solution, but I'm open to suggestions. |
Could we have NOTIFY (if parameter is "me", at least) act like DESCRNOTIFY if invoked on the welcome screen? Or at least have a way to opt-in to that behavior? |
IMO the more appropriate solution would be allowing player = #-1, even if that would require a lot of work on the interpreter -- otherwise you'd have to add a chain of arguments from do_parse_mesg -> interp_loop -> prim_notify, or add a "is welcome screen" global, which feel kinda janky. |
Yeah, that's kind of what Proto did - had a PSafe() macro that was used in the place of "player" - though it changed #-1 to OWNER(program). We would probably need to treat #-1 as descriptor-specific, in at least this case. |
I tested most things I thought might break something, and none of them did. From my understanding of the internals, we're unlikely to put the MUCK in a state where it crashes because we are satisfying all the MUCK needs to parse most events. I actually went around and grep'd the code for places where a disembodied descriptor (i.e. a descriptor not associated to a player) could cause a problem, and I believe the way we're passing One in to the MPI parser resolves all the issues I could suss out. There's a lot of stuff that isn't ideal, but I'm not really worried about that -- I think it's okay for this to be a little jenky at first, as long as it doesn't crash, lock up, or do some other 'massively bad' behavior. The one 'game breaking' oddity I have found is with MUF 'READ'. It doesn't crash, but it makes the MUF become uneditable:
This happened no matter if I logged in / logged out again or whatever, I had to restart the MUCK to clear it. I've never seen that happen before. It seemed like it was capturing input from both the person on the login screen and One. If the person on the login screen entered something first, One would get this message:
If One entered something first, it seems like it clears up whatever blockage there is, but the output is not sent to the MPI message (which makes sense, because the MPI message is displayed before the READ fires off ... when a MUF goes into a read state, it basically 'context switches' and MPI won't wait for MUF to come back from a context switch). Putting the MUF in 'background' mode will disable READ and at least for a short MUF will work and whatever's on the stack gets put in the MPI message. However, if a background mode MUF is longer than a certain number of instructions then it will context switch; the MPI won't wait for the context switch and will just output what it's got (which will be an empty string). A hacky fix for this would be to force any MUFs triggered by the welcome screen to be in BACKGROUND mode and it just sucks to be you if your program gets context switched before it displays a message. People in such a situation can always increase the instr_slice I feel like leaving 'READ' open will lead to a somewaht annoying bug, as someone could rightfully feel like they could make their own custom login interface by using READ on a welcome-screen MUF. They'll leave their MUCK in a state where it has to be rebooted in order to unblock it which isn't great, but they aren't going to lose data or make a crash which makes it okay-ish. Of course, long term leaving a MUCK in this state, well, I didn't test that. Some wire is crossed in an unholy fashion to make this kind of error, so I'm sure leaving it up for a long time after getting 'stuck' can be problematic. Off the cuff, I don't know what the best fix for this is. I'm inclined to merge this issue and create a new issue to fix the bug we're introducing with it. I'd be willing to take on that bug as there's a lot of possible ways to fix it and frankly most of the ways are bad, so I feel like that's a 'high end, somebody who really knows the system should take it' grade bug. @wyld-sw does that sound reasonable to you? |
Definitely sounds reasonable. Thanks for the deep dive into the particulars. |
Kinda sorta responds to #620.
Abusing {&cmd} and {&arg} to store descriptor and hostname feels bad, but you can't define MPI variables from outside of do_parse_mesg and since the only player we have is One (or whoever welcome_mpi_who is) it's the only information we have to work with.