Skip to content

Make OverlapChecker an interface #5324

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 13, 2021
Merged

Make OverlapChecker an interface #5324

merged 4 commits into from
May 13, 2021

Conversation

dongruoping
Copy link
Contributor

@dongruoping dongruoping commented Apr 28, 2021 •
edited
Loading

Proposed change(s)

Create an IOverlapChecker interface and make BoxOverlapChecker implement it.
This allows other collider overlap strategy like OverlapSphereNonAlloc or OverlapCapsuleNonAlloc.

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

https://jira.unity3d.com/browse/MLA-1934

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Breaking change
  • Documentation update
  • Other (please describe)

Checklist

  • Added tests that prove my fix is effective or that my feature works
  • Updated the changelog (if applicable)
  • Updated the documentation (if applicable)
  • Updated the migration guide (if applicable)

Other comments

@dongruoping dongruoping requested a review from surfnerd April 30, 2021 22:07
@dongruoping dongruoping requested a review from chriselion May 13, 2021 18:19
surfnerd
surfnerd reviewed May 13, 2021
View reviewed changes
surfnerd
surfnerd reviewed May 13, 2021
View reviewed changes
com.unity.ml-agents/Runtime/Sensors/IOverlapChecker.cs Outdated
/// Perceive the latest grid status. Call OverlapBoxNonAlloc once to detect colliders.
/// Then parse the collider arrays according to all available gridSensor delegates.
/// </summary>
void Update();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be called Perceive?

Copy link
Contributor

Choose a reason for hiding this comment

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

PerceiveGrid?

surfnerd
surfnerd reviewed May 13, 2021
View reviewed changes
surfnerd
surfnerd approved these changes May 13, 2021
View reviewed changes
Copy link
Contributor

@surfnerd surfnerd left a comment

Choose a reason for hiding this comment

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

LGTM, we can discuss the naming in slack if you'd like.

@dongruoping dongruoping merged commit c3e74ee into main May 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the develop-overlap-interface branch May 13, 2021 21:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@surfnerd surfnerd surfnerd approved these changes

@chriselion chriselion Awaiting requested review from chriselion

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dongruoping @surfnerd