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

Move GridSensor into main package #5256

Merged
merged 20 commits into from
Apr 14, 2021
Merged

Conversation

dongruoping
Copy link
Contributor

@dongruoping dongruoping commented Apr 14, 2021

Proposed change(s)

Move OneHotGridSensor and dependent files into main package. CountingGridSensor is still in extension with the concern of compression type limits.
By default GridSensorComponent creates a OneHotGridSensor.

Also added check if physics module exists since it's only optional dependency in package.

The rest of the changes are trivial: variable names, moving files, namespaces.

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

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

@@ -75,7 +75,7 @@ public IEnumerator RuntimeApiTestWithEnumeratorPasses()

// Can't actually create an Agent with InferenceOnly and no model, so change back
behaviorParams.BehaviorType = BehaviorType.Default;
#if MLA_UNITY_PHSYICS_MODULE
#if MLA_UNITY_PHYSICS_MODULE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

barely noticed the difference... not sure why this wasn't causing any trouble

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accidentally found a bug in test - this was never run and in fact the test will fail if this part was enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops, thanks.

@@ -1,230 +0,0 @@
# Summary
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted the doc since most of the information here doesn't apply anymore.
Only CountingGridSensor is in extension now and I don't think that worth a whole doc.

}

// Copied from GridSensorTests in main package
public static float[][] DuplicateArray(float[] array, int numCopies)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't find a good way to share utility methods between main package tests and extension tests

Copy link
Contributor

Choose a reason for hiding this comment

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

you can have the extension package depend on an asmdef that has this method in it. Then you can use it between both.

@@ -319,7 +319,7 @@ public int Write(ObservationWriter writer)
// For each ray, write the information to the observation buffer
for (var rayIndex = 0; rayIndex < numRays; rayIndex++)
{
m_RayPerceptionOutput.RayOutputs[rayIndex].ToFloatArray(numDetectableTags, rayIndex, m_Observations);
m_RayPerceptionOutput.RayOutputs?[rayIndex].ToFloatArray(numDetectableTags, rayIndex, m_Observations);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix RuntimeAPITest

Co-authored-by: Chris Goy <christopherg@unity3d.com>
docs/Migrating.md Outdated Show resolved Hide resolved
will be slightly different.

For creating your GridSensor implementation with custom data:
* To create custom GridSensor, derive from `GridSensorBase` instead of `GridSensor`. Besides overriding
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in the documentation somewhere (and refer to it from the migration guide)

dongruoping and others added 2 commits April 14, 2021 14:47
@dongruoping dongruoping merged commit 30fc6bd into main Apr 14, 2021
@delete-merged-branch delete-merged-branch bot deleted the develop-move-gridsensor-main branch April 14, 2021 22:26
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants