eventhorizon5 / skyscraper

The Skyscraper Project (Skyscrapersim) - an open source building, city, and elevator simulator
https://www.skyscrapersim.net
GNU General Public License v2.0
28 stars 7 forks source link

Destination Dispatch Bugs & Suggestions #22

Open MultiMonorail opened 1 year ago

MultiMonorail commented 1 year ago

Here are some bugs I found while playing with the Destination Dispatch system in it's initial form:

This also has an effect on a multiple elevator scenario:

Also, the directional chime and lights don't work, I'm not sure if this was intended.

One more thing: Floor buttons inside the elevator should still light up even in DD and hybrid mode. This is seen in many elevators with a hybrid configuration.

Besides all of that, this is a good start to the Destination Dispatch feature!

eventhorizon5 commented 1 year ago

Thanks for those findings, I'll work on them. For the first issue, that's mainly because the controller is using the standard elevator routing functions to operate the elevators, and so it comes with the quirks of that. Call buttons act differently, in that they manage the notifications and opening of the elevator, overriding the elevator routing functions (I've wanted that functionality moved possibly into the elevator so the call buttons and controllers don't have to worry about it). I mainly want to move this stuff around to the proper objects in order to make it more realistic, and to simplify the code (I don't like having 2 sets of arrival notification code and door opening stuff).

I'm aware that the chimes and lights don't work, that's because of what I mentioned before about the standard routing, but I thought they'd work fine. I decided to leave that until after a build came out, and then I'd look into it.

MultiMonorail commented 1 year ago

I also found that the Phase 1 fire service key switches don't work on Destination Dispatch elevators. Also, in the Triton Center DD building, the call station panel for the pool elevator isn't found on the 100th floor.

eventhorizon5 commented 1 year ago

I've fixed the pool elevator call panel and some other panel issues.

eventhorizon5 commented 1 year ago

The indicators and chimes should now be working fine again.

eventhorizon5 commented 1 year ago

The first issue has been fixed, I just have to test the functionality more to make sure it doesn't break the existing elevator functions. The elevator would close it's doors because it simply did an AddRoute() call, which doesn't process the route until after that function. I added code so that AddRoute() would perform a notify and doors open if it's on the same floor, and not add the route to the routing table.

eventhorizon5 commented 1 year ago

The floor button lighting issue is now fixed. I also added a new call type for destination dispatch calls, where they'll show up as "Destination" calls when you display the elevator routing tables.

eventhorizon5 commented 1 year ago

Fire Phase 1 has been fixed.

MultiMonorail commented 1 year ago

I tried out the update you put out today, and I have some stuff that you should know about:

  1. Although the first issue was fixed, the destination indicator does not notify the user what elevator to take in that scenario. And when the doors are closing as the user inputs a destination, the doors keep closing instead of reopening.
  2. The second issue is still in the simulator after the update.
  3. The elevator in DD mode chimes on arrival even if it's set to false, which is the default.
  4. The floor button lighting should occur right as the elevator doors open on the pick-up level. Right now, it lights up when the doors close.
  5. For the indicator, it should only show which elevator is coming for about a few seconds, and not wait until the elevator doors close to turn off. This is so that one controller can handle more than one requests.
  6. The indicator should notify the user what elevator will pick them up right when they request the destination. Right now, it waits until the elevator is heading to the pick-up floor before notifying.
  7. Also, for some reason, the DD system can't handle multiple calls for one elevator. This was seen by selecting multiple floors in the Simple building. The elevator will only register the first call.

Hoping that these will help you out!

eventhorizon5 commented 1 year ago

Some of that is due to the fact that I don't know exactly how DD is supposed to behave, it's better when someone like you provides input since you know how it works in real life better than I do. Since this new feature is a different operating mode for elevators, I expected it to introduce lots of bugs.

I'll check the second issue. With 1 and 6 (possibly the same issue), the indicator displays the elevator letter once it selects the elevator to use (once that elevator becomes available). To change that to display earlier will require reworking the code more, I'll have to look into how to do that, since right now I'm not quite sure how to do it.

4 and 5 are part of the same code I think, I delayed the processing code since I didn't know how long to display the letter indicator, I'll fix that.

I'll check what's going on with 3.

For 7, if you look at the console log, it actually does register multiple calls (routes) for the elevator but isn't working properly. It won't proceed to multiple floors for the same elevator arrival because the code isn't there yet for that, I just coded a simple multi-elevator setup for it.

eventhorizon5 commented 1 year ago

I checked issue 1 that you had, and the functions appear to be working fine for me. I tested the Simple DD building, and it works correctly as the doors are opening, doors opened, and while the doors are closing (when closing, they re-open). I'm not sure why yours would be behaving differently, I can try on another system to see how it's working.

eventhorizon5 commented 1 year ago

For the 2nd issue, it appears to work fine for me too (the issue with it choosing a different elevator for a call instead of the one open on the floor), I tested it in the Triton Center DD building.

MultiMonorail commented 1 year ago

It looks like the 1st issue is gone, but I found something else: If an elevator has it's doors open and the user enters a destination that's opposite of the elevator's last active direction, it will still close and open again. For example: If I go up to floor 3 and request a ride down to floor 2, the elevator will still close and open since it's last active direction was up.

I think this is what's also causing the 2nd issue too, since I tested it from the 14th floor of the Zone 1 elevators in the Triton Center and the 2nd issue happened; but when I tested it from a intermediate floor, the issue wasn't there.

eventhorizon5 commented 1 year ago

That issue is due to how the route processing code is in the elevators, the standard mode (non-DD) for elevators works the same. DD mode does this too because it's using the same route direction code as standard mode, I can see if I can change both, if the standard behavior is wrong too.

eventhorizon5 commented 1 year ago

I have most of the DD operational/routing code working, I'll put out a build today so you can try it, it's working really well now. I'm going to also try to add sound support for the indicators, and eventually I'll get to creating the letter indicators above the doors.

MultiMonorail commented 1 year ago

I've just tried out the new update, and most of the stuff with the indicator is fixed! There are still some stuff with the way elevators are called with DD that came up with this update:

  1. When an elevator that was once on a different floor arrives for a call, the directional indicator and chime won't turn on. (This was probably in response to issue 3. Sorry if I worded it poorly.)
  2. This also effects directional messages as well, it will default and say that the elevator is going up, even when the elevator is going down.
  3. I've already mentioned this, but if an elevator on an intermediate floor has it's doors open and the user enters a destination that's opposite of the elevator's last active direction, it will still close and open again / call a different elevator. This situation on the top and bottom floors is non-existent now.

Also, when an elevator is not available, the console will spam: "ProcessRoutes: No available elevators found, will try again". I think you should make it so that it gives one message that the controller is trying again, and make the current situation the norm in Verbose Mode.

Hope this helps! Sorry if I'm bugging you so much with all of these fixes. The Destination Dispatch system is coming along great, and every update is an improvement from the last!

eventhorizon5 commented 1 year ago

Your insight is helpful to me, I hope you enjoy the current update because when I was working with it, the DD functionality really seems to be working well now.

About the console spam, I didn't know how much that would show up, but that error is basically a controller malfunction because it can't find available elevators to assign to (in that situation, it doesn't show a letter either because there's nothing to assign to, do you have any ideas of what it should do in this situation?). I'll probably add a flag so that it'll print the message once and then reset it when the system becomes available again.

I'll check those other issues too.

MultiMonorail commented 1 year ago

In the situation you mentioned, the controller / panel would say "XX" or "Access Denied". But in those situations, the controller would just stop finding elevators. Those messages would also show if the floor was unavailable or if the user was already on the floor. If you still want the controller to look for elevators though, I think a "Please wait..." message would be appropriate, but I don't recall any DD systems that use this method, only the first one.

And I enjoy the update! The system is working well and in good shape for other users to create buildings with it.

eventhorizon5 commented 1 year ago

I'll probably change it show it shows XX (the texture ButtonXX would have to be loaded for that to work though, so people need to know about that). Right now it keeps retrying the route and continues once elevators become available and then shows the letter, I'll change it so that if it can't find elevators, it drops the route and fails with an XX. The RequestRoute() function submits the destination info to a routing table, and then ProcessRoutes() processes that table, so the original failure message loop meant that ProcessRoutes failed and didn't know what to do, so it would retry again in the next sim loop.

The current script code should be finished and ready to use in buildings, I just still need to add the other indicators so i haven't decided on how the script code will look yet.

eventhorizon5 commented 1 year ago

Those 3 points you made appear to be fixed now, and the console spam is now an "XX" call failure.

eventhorizon5 commented 1 year ago

I put a new build out today, see if it's working better for you.

MultiMonorail commented 1 year ago

Most of the stuff is working great! There is one more thing DD related though: The sound for showing the elevator ID on the DD panel is always playing in one spot. Each panel should have the sound.

MultiMonorail commented 1 year ago

Also, I just found this out: In multiple elevator scenarios, if I call an elevator to one floor and it leaves, if I don't take that elevator and call that same floor again, it will still take me to the elevator that just left instead of another elevator.

eventhorizon5 commented 1 year ago

I noticed that sound issue before but wasn't sure if it was broken or just quiet. The sound should be positioned with the call station, maybe it's playing at the center of the building or something. I'll check that other situation you mentioned too :) Let me know if you find anything else.

The things I found: I tried it today and it incorrectly assigned an elevator in the middle of traveling to a destination. Yesterday there was an issue with the controller not withdrawing routes when they were completed, a route got stuck and was causing issues.

eventhorizon5 commented 1 year ago

That issue where it assigned the wrong elevator ended up being a major bug I found in the FindClosestElevator() function, it's been fixed now. You should notice it if it seems to skip the first elevator in a group a lot, and possibly other things.

MultiMonorail commented 1 year ago

With the new update, it looks like an issue has come back, but only in a multiple elevator scenario: With an elevator on an intermediate floor having it's doors open; if the user enters a destination that's opposite of the elevator's last active direction, it will call a different elevator. It might have to do with the fix you made for the FindClosestElevator() function. If you can try and fix it without it affecting anything else, that would be good. If not, you can just leave it like it is. Also, the sound issue is still in the simulator.

The assigning the wrong elevator issue was fixed though!

eventhorizon5 commented 1 year ago

I'll check that elevator calling issue. I forgot to fix that sound issue, I'll have a look at it now.

eventhorizon5 commented 1 year ago

The sound issue has been fixed, the commands in the indicator were just out of order, so it moved the indicator before it created the sound, leaving the sound at the center.

eventhorizon5 commented 1 year ago

The elevator selection issue has been fixed, the last check in the elevator's AvailableForCall() function was too simple when checking the queue direction, so I added a test that'll pass if elevators are on the same floor with queues empty. I should probably move that into the IsIdle() function, but I didn't want to touch that function for now, I might look into that though.

eventhorizon5 commented 1 year ago

I put out a new build with those fixes, hopefully it's working good now. Let me know if you find any issues with this new build.

MultiMonorail commented 1 year ago

I've tried the build for the first time, mainly to check the issues I had, and it seems like these issues are fixed! I'll test the build again tomorrow to see if there's anything else relating to DD / the keypad feature.

MultiMonorail commented 1 year ago

Playing around with the keypad function, while it does work as it should for the most part, I do have something that you should know about:

In many buildings, there is no floor 13. If I test the keypad function in a building starting at floor 1 and skipping 13, the controller would think I want to travel to the 13th floor which has an ID of 15. What I would suggest is to first add in a code for the buildings called InvalidInput for within the controller section. If a building creator adds in something like "InvalidInput = 0, 13", the controller would mark 0 and 13 as invalid regardless of whether or not they are floors within the building.

Also, if I have a building with 10 floors, and the top floor has an ID of R instead of 10, If I test the key pad function and put in 10, the controller would show the floor as invalid even though there is a "10th floor". I would suggest adding in an AlternateID code for when a creator is assigning floors. For example: A building has a floor with an ID of R and an Alternate ID of 10, if a user inputs 10 into the keypad, the elevator would go up to the floor with the Alternate ID of 10, aka R.

Otherwise, everything else appears to be working great from my end of things!

eventhorizon5 commented 1 year ago

I created a new floor parameter called "NumberID", and that should fix those issues. You just specify a number for the floor, and that's what the call station will use.

MultiMonorail commented 1 year ago

I tested the new update, and some issues have come up regarding the keypad:

  1. The keypad now doesn't use the floor ID on default. It now skips to the actual floor number if a NumberID isn't set. (I originally suggested NumberIDs to be used for special cases for when a floor ID isn't a number.)
  2. The 0 button should act as a number and shouldn't always call an elevator to the recall floor, only the * button.
  3. The limited input feature by Xteal makes selecting a floor a bit too quick compared to real-life. Once the input cap has been reached, it should wait about a second before assigning an elevator, unless an input is invalid.

That last one should be a setting that would also impact regular DD panels too, since it usually takes a half-a-second to about a second for when a passenger selects a floor to when an elevator is assigned. Those seconds would also be when the floor the user selected is shown.

I also found issues that affect both regular and keypad DD:

  1. The Simple - Destination Dispatch building doesn't have a "<A" texture, which causes the elevator ID to not show on the indicator.
  2. When an elevator arrives at a floor to drop off someone off, if that same elevator is arriving for a call, the directional indicator and chime won't turn on.

I also tried out hybrid mode. The older call buttons don't work, but the newer ones do.

eventhorizon5 commented 1 year ago

I'll look those over - thanks :) This was a pretty massive update (if you saw the dispatch controller code changes, you'd know why), so I was expecting a lot of bugs. Thanks for helping find them for me.

eventhorizon5 commented 1 year ago

I've fixed 1, possibly 2, and fixed 3, when you said "it usually takes a half-a-second to about a second for when a passenger selects a floor to when an elevator is assigned", I'm not sure the best way to do that, because the current delay is a key input delay, I could add a controller delay which would delay it further (key input delay plus controller selection delay, that shouldn't be too hard).

The Simple - DD building has been fixed. I'll look at that last number 2 one, because the notifications have had lots of problems lately.

MultiMonorail commented 1 year ago

You could do what you mentioned and let the user change the delay in the building code.

eventhorizon5 commented 1 year ago

I'll do that, right now it's hard-coded to 2 seconds on a timer.

MultiMonorail commented 1 year ago

Ok. If you can, also have that setting available for non-keypad DD panels as well, since right now it instantly assigns a person to an elevator.

eventhorizon5 commented 1 year ago

That's what I was planning on doing. A controller delay would affect everything in DD mode. Should it also affect standard mode too? (right now the elevators instantly respond which is how the behavior has always been).

MultiMonorail commented 1 year ago

I don't think so.

eventhorizon5 commented 1 year ago

I'm checking out the issue "When an elevator arrives at a floor to drop off someone off, if that same elevator is arriving for a call, the directional indicator and chime won't turn on." and can't seem to reproduce it, do you know what you did to make it happen? The Triton Center is a little complicated with it, but it seems to work fine in the Simple building.

MultiMonorail commented 1 year ago

Here's what I did in the Simple building:

  1. I put in a destination for a floor (Floor 4 for instance).
  2. I then flew up to that same floor and put in a destination for a different floor (Floor 10 for instance).
  3. When the elevator arrived at floor 4, it didn't chime, and then continued to floor 10.

Also, I just realized that there are some instances in standard mode with a controller delay, so you can have a setting for that if you want.

eventhorizon5 commented 1 year ago

I found it now, I was checking the regular Simple building and not the DD one.

eventhorizon5 commented 1 year ago

That issue's now fixed. If the elevator car's NotifyArrival() gets a direction of 0, it asks the dispatch controller if there's any pending routes for that floor and elevator, and uses the direction of that for the arrival notification (chime/directionindicator). This appeared to be the only way to fix it, it was a more complex fix, not sure if there's a simpler way.

eventhorizon5 commented 1 year ago

With hybrid mode, what did you test with that? I'll see if I can check it.

MultiMonorail commented 1 year ago

I mainly checked to see if the buttons would spawn in, and if they did, if they called the elevator. The older buttons won't spawn in with the newer code, but the newer ones will.

I also found something DD related just now: For a one elevator scenario, when the elevator arrives at a floor to drop off someone off, if that same elevator is arriving for a call to a floor in the opposite direction of the last active direction, the doors will close, the elevator will then open the doors, and then it will somehow delete the second call. For example:

  1. I put in a destination for a floor (Floor 4 for instance).
  2. I then flew up to that same floor and put in a destination for a different floor bellow 4 (Floor 1 for instance).
  3. When the elevator arrived at floor 4, the doors opened and closed, then the doors opened again and the route to floor 1 was deleted.
eventhorizon5 commented 1 year ago

The old buttons won't work with the Call Station code, if that's what you're referring to, the call buttons are a different type of C++ object and so they don't work with it.

I tried that scenario you mentioned, and it's behaving pretty badly :) I'll see what's going on.

eventhorizon5 commented 1 year ago

I fixed that bad situation, and also have a fix in place that should hopefully fix all arrival notification issues for good. I found that the elevator was executing commands in the wrong order, it would delete the route (with call information) before chiming/notifying, I reversed those and things are working much better now. I'll try to test your situations and see if I can find anything broken. I removed the PendingRoutes() code that I added recently (where it asks the controller about routes to determine call direction, that's kinda crazy, the elevator should already know that), since it shouldn't be needed anymore.

eventhorizon5 commented 1 year ago

That situation you mentioned was due to the controller not having enough information about calls, it just stored one call without direction info. I made it store multiple calls along with directions of each, and now it's working much better.

eventhorizon5 commented 1 year ago

I brought back the PendingRoutes code, since removing it caused one issue to come back. So far things are looking good, I'll do more testing.