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.

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

Turns physics modules into optional dependencies. #5112

Merged
merged 4 commits into from
Mar 22, 2021
Merged

Turns physics modules into optional dependencies. #5112

merged 4 commits into from
Mar 22, 2021

Conversation

surfnerd
Copy link
Contributor

@surfnerd surfnerd commented Mar 14, 2021 •
edited
Loading

Proposed change(s)

  • Make physics and physics2d optional deps.
  • Update yamato tests to run coverage tests separately from the package tests. This allows us to fail quickly if coverage is not up to par with our goals.

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

#5104 (Make both physics modules optional dependencies (com.unity.modules.physics and com.unity.modules.physics2d))
MLA-1850

Screen Shot 2021-03-15 at 8 21 24 PM

Types of change(s)

  • Code refactor

Checklist

  • Added tests that prove my fix is effective or that my feature works

Other comments

@surfnerd surfnerd requested a review from chriselion March 14, 2021 21:20
@surfnerd surfnerd force-pushed the develop-physics-optional-dep branch from 5994dff to a27e9b1 Compare March 15, 2021 16:28
@delete-merged-branch delete-merged-branch bot deleted the branch main March 15, 2021 23:13
@surfnerd surfnerd force-pushed the develop-physics-optional-dep branch from a27e9b1 to f17d9c9 Compare March 15, 2021 23:43
@surfnerd surfnerd changed the base branch from develop-analytics-optional-dep to main March 15, 2021 23:45
@surfnerd surfnerd self-assigned this Mar 16, 2021
@surfnerd surfnerd force-pushed the develop-physics-optional-dep branch from e6f24a8 to f9e99da Compare March 16, 2021 05:13
surfnerd
surfnerd commented Mar 17, 2021
View reviewed changes
.yamato/com.unity.ml-agents-optional-dep-tests.yml
NOT pull_request.draft AND
(pull_request.changes.any match "com.unity.ml-agents/**" OR
pull_request.changes.any match ".yamato/com.unity.ml-agents-test.yml")
optional_deps:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yml to test the package with different project setups to ensure all tests pass with each optional dependency removed.

surfnerd
surfnerd commented Mar 17, 2021
View reviewed changes
com.unity.ml-agents.extensions/package.json
@@ -5,6 +5,7 @@
"unity": "2018.4",
"description": "A source-only package for new features based on ML-Agents",
"dependencies": {
"com.unity.ml-agents": "1.9.0-preview"
"com.unity.ml-agents": "1.9.0-preview",
"com.unity.modules.physics": "1.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add this for physics sensors in extensions.

@surfnerd surfnerd force-pushed the develop-physics-optional-dep branch from cb06ec5 to b4e832f Compare March 19, 2021 18:45
@surfnerd surfnerd enabled auto-merge (squash) March 19, 2021 20:07
@surfnerd surfnerd force-pushed the develop-physics-optional-dep branch 2 times, most recently from 78db448 to ff2184e Compare March 22, 2021 17:44
@surfnerd surfnerd force-pushed the develop-physics-optional-dep branch from 03ffbbf to acb9987 Compare March 22, 2021 20:11
@surfnerd surfnerd disabled auto-merge March 22, 2021 20:29
@surfnerd surfnerd enabled auto-merge (squash) March 22, 2021 21:45
chriselion
chriselion reviewed Mar 22, 2021
View reviewed changes
.yamato/com.unity.ml-agents-test.yml
testProject: DevProject
enableNoDefaultPackages: !!bool true
- version: 2019.4
enableCodeCoverage: !!bool true
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove enableCodeCoverage now, right?

chriselion
chriselion reviewed Mar 22, 2021
View reviewed changes
com.unity.ml-agents/Runtime/Sensors/RayPerceptionSensorComponent2D.cs
@@ -14,7 +14,7 @@ public class RayPerceptionSensorComponent2D : RayPerceptionSensorComponentBase
public RayPerceptionSensorComponent2D()
{
// Set to the 2D defaults (just in case they ever diverge).
RayLayerMask = Physics2D.DefaultRaycastLayers;
RayLayerMask = -5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this anymore, now that we're always using k_PhysicsDefaultLayers in the base class. (sorry, github wouldn't let me do a multi-line suggestion).

chriselion
chriselion approved these changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@chriselion chriselion left a comment

Choose a reason for hiding this comment

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

Two small nits, looks good otherwise.

@surfnerd surfnerd merged commit b25dc5a into main Mar 22, 2021
@delete-merged-branch delete-merged-branch bot deleted the develop-physics-optional-dep branch March 22, 2021 22:10
@surfnerd surfnerd mentioned this pull request Mar 22, 2021
Merged
@surfnerd surfnerd mentioned this pull request Apr 7, 2021
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chriselion chriselion chriselion approved these changes

@dongruoping dongruoping Awaiting requested review from dongruoping

@vincentpierre vincentpierre Awaiting requested review from vincentpierre

Assignees

@surfnerd surfnerd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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