Skip to content
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

Allow users to re-enter elevator that changes indicators without moving. #74

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

apottere
Copy link

@apottere apottere commented Feb 7, 2015

See #59

@@ -120,6 +120,12 @@ var createWorldCreator = function() {
}
});
});
elevator.on("indicatorstate_change", function() {
var elevator = this;
if(elevator.isOnAFloor() && !elevator.isMoving && !elevator.isFull()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this addition, the expression elevator.isOnAFloor() && !elevator.isMoving && !elevator.isFull() appears twice in world.js. I think it should be refactored into a function elevator.isSuitableForUserEntrance(). Though that could be done after your pull request is accepted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did think about that, but I wanted to touch as little existing code as possible. I could certainly pull that after this gets merged.

@magwo
Copy link
Owner

magwo commented Feb 7, 2015

I'm afraid this probably can't be done like this.. I think this will cause "sliding users", meaning passengers that are entering the elevator just as it starts moving, making them fly into the elevator between floors sort of. This should be done like the re-arrival behaviour - queueing the floor another time. It's a little messy.

@roryokane
Copy link
Contributor

Then it sounds like this pull request can be fixed by changing

elevator.trigger("entrance_available");

to

elevator.goToFloor(elevator.currentFloor(), true);

But I think before that line, you must either wrap elevator with asElevatorInterface, or fetch that same elevator from world.elevatorInterfaces, so that goToFloor can be used.

@magwo
Copy link
Owner

magwo commented Feb 7, 2015

Yes, basically. I'm thinking of moving the queue logic into the elevator object so that this type of thing can be done without involving the interface object. What do you think about that?

@roryokane
Copy link
Contributor

Hmm, well, in both uses of the interface in world.js, we don’t need the whole queue logic, just a command to “wait on the current floor until all passengers have entered”. The current way of doing that is putting the current floor at the front of the queue. There might be a cleaner way of doing that, but I can’t think of it.

So yes, moving the queue logic to the elevator object is better than the current organization. It might not be the best design overall, but I can’t think of a better design right now.

@magwo
Copy link
Owner

magwo commented Feb 8, 2015

Agree.

@apottere
Copy link
Author

apottere commented Feb 8, 2015

I did try re-queueing it like it's done below, but that's causes an issue where elevators changing their indicators after they've already been told to goToFloor has passengers load, but they never go to the next floor.

@roryokane
Copy link
Contributor

@apottere Are you calling goToFloor on the elevator interface, defined in interfaces.js? It sounds like you might be calling goToFloor on the unwrapped elevator, defined in elevator.js, which ignores the queue logic. As I said, you have to “wrap elevator with asElevatorInterface, or fetch that same elevator from world.elevatorInterfaces”.

@apottere
Copy link
Author

apottere commented Feb 8, 2015

I was calling it on the interface, like a few lines below:

world.elevatorInterfaces[elevIndex].goToFloor(floor.level, true);

That would cancel the call I made in my game script, which was essentially:

elevator.goingDownIndicator(true);
elevator.goToFloor(floor);

@apottere
Copy link
Author

I updated to use the interface - I'm not sure what I thought was broken last time I tried that, but I'm certainly not seeing it now. With that change, the code below works on challenge #1, and the goToFloor call doesn't get canceled:

{
    init: function(elevators, floors) {
        var elevator = elevators[0];
        var floor = floors[0];

        elevator.goingDownIndicator(false);
        elevator.goingUpIndicator(false);

        floor.on('up_button_pressed', function() {
            elevator.goingUpIndicator(true);
            elevator.goToFloor(2);
        });

    },
    update: function() {}
}

@jcoveney
Copy link

This has come up in our play of the game. Hope it gets fixed!

@Rybadour
Copy link

Any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants