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

Reorder angles in RayPerceptionSensor #5746

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RedTachyon
Copy link

@RedTachyon RedTachyon commented May 15, 2022

Proposed change(s)

This PR changes the order in which rays are recorded in the RayPerceptionSensor. Currently, they start from the central ray, and then alternate between one to the left and one to the right. This makes it tricky to process these observations with a convolutional layer, which in my experience caused at least some people to reimplement a ray sensor instead of using the built-in one.

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

Mentioned in Issue #5737

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

Copy link
Contributor

@jrupert-unity jrupert-unity left a comment

Choose a reason for hiding this comment

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

The change makes sense but will have to happen with a means of maintaining backwards compatibility until we want to make a major version change since it would otherwise break existing models. This will be added to the backlog.

Copy link
Contributor

@jrupert-unity jrupert-unity left a comment

Choose a reason for hiding this comment

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

I hadn't noticed that the issue has already been logged. I'll follow up soon.

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.

2 participants