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

Nmark tactic #1664

Merged
merged 10 commits into from
Jun 16, 2021
Merged

Nmark tactic #1664

merged 10 commits into from
Jun 16, 2021

Conversation

kfu02
Copy link
Contributor

@kfu02 kfu02 commented Jun 15, 2021

Description

This implements the nmark tactic, which marks the n enemies closest to the ball with the robots of ours that are closest to them. For now, ignore the roughness of the defensive_clear.py file, it will be uncommented and filled in once pivot_kick from #1644 is merged in.

Associated Issue

https://app.clickup.com/t/y3fvx9

Steps to test

Test Case 1

  1. Start sim
  2. Drag enemy robots (blue) around the field
  3. Kick ball around

Expected result: Our 2 closest robots to the 2 enemies closest to the ball should move to mark those enemies, regardless of where the ball is. (I know this wording is a little unclear but the behavior makes sense when you see it.)

Test Case 2

  1. Change the number in the init() of play/defensive_clear.py that controls how many robots nmark handles.
  2. Repeat Test Case 1.

Expected result: NMark works the same, but with more (or less) than 2 robots.

Copy link
Contributor

@kylestach kylestach left a comment

Choose a reason for hiding this comment

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

Mostly good, a few minor comments.

skills = self.two_mark.tick(role_results[self.two_mark])
skill_dict = {}
# skill_dict.update(role_results[self.striker_tactic])
skill_dict.update(role_results[self.two_mark])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd appreciate it if you could remove the dead code.

# Get role requests from all tactics and put them into a dictionary
role_requests: play.RoleRequests = {}
# role_requests[self.striker_tactic] = self.striker_tactic.get_requests(world_state, None)
role_requests[self.two_mark] = (self.two_mark.get_requests(world_state, None))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this have a tactic for the robot kicking the ball? And also in theory a pass seeking tactic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and yes. I started off writing the defensive clear play only to realize both the nmark and striker tactics weren't written yet. So this PR is step 1. For step 2 I'll write a striker tactic, uncomment the aforementioned "dead code", and possibly add the pass seeker, then put up a new PR for the full defensive clear play.

Comment on lines +66 to +71

def create_request(self, **kwargs) -> role.RoleRequest:
"""Creates a sane default RoleRequest.
:return: A list of size 1 of a sane default RoleRequest.
"""
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def create_request(self, **kwargs) -> role.RoleRequest:
"""Creates a sane default RoleRequest.
:return: A list of size 1 of a sane default RoleRequest.
"""
pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be fixed at the interface level, as discussed on Teams, ignoring for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue #1665 made

@kfu02 kfu02 requested review from kylestach and hdemusg June 15, 2021 23:26
Copy link
Contributor

@Yudai-8-om Yudai-8-om left a comment

Choose a reason for hiding this comment

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

LGTM! just added one comment.

from dataclasses import dataclass
from typing import List, Optional

import stp.action as action
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe action is imported but not used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not, nice catch

Copy link
Contributor Author

@kfu02 kfu02 left a comment

Choose a reason for hiding this comment

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

cost nit

rj_gameplay/rj_gameplay/tactic/nmark_tactic.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kylestach kylestach left a comment

Choose a reason for hiding this comment

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

LGTM

@kfu02 kfu02 merged commit 20d670b into ros2 Jun 16, 2021
@kfu02 kfu02 deleted the nmark_tactic branch June 16, 2021 03:10
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