Skip to content

Don't inherit from Academy, remove virtual methods #3184

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

Merged
merged 25 commits into from
Jan 10, 2020
Merged

Don't inherit from Academy, remove virtual methods #3184

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c241b97
WIP settings override for scenes
Jan 8, 2020
fb9cef9
fix default values
Jan 8, 2020
abee213
redo 3dball scene
Jan 8, 2020
5e21640
convert Basic scene
Jan 8, 2020
ad556a8
convert Bouncer
Jan 8, 2020
d6b3718
convert Bouncer
Jan 8, 2020
45cee86
add Monitor offset, convert Crawler
Jan 8, 2020
44c37a5
convert pyramids
Jan 8, 2020
f6889d7
update Reacher and Tennis
Jan 8, 2020
25627b4
commit walker
Jan 8, 2020
de8be79
Convert WallJump
Jan 8, 2020
d6b8b1e
convert pushblock
Jan 8, 2020
24afce2
convert Hallway
Jan 8, 2020
0905dea
fix hallway
Jan 8, 2020
d3760a6
convert GridWorld
Jan 9, 2020
465ddfd
fix HallwaySettings
Jan 9, 2020
c6f74f1
convert soccer
Jan 9, 2020
2cc0d96
remove TemplateAcademy
Jan 9, 2020
a241222
tooltips and remove saved settings from Academy
Jan 9, 2020
4ee978c
cleanup
Jan 9, 2020
4559afa
EnvReset delegate, remove virtual methods, convert FoodCollector
Jan 9, 2020
75243e0
forgot to add FoodCollectorSettings.cs
Jan 9, 2020
839794a
fix usings
Jan 9, 2020
61a9548
Doc changes for making Academy non-virtual (#3195)
Jan 10, 2020
eb7c0d3
Merge remote-tracking branch 'origin/master' into develop-settings-ov…
Jan 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion UnitySDK/Assets/ML-Agents/Examples/Crawler/Scripts/CrawlerAcademy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ public override void InitializeAcademy()
Monitor.verticalOffset = 1f;
Physics.defaultSolverIterations = 12;
Physics.defaultSolverVelocityIterations = 12;
Time.fixedDeltaTime = 0.01333f; // (75fps). default is .2 (60fps)
Time.fixedDeltaTime = 0.01333f; // (75fps). default is .02 (60fps)
Copy link
Contributor

@vincentpierre vincentpierre Jan 8, 2020

Choose a reason for hiding this comment

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

<opinion>
I do think we should avoid changing those in our environments.
</opinion>

Copy link
Contributor Author

@chriselion chriselion Jan 8, 2020

Choose a reason for hiding this comment

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

I don't know the history of them; since the scenes that override them are using ragdolls, I assume it was to improve stability. It might all change with the physx changes anyway.

Time.maximumDeltaTime = .15f; // Default is .33
}

Expand Down
52 changes: 52 additions & 0 deletions UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/SettingsOverrides.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
using UnityEngine;
Copy link
Contributor

@vincentpierre vincentpierre Jan 8, 2020

Choose a reason for hiding this comment

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

Is SimulationSettingsOverrides more appropriate?

Copy link
Contributor Author

@chriselion chriselion Jan 8, 2020

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor

@surfnerd surfnerd Jan 8, 2020

Choose a reason for hiding this comment

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

This is directly modifying what unity calls ProjectSettings, maybe ProjectSettingsOverrides?

Copy link
Contributor Author

@chriselion chriselion Jan 8, 2020

Choose a reason for hiding this comment

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

That works for me too. Since this will only be used in our scenes, I'm less worried about the naming or making sure that it handles all possible cases.

using MLAgents;

public class SettingsOverrides : MonoBehaviour
{
// Original values
Vector3 m_OriginalGravity;
float m_OriginalFixedDeltaTime;
float m_OriginalMaximumDeltaTime;
int m_OriginalSolverIterations;
int m_OriginalSolverVelocityIterations;

public float gravityMultiplier = 1.0f;
Copy link
Contributor

@vincentpierre vincentpierre Jan 8, 2020

Choose a reason for hiding this comment

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

Not sure if we should have gravity multiplier or gravity setting (with default at -9.81)

Copy link
Contributor Author

@chriselion chriselion Jan 8, 2020

Choose a reason for hiding this comment

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

All of the scenes today use a multiplier (but FloatPropertiesChannel use the negative of the value)

public float fixedDeltaTime = .02f;
public float maximumDeltaTime = .33f;

[Header("Advanced physics settings")]
public int solverIterations = 6;
public int solverVelocityIterations = 1;

public void Awake()
{
// Save the original values
m_OriginalGravity = Physics.gravity;
Debug.Log($"Time.fixedDeltaTime = {Time.fixedDeltaTime} Time.maximumDeltaTime={Time.maximumDeltaTime}");
m_OriginalFixedDeltaTime = Time.fixedDeltaTime;
m_OriginalMaximumDeltaTime = Time.maximumDeltaTime;
m_OriginalSolverIterations = Physics.defaultSolverIterations;
m_OriginalSolverVelocityIterations = Physics.defaultSolverVelocityIterations;

// Override
Physics.gravity *= gravityMultiplier;
Time.fixedDeltaTime = fixedDeltaTime;
Copy link
Contributor Author

@chriselion chriselion Jan 8, 2020

Choose a reason for hiding this comment

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

Most scenes only care about gravity, but Crawler and Walker override these deltaTime and solver iterations settings.

Time.maximumDeltaTime = maximumDeltaTime;
Physics.defaultSolverIterations = solverIterations;
Physics.defaultSolverVelocityIterations = solverVelocityIterations;

var academy = FindObjectOfType<Academy>();
Copy link
Contributor Author

@chriselion chriselion Jan 8, 2020

Choose a reason for hiding this comment

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

Will eventually be replaced with singleton access.

academy.LazyInitialization();

academy.FloatProperties.RegisterCallback("gravity", f => { Physics.gravity = new Vector3(0, -f, 0); });
}

public void OnDestroy()
{
Physics.gravity = m_OriginalGravity;
Time.fixedDeltaTime = m_OriginalFixedDeltaTime;
Time.maximumDeltaTime = m_OriginalMaximumDeltaTime;
Physics.defaultSolverIterations = m_OriginalSolverIterations;
Physics.defaultSolverVelocityIterations = m_OriginalSolverVelocityIterations;
}
}
3 changes: 3 additions & 0 deletions UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/SettingsOverrides.cs.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion UnitySDK/Assets/ML-Agents/Examples/Walker/Scripts/WalkerAcademy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ public override void InitializeAcademy()
// make walker joint calculations more accurate.
Physics.defaultSolverIterations = 12;
Physics.defaultSolverVelocityIterations = 12;
Time.fixedDeltaTime = 0.01333f; //(75fps). default is .2 (60fps)
Time.fixedDeltaTime = 0.01333f; //(75fps). default is .02 (60fps)
Time.maximumDeltaTime = .15f; // Default is .33

FloatProperties.RegisterCallback("gravity", f => { Physics.gravity = new Vector3(0, -f, 0); });
Expand Down