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

split robot controllers into separate files #30

Merged
merged 5 commits into from
Jan 4, 2021
Merged

split robot controllers into separate files #30

merged 5 commits into from
Jan 4, 2021

Conversation

Adman
Copy link
Member

@Adman Adman commented Dec 27, 2020

Signed-off-by: Adrian Matejov ado.matejov@gmail.com

Copy link
Contributor

@mrshu mrshu left a comment

Choose a reason for hiding this comment

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

The fact that the filenames are so long is rather unfortunate but what can we do -- I guess this is the price we'll have to pay 🙂

I have only a couple of nitpicks @Adman, most of them left by myself anyway, so feel free to discard any that do not make sense.

Thanks for putting this together!

controllers/rcj_soccer_player_b1/rcj_soccer_player_b1.py Outdated Show resolved Hide resolved
controllers/rcj_soccer_player_b2/rcj_soccer_player_b2.py Outdated Show resolved Hide resolved
controllers/rcj_soccer_player_b2/rcj_soccer_player_b2.py Outdated Show resolved Hide resolved
@Adman
Copy link
Member Author

Adman commented Dec 28, 2020

@mrshu So, I created an abstract robot class, which is located in controllers/abstract_robot.py. Its purpose is to make it easier for participants to read and process data received by supervisor. One small disadvantage is that each robot controller must include some byrocracy on top of the file in order to add this abstract robot file into path.
Could you please review once again? Thanks a lot!

@Adman Adman mentioned this pull request Dec 29, 2020
@mrshu mrshu mentioned this pull request Dec 30, 2020
Copy link
Contributor

@mrshu mrshu left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @Adman!

The bureaucracy at the beginning of the file is a small price to pay for the ability to share code among robots. I think we can live with that.

One last small suggestion: if possible, I think it would make sense to create a two-team folder structure, something like

$ tree team_robots
team_robots/
├── teamA
│   ├── robot1
│   │   └── robot1.py
│   ├── robot2
│   │   └── robot2.py
│   ├── robot3
│   │   └── robot3.py
│   └── utils
│       └── rcj_serializer.py
└── teamB
    ├── robot1
    │   └── robot1.py
    ├── robot2
    │   └── robot2.py
    ├── robot3
    │   └── robot3.py
    └── utils
        └── rcj_serializer.py

We could then reflect that in the README as well, and make it so that each folder in team_robots is a separate Git repository on its own. Would that make sense?

controllers/rcj_soccer_player_b1/rcj_soccer_player_b1.py Outdated Show resolved Hide resolved
controllers/rcj_soccer_player_b1/rcj_soccer_player_b1.py Outdated Show resolved Hide resolved
controllers/abstract_robot.py Outdated Show resolved Hide resolved
@Adman
Copy link
Member Author

Adman commented Dec 30, 2020

@mrshu Regarding directory structure comment: Not sure if this directory structure is going to work. Each robot controller needs to be put into controllers/robot1/robot1.py. So in the end the controllers folder contains 6 folders (one per each robot). If both teams want to include helper scripts, its fine to put it to controllers/robot1 folder, but not into controllers/ because there could be conflicts if both teams have scripts with the same name. I haven't checked the docs if it's possible to have team_a and team_b controllers structure, so let's see if we can manage to configure it somehow.

@mrshu
Copy link
Contributor

mrshu commented Dec 30, 2020

You are probably right @Adman, there is a good chance it is not going to work. I checked the docs and it does not seem like teamA and teamA would be possible -- Webots checks the subfolders of controllers/ directly.

I guess that only leaves us with one option: symlinking common code. Since it ought to be supported on (modern) Windows as well, I guess it may actually work.

The folder structure we'd end up with would then look as follows:

controllers
├── robot_teamA_1
│   ├── robot_teamA_1.py
│   └── utils
│       └── special_teamA_serializer.py
├── robot_teamA_2
│   ├── robot_teamA_2.py
│   └── utils -> ../robot_teamA_1/utils/
├── robot_teamA_3
│   ├── robot_teamA_3.py
│   └── utils -> ../robot_teamA_1/utils/
├── robot_teamB_1
│   ├── robot_teamB_1.py
│   └── utils
│       └── rcj_serializer_from_teamB.py
├── robot_teamB_2
│   ├── robot_teamB_2.py
│   └── utils -> ../robot_teamB_1/utils/
└── robot_teamB_3
    ├── robot_teamB_3.py
    └── utils -> ../robot_teamB_1/utils/

It's not very pretty, but we could at least get rid of the bureaucracy at the top of files and actually make these work with Webots. On the downside, this will force us to do two other things:

  1. force naming convention on the controllers (i.e. "make it unique by mentioning your team name")
  2. make one of the controllers the "base"

I guess we can live with that, although I must say it really does not look nice -- more like the opposite.

@Adman Adman force-pushed the demo-players branch 2 times, most recently from fde8daf to 5138e37 Compare December 30, 2020 21:09
@Adman
Copy link
Member Author

Adman commented Dec 31, 2020

@mrshu We have to decide between two options when we are preparing the match.

  1. Change controller's name in soccer.wbt world
  2. Change controller's python script name in controllers according to soccer.wbt.

If we decide to go with the second option, we effectively destroy the possibility to create symlinks.

@mrshu
Copy link
Contributor

mrshu commented Jan 2, 2021

@Adman Good point. I don't see any other way than going with 1., changing the folder/script name can have such a massive amount of unintended consequences due to how Webots implements it, that it does not seem to be worth it in any way.

@mrshu
Copy link
Contributor

mrshu commented Jan 2, 2021

@Adman please feel free to let me know if there is anything I ought to take a closer look at here.

@Adman
Copy link
Member Author

Adman commented Jan 2, 2021

@mrshu Computing the angles has been moved to abstract robot class.

Would you like me to create example utils.py scripts as well as symlinks from other controller folders?

@mrshu
Copy link
Contributor

mrshu commented Jan 2, 2021

@Adman I'd much appreciate that -- please feel free to go ahead!

@Adman
Copy link
Member Author

Adman commented Jan 2, 2021

@mrshu I created the file utils.py within B1 and Y1 controller folders and utils.py symlinks from B2,B3 to B1 and Y2,Y3 to Y1.

@Adman
Copy link
Member Author

Adman commented Jan 2, 2021

@felipenmartins Seems unrelated to this pull request. We probably need to investigate how paths in windows work :(

Could you please check if the problem persists on master branch as well?

@felipenmartins
Copy link
Contributor

The creation of the utils.py also created errors. Now, for all robots I get the following error message:

Traceback (most recent call last):
  File "rcj_soccer_player_b2.py", line 14, in <module>
    import utils
  File "C:\Users\Admin\OneDrive - Hanzehogeschool Groningen\Documents\GitHub\webots-soccer-sim-playground\controllers\rcj_soccer_player_b2\utils.py", line 1
    ../rcj_soccer_player_b1/utils.py

@felipenmartins
Copy link
Contributor

@felipenmartins Seems unrelated to this pull request. We probably need to investigate how paths in windows work :(

Could you please check if the problem persists on master branch as well?

Yes. I just cloned the master again. The errors related to the "utils" disappeared, but the error related to the json file is still there:

Traceback (most recent call last):
  File "rcj_soccer_referee_supervisor.py", line 46, in <module>
    referee.kickoff()
  File "C:\Users\Admin\OneDrive - Hanzehogeschool Groningen\Documents\GitHub\webots-soccer-sim-playground\controllers\rcj_soccer_referee_supervisor\referee\referee.py", line 132, in kickoff
    msg=f"Robot {robot_name} is kicking off."
  File "C:\Users\Admin\OneDrive - Hanzehogeschool Groningen\Documents\GitHub\webots-soccer-sim-playground\controllers\rcj_soccer_referee_supervisor\referee\json_logger.py", line 38, in event
    with self.logfile.open('a') as outfile:
  File "C:\Program Files\Python37\lib\pathlib.py", line 1203, in open
    opener=self._opener)
  File "C:\Program Files\Python37\lib\pathlib.py", line 1058, in _opener
    return self._accessor.open(self, flags, mode)
OSError: [Errno 22] Invalid argument: 'reflog\\The_Blues_vs_The_Yellows-2021-01-02T15:41:28.586156Z.jsonl'
WARNING: 'rcj_soccer_referee_supervisor' controller exited with status: 1.

@Adman
Copy link
Member Author

Adman commented Jan 2, 2021

@felipenmartins Could you try this https://stackoverflow.com/a/59761201 and then clone the repository with demo-players branch again ?

@felipenmartins
Copy link
Contributor

@felipenmartins Could you try this https://stackoverflow.com/a/59761201 and then clone the repository with demo-players branch again ?

@Adman Do you want me to enable "Developer mode" and enable symlinks? I guess I could try it, but wouldn't the error persist because of the ":" character in the json file name?

OSError: [Errno 22] Invalid argument: 'reflog\\The_Blues_vs_The_Yellows-2021-01-02T16:58:18.279552Z.jsonl'

@felipenmartins
Copy link
Contributor

@Adman I have enabled Developer Mode, but I don't know how to "enable symlinks". I have GitHub Desktop installed, but the command

git config --global core.symlinks true

does not work. It says "git is not recognized", or something. I tried with

github config --global core.symlinks true

which opens the GitHub desktop app, but does not change any configuration (apparently). After that I cloned the branch again, but the errors with the "utils" persist.

@mrshu
Copy link
Contributor

mrshu commented Jan 2, 2021

@felipenmartins let me merge master to this branch, that should fix the

OSError: [Errno 22] Invalid argument: 'reflog\\The_Blues_vs_The_Yellows-2021-01-02T16:58:18.279552Z.jsonl'

errors.

With regards to the GitHub desktop app, do you think it would be possible to open something like a "git" terminal from the app? That is where git config --global core.symlinks true should already work quite well.

@mrshu
Copy link
Contributor

mrshu commented Jan 2, 2021

Ah, it seems like what I was thinking of no longer exists...

https://stackoverflow.com/a/54679083

@Adman
Copy link
Member Author

Adman commented Jan 2, 2021

@mrshu @felipenmartins I changed the controller structure so that each robot controller folder contains both abstract robot class and utils file. I completely don't like it but it will make the life easier so we are not hacking around the paths and symbolic links.

Copy link
Contributor

@felipenmartins felipenmartins left a comment

Choose a reason for hiding this comment

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

@Adman Now that you copied "utils.py" and "rcj_soccer_robot.py" to all robot folders, it works on my Windows machine. :-)
I wonder: is it possible to make just one file from the content of "utils.py" and "rcj_soccer_robot.py"?

Copy link
Contributor

@felipenmartins felipenmartins left a comment

Choose a reason for hiding this comment

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

@Adman Please, ignore my previous comment about merging the files utils and rcj_soccer_player. Now I understand that it is actually appropriate that they are independent files.
So, from me, this is good!

@mrshu
Copy link
Contributor

mrshu commented Jan 2, 2021

Thanks @Adman but I really don't think we can do it this way.

By copying identical code we are teaching the wrong paradigm. I would really prefer us do some bureaucracy in the beginning of the files rather than having verbatim copies of the same code over and over. You know very well how it is: you change one file, forget to change the rest and spend the next three hours debugging what's wrong. There is a good chance this will save us some headaches in the beginning but I worry that we'll have to do a complete overhaul in the future because sharing code between robots will be extremely cumbersome.

That said, if despite all this you think it would make more sense this way, I am happy to try it out ;)

Signed-off-by: Adrian Matejov <ado.matejov@gmail.com>
Signed-off-by: Adrian Matejov <ado.matejov@gmail.com>
Signed-off-by: Adrian Matejov <ado.matejov@gmail.com>
Signed-off-by: Adrian Matejov <ado.matejov@gmail.com>
* others can import it from there

Signed-off-by: Adrian Matejov <ado.matejov@gmail.com>
Copy link
Contributor

@mrshu mrshu left a comment

Choose a reason for hiding this comment

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

Sorry it took this may tries @Adman -- thanks for pushing it through!

The only small comment I'd have would be about @felipenmartins's variables. I believe he altered the speed a bit in order for the game to be a bit more interesting. I am not sure if that survived but I guess we can update that real quick any time we want in the future.

Let's get this merged in!

@mrshu mrshu merged commit c385342 into master Jan 4, 2021
@mrshu mrshu deleted the demo-players branch January 4, 2021 14:30
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.

3 participants