search

CityScope / CS_Cooper-Hewitt

meta repo/sandbox repo for keeping everything related to the Cooper Hewitt exhibition
5 stars 2 forks source link

Agents should update their destinations when buildings change #84

Closed aberke closed 5 years ago

aberke commented 5 years ago

Currently, agents calculate/choose their route at the beginning of their trip. If the destination block for their trip is moved while they are traveling, they still go to where the block originally was.

@LAAP is this what you believed to be an interaction issue? cc @yasushisakai

aberke commented 5 years ago

PR to hopefully address this: https://github.com/CityScope/CS_Cooper-Hewitt/pull/85

agrignard commented 5 years ago

@LAAP it looks like this pull Request made by @aberke https://github.com/CityScope/CS_Cooper-Hewitt/pull/85 is making agent recompute their path as soon as you move a building

Did you have the chance to test it on the table? I am curious to see how it look it sounds cool

LAAP commented 5 years ago

@aberke and @yasushisakai are focused now on this issue. It will be very cool

agrignard commented 5 years ago

@LAAP , @aberke , @yasushisakai Did you have the chance to make it work? As the date of 16th is here you now should be able to ahev all the issue related to ABMobility fixed.

agrignard commented 5 years ago

I'd like to close this issue as soon as possible. Can @yasushisakai or @aberke have a lookt at it, and update at least what is the problem?

Is due to one of the change that has been done last week? Maybe this one https://github.com/CityScope/CS_Cooper-Hewitt/pull/79. In any case it is important to point to which one so we can track the different change (this is one of the reason we have a revision control architecture) and be more strict for future PR

aberke commented 5 years ago

@yasushisakai and I have developed an understanding of the bug and just need to find more time to work on it together -- there has been much other work going on

agrignard commented 5 years ago

Can you be a bit more precise by "developed an understanding of the bug" and "there has been much other work going on" are you referring to some specific code/implementation/design, if yes which commit or issues?

If you don't have enough time with @yasushisakai it's ok but it is crucial to fix this issue very soon and I can spend some time on that however there is not enough information to start from the right place

What 's happening exactly? Agent doesn't recompute there path? Recompte of wrong path? go to the wrong building? How can we reproduce it now that the UDPSender can emulate the behavior of the physical table?

aberke commented 5 years ago

Here is a PR for the fix: https://github.com/CityScope/CS_Cooper-Hewitt/pull/92

The problem/bug was that the immutable properties of buildings (id, capacity, etc) were being updated in the updateGridFromUDP function. There were two issues that came out of this this was being done:

agrignard commented 5 years ago

This PR might have fix the behavior of the agent however it cannot be merge as it is as it has introduces some change in the handling of the special building when id -1 and id >18

It looks like the agent is now updating there path however we lost some key feature in this pull request:

The interactive building doesn't work anymore, if you put building 20,21, 22 there is no effect anymore When we have no block on the grid -1 in red was displayed (at least to help debugging) now if there is no block on the grid the id is not displayed anymore.

aberke commented 5 years ago

I restored the special debug interactions for 20,21,22 - you are right that a separate PR should remove the debug behavior. I don't understand the utility in having the -1 show as red -- simply not showing a building (since there is no building there) should be enough. @yasushisakai and I will test these changes this morning

aberke commented 5 years ago

Within that commit — I amended it

On Tue, Nov 20, 2018 at 12:30 PM Arnaud Grignard notifications@github.com wrote:

@aberke https://github.com/aberke I dont understand what you mean when you say

"I restored the special debug interactions for 20,21,22 - you are right that a separate PR should remove the debug behavior." what do you mean in which commit?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CityScope/CS_Cooper-Hewitt/issues/84#issuecomment-440360697, or mute the thread https://github.com/notifications/unsubscribe-auth/ABbllBJDar41KZClAkyTI1B16XjUxmakks5uxDwrgaJpZM4Yhi2w .

agrignard commented 5 years ago

Within which commit ?

I am bit lost in the different version. I think we can try to stop temporally the spatial effect and focus on this issue Agents should update their destinations when buildings change but right now we are in a in between state as the merge has been done after my push.

Basically what I did this morning is to be able to trigger the special effect by knowing if the special building (id >18) are in the table, also it's important to knwo if id=_1 is on the table not only for debug but to shut don a spatial effect, that 's one of the reason why I said we need to have a trace of building id =-1 and >18 (on top of the fact that it is how it was and that it's not the right timing to change stuff that are working). However I think we lost this information in the PR 92

yasushisakai commented 5 years ago

I am a bit lost in the different version

Yeah, let me clarify, looks like there are three things?

  1. Agents should update their destinations when builidngs change
    • PR 92
  2. Id > 18 (debug) special effects
    • restored in PR 92
  3. -1 status showing for dubugging
    • currenlty lost, BuilindingsOnGrid held that idea.

Is this the status?

agrignard commented 5 years ago

What I propose is to focus on making the Agent move to their location And for now, let's not change ANYTHING if it has not been request in a dedicated issue and let's NOT ADD ANY FEATURE if it has not be asked in a dedicated issue. We are to close to the deployment to play this game.

For the temporal spatial effect (issue#86) it was an enhancement (it is now a bug), let's forget about this for now the priority is this issue #84

Let me know if you need any help clarification

agrignard commented 5 years ago

I just revert the last commit so the current master is the PR92 + 3ee0cab (a part of it has some of this code has been remove by the merge of the PR92)

agrignard commented 5 years ago

Except having some conflict with issue #86 the behavior fo the agent should be fine.

The fact that we don't display id=-1 is ok for now as we are getting close to the end of the project

Let's close this issue once we can validate that it works on the physical table

yasushisakai commented 5 years ago

Physical table ran for 24h+