Conversation
FilipAlg
left a comment
FilipAlg
left a comment
There was a problem hiding this comment.
I think more tooltips should be added and I don't see a point to disabling editing of everything during runtime. This to me is one of the great strengths of AGXUnity
| [NonSerialized] | ||
| private Mesh m_nodeGizmoMesh = null; | ||
|
|
||
| private void OnDrawGizmosSelected() | ||
| { | ||
| #if UNITY_EDITOR | ||
| if ( m_nodeGizmoMesh == null ) | ||
| m_nodeGizmoMesh = Resources.Load<Mesh>( @"Debug/Models/HalfSphere" ); | ||
|
|
||
| if ( m_nodeGizmoMesh == null ) | ||
| return; | ||
|
|
||
| Gizmos.color = Color.yellow; | ||
| Gizmos.DrawWireMesh( m_nodeGizmoMesh, | ||
| transform.position, | ||
| transform.rotation, | ||
| Vector3.one * 0.2f ); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Is this gizmo necessary? Since there is no local offset it is not really unclear where the sensor is
There was a problem hiding this comment.
Nope, not necessary, just some flair to more easily see that the GO has a sensor if there are multiple components. Could remove this if you want.
AGXUnity/Sensor/ImuSensor.cs
Outdated
| SensorEnvironment.Instance.Native?.remove( Native ); | ||
|
|
||
| if ( Simulation.HasInstance ) { | ||
| Simulation.Instance.StepCallbacks.PostStepForward -= OnPostStepForward; |
There was a problem hiding this comment.
Should this be post synchronize transforms instead? I can imagine users would like to be able to access IMU data in post step forward
There was a problem hiding this comment.
Sounds reasonable
AGXUnity/Sensor/ImuSensor.cs
Outdated
|
|
||
| OutputBuffer = new double[ outputCount ]; | ||
|
|
||
| Simulation.Instance.StepCallbacks.PostStepForward += OnPostStepForward; |
There was a problem hiding this comment.
See other comment regarding callback
AGXUnity/Sensor/ImuSensor.cs
Outdated
| return false; | ||
| } | ||
|
|
||
| PropertySynchronizer.Synchronize( this ); |
There was a problem hiding this comment.
This doesn't do anything right? Native has not yet been created here
There was a problem hiding this comment.
I don't have a feel for when it's useful or not, nor do I understand from reading the comment if it's necessary or how to test if it makes a difference. Moving to end of initialize for now.
AGXUnity/Sensor/ImuSensor.cs
Outdated
| /// <summary> | ||
| /// Cross axis sensitivity | ||
| /// </summary> | ||
| [Tooltip("Cross axis sensitivity")] |
There was a problem hiding this comment.
It's not clear to me just reading this what this property does. Setting it > 1 seems to yield NaN values which suggests that some range should be applied
There was a problem hiding this comment.
Added better tooltip and clamping 0-1
| { | ||
| SensorEnvironment.Instance.GetInitialized(); | ||
|
|
||
| if ( WheelRadius <= 0.0f ) { |
There was a problem hiding this comment.
Apply this constraint with the ClampAboveZeroInInspector attribute as well
There was a problem hiding this comment.
WheelRadius field already has that constraint. I suppose I could remove the check at line 109 but I don't see the need
AGXUnity/Sensor/OdometerSensor.cs
Outdated
| return null; | ||
| } | ||
|
|
||
| public double GetOutput( OdometerOutput output ) |
There was a problem hiding this comment.
Yes, also in IMU
| [HideInInspector] | ||
| public bool IsWheelJoint => ConstraintComponent == null || ConstraintComponent is WheelJoint; | ||
|
|
||
| [RuntimeValue] public float SensorValue { get; private set; } |
There was a problem hiding this comment.
Cant we just use this property directly instead of having both OutputBuffer and this?
There was a problem hiding this comment.
They have slightly different uses, one uses the runtimevalue attribute to be read in the inspector, the other uses the consistent outputbuffer name used by the similar sensors. But it's of course quite possible not to use this distinction. I have no strong opinions here because I'm not sure how user will use the sensor output.
| [Tooltip("Set the field vector of the uniform magnetic field used in the simulation [in Tesla]")] | ||
| [HideInRuntimeInspector] | ||
| [DynamicallyShowInInspector( "UsingUniformMagneticField" )] | ||
| public Vector3 MagneticFieldVector = new Vector3( 19.462e-6f, 44.754e-6f, 7.8426e-6f ); |
There was a problem hiding this comment.
What are these values based on?
There was a problem hiding this comment.
AGX manual defaults
Co-authored-by: Filip Henningsson <114672787+FilipAlg@users.noreply.github.com>
Co-authored-by: Filip Henningsson <114672787+FilipAlg@users.noreply.github.com>
Co-authored-by: Filip Henningsson <114672787+FilipAlg@users.noreply.github.com>
2 participants
No description provided.