Skip to content

Feature/imu#198

Open
jokimmortal wants to merge 30 commits intomasterfrom
feature/imu
Open

Feature/imu#198
jokimmortal wants to merge 30 commits intomasterfrom
feature/imu

Conversation

@jokimmortal
Copy link
Contributor

No description provided.

Copy link
Contributor

@FilipAlg FilipAlg left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +386 to +404
[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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this gizmo necessary? Since there is no local offset it is not really unclear where the sensor is

SensorEnvironment.Instance.Native?.remove( Native );

if ( Simulation.HasInstance ) {
Simulation.Instance.StepCallbacks.PostStepForward -= OnPostStepForward;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be post synchronize transforms instead? I can imagine users would like to be able to access IMU data in post step forward


OutputBuffer = new double[ outputCount ];

Simulation.Instance.StepCallbacks.PostStepForward += OnPostStepForward;
Copy link
Contributor

Choose a reason for hiding this comment

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

See other comment regarding callback

return false;
}

PropertySynchronizer.Synchronize( this );
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't do anything right? Native has not yet been created here

/// <summary>
/// Cross axis sensitivity
/// </summary>
[Tooltip("Cross axis sensitivity")]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
SensorEnvironment.Instance.GetInitialized();

if ( WheelRadius <= 0.0f ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Apply this constraint with the ClampAboveZeroInInspector attribute as well

Comment on lines +179 to +182
private double GetSignalResolution()
{
return 2.0 * System.Math.PI * WheelRadius / PulsesPerRevolution;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private double GetSignalResolution()
{
return 2.0 * System.Math.PI * WheelRadius / PulsesPerRevolution;
}
private double SignalResolution => 2.0 * System.Math.PI * WheelRadius / PulsesPerRevolution;

return null;
}

public double GetOutput( OdometerOutput output )
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be private?

[HideInInspector]
public bool IsWheelJoint => ConstraintComponent == null || ConstraintComponent is WheelJoint;

[RuntimeValue] public float SensorValue { get; private set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Cant we just use this property directly instead of having both OutputBuffer and this?

[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 );
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these values based on?

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