RayPerception sensor #2874
RayPerception sensor #2874
Conversation
| @@ -1,10 +1,8 @@ | |||
| using System.Collections.Generic; | |||
There was a problem hiding this comment.
Cleanup suggested by Rider.
| } | ||
|
|
||
| var subList = new float[detectableObjects.Length + 2]; |
There was a problem hiding this comment.
Got rid of this, and just wrote to the buffer with an offset instead.
| @@ -10,9 +11,6 @@ namespace MLAgents | |||
| /// </summary> | |||
| public class RayPerception3D : RayPerception | |||
| { | |||
| RaycastHit m_Hit; | |||
| float[] m_SubList; | |||
There was a problem hiding this comment.
Same comment as above. Also RayCast hit is a struct so we don't need to keep it around (I think, correct me if I'm wrong)
| // TODO reenable checks there are enough Visual Observation Placeholder in the model. | ||
| // If there are not enough Visual Observation Input compared to what the | ||
| // sensors expect. | ||
| var visObsIndex = 0; |
There was a problem hiding this comment.
There were removed in https://github.com/Unity-Technologies/ml-agents/pull/2731/files#diff-bbd30aae9fbe4211cf7e967224f2864dL242
Add them back here (and also check for not enough visual inputs, which was missing earlier).
| Transform transform, float[] perceptionBuffer, | ||
| bool legacyHitFractionBehavior = false) | ||
| { | ||
| Array.Clear(perceptionBuffer, 0, perceptionBuffer.Length); |
There was a problem hiding this comment.
This was migrated (and improved IMHO) from the old code.
|
|
||
| bool castHit; | ||
| RaycastHit rayHit; | ||
| if (castRadius > 0f) |
There was a problem hiding this comment.
The size of the spherecast is now adjustable. If set to 0 (or smaller) it will use raycasts instead.
| [Tooltip("Whether to stack previous observations. Using 1 means no previous observations.")] | ||
| public int observationStacks = 1; | ||
|
|
||
| // TODO layerMask for raycasts |
There was a problem hiding this comment.
See https://jira.hq.unity3d.com/browse/MLA-16 - would be nice but not sure if there's an easy way to make it look good in the editor.
| /// Returns the shape of the sensor observations that will be created. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public abstract int[] GetObservationShape(); |
There was a problem hiding this comment.
Added this to the editor components; they now have to return the size that the sensors will create.
| /// <returns></returns> | ||
| public abstract int[] GetObservationShape(); | ||
|
|
||
| public virtual bool IsVisual() |
There was a problem hiding this comment.
Not strictly needed, but I thought it made some of the code cleaner.
UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/RayPerception.cs
Show resolved
Hide resolved
|
Will this have a follow up with docs changes? |
| @@ -150,7 +151,7 @@ public void MoveAgent(float[] act) | |||
| { | |||
| var myTransform = transform; | |||
| myLaser.transform.localScale = new Vector3(1f, 1f, m_LaserLength); | |||
| var position = myTransform.TransformDirection(RayPerception3D.PolarToCartesian(25f, 90f)); | |||
| var position = myTransform.TransformDirection(RayPerceptionSensor.PolarToCartesian(25f, 90f)); | |||
There was a problem hiding this comment.
Should this be 3DRayPerceptionSensor to show this is 3D?
There was a problem hiding this comment.
Should PolarToCartesian live in some Utility class? Should the Debug.DrawRay be part of the sensor instead ?
There was a problem hiding this comment.
Let me try to clean some of this up a bit. I'd lean towards not doing any trig functions at runtime, and either save a quaternion "base" and "increment", or storing the local space vectors on the sensor.
There was a problem hiding this comment.
🤦♂ This should just be "25.0 * transform.forwards" since the angle is hardcoded to 90
There was a problem hiding this comment.
Cleaned this part up. It had no need for any RayPerception* code here. Note this is "game logic" inside MoveAgent(), not actually related to observations.
| /// <summary> | ||
| /// Converts polar coordinate to cartesian coordinate. | ||
| /// </summary> | ||
| public static Vector2 PolarToCartesian(float radius, float angle) | ||
| { | ||
| var x = radius * Mathf.Cos(DegreeToRadian(angle)); | ||
| var y = radius * Mathf.Sin(DegreeToRadian(angle)); | ||
| var x = radius * Mathf.Cos(RayPerceptionSensor.DegreeToRadian(angle)); |
There was a problem hiding this comment.
Same comment as in PolarToCartesian. Is there a better place to put these ?
There was a problem hiding this comment.
Also made the PolarToCartesian methods private in their respective classes.
There was a problem hiding this comment.
This is folded into RayPerceptionSensor now.
| } | ||
|
|
||
| const float castRadius = 0.5f; | ||
| const bool legacyHitFractionBehavior = true; |
There was a problem hiding this comment.
I feel this adds complexity. It is fair to say, you need to retrain when updating. I don't think we should have legacy code in a beta project.
There was a problem hiding this comment.
Agreed it adds complexity. Disagree that we shouldn't support it, at least for one more release. The old code is marked as [Obsolete] and we can remove it after the next release ships.
| { | ||
| if (input.shape.Length == 4) | ||
| { | ||
| if (input.name.StartsWith(TensorNames.VisualObservationPlaceholderPrefix)) |
There was a problem hiding this comment.
Magic strings... But I don't see an alternative without changing the way the trainer code works... @ervteng any refactor of the trainers (related to model inputs) planned?
There was a problem hiding this comment.
Yeah, I don't love it either. There are several other places in the (resurrected) barracuda code that check for this too. Trainer refactor is probably not sufficient, will likely need some tf2bc.py changes too.
There was a problem hiding this comment.
I wasn't planning on changing how the trainer detects these... but that was because I was assuming it wasn't going to change on the C# side :P would the alternative be having a "type" for the observation that's an enum or similar?
I think the tricky thing about the tf2bc part is that it uses the TensorFlow tensor names to determine what to put as input and output, and those are limited to being strings.
Definitely will add a migration guide for it. I don't think there are any docs on the old code, but I can find a place to add a writeup on the new stuff. |
UnitySDK/Assets/ML-Agents/Scripts/Sensor/RayPerceptionSensorComponent2D.cs
Outdated
Show resolved
Hide resolved
I also made some adjustments to the raycast logic on how it handles hit perception. Hits against known object tags are the same but:
I think these make sense and are more "continuous", i.e. an object just at the end of the ray will have a hit fraction of .99, and if the object moves out of range, the hit fraction increases to 1.0. The old behavior can be enabled via a flag (RayPerception3D uses it but the sensor doesn't).
Rays are evenly distributed in a "cone" in front of the agent. There's always a ray at 90 degrees, and also "rays per direction" to the left and right, up to "max rays degrees". This is a bit more restrictive than the old system, but also means you only need parameters. Can change to a list of angles (or list of offsets) if that might be better.
Currently only converted WallJump and trained a new model for it. Can update other scenes later once this is merged.
https://jira.hq.unity3d.com/browse/MLA-310 and https://jira.hq.unity3d.com/browse/MLA-311 are followup jiras to improve the editor UI and debug drawing respectively.