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

Avoid spiral of death when physics step get more time than rendering + add physics to frame profiler. #144

Open
wants to merge 3 commits into
base: master
from

Conversation

@and3md
Copy link
Contributor

and3md commented Nov 10, 2019

Avoid spiral of death when physics step get more time than rendering. When there are more than 5 steps per Update, next steps are abandoned and time reduced. Don't know how big should be default value, you can change it if you think it is too low/high.

This value can be changed by MaxPhysicsTicksPerUpdate in TPhysicsProperties. When you change it to 0 protection will not work. When you change it to 1 there will be one physics step per update so simulation will slow down on weak hardware.

Physics is in frame profiler now:

-------------------- FrameProfiler begin
Frame time: 0.02 secs (we should have 64.29 FPS based on this):
- BeforeRender: 0%
- Render: 94% (0.01 secs, we should have 68.38 "only render FPS" based on this)
  - TCastleTransform.Render transformation: 2%
  - TCastleScene.Render: 38%
    - ShapesFilterBlending: 2%
- Update: 5%
  - TCastleSceneCore.Update: 0%
  - Update Kraft Physics: 2% (0.00037 secs)
- Other:
-------------------- FrameProfiler end
@michaliskambi

This comment has been minimized.

Copy link
Member

michaliskambi commented Nov 21, 2019

Note from Andrzej, not to forget: look at Bullet's solution for this, similar to this PR, in https://github.com/bulletphysics/bullet3/blob/master/src/BulletDynamics/Dynamics/btDiscreteDynamicsWorld.cpp stepSimulation().

Copy link
Member

michaliskambi left a comment

I add some suggestions. In particular, maybe we can avoid slowing down the simulation to avoid the "spiral of death"? I propose some solution in the comments. Please test if see if my proposal makes sense :)

Note that Bullet does it exactly like you propose. So my proposition may very likely be wrong :)

@@ -22,6 +22,7 @@
FAngularVelocityRK4Integration: Boolean;
FLinearVelocityRK4Integration: Boolean;
FFrequency: Single;
FMaxPhysicsTicksPerUpdate: Integer;

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Dec 7, 2019

Member

Better to make FMaxPhysicsTicksPerUpdate a Cardinal, not Integer, since it cannot be < 0.

The check if PhysicsProperties.MaxPhysicsTicksPerUpdate > 0 below could then be just if PhysicsProperties.MaxPhysicsTicksPerUpdate <> 0.

next Update will get more time and leads to infinite time.
When you set it to 1 simulation will get max one step per Update, so on very
weak hardware physics will slow down instead of jumping. }
property MaxPhysicsTicksPerUpdate: Integer read FMaxPhysicsTicksPerUpdate write FMaxPhysicsTicksPerUpdate default DefaultMaxPhysicsTicksPerUpdate;

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Dec 7, 2019

Member

Some improvements to wording. Avoid starting the documentation of an item with "This..." (because all the documentation comments could start with "This..." :) ).

I propose such comment:

Non-zero value avoids the "spiral of death" when the physics takes
a long time to calculate.

When the value is zero, physics makes always as many steps as necessary,
to advance the time by @code(1 / @link(Frequency)) steps.
This means that if physics takes a long time to calculate,
next time it will take even longer time to calculate (it will need more steps),
thus we have a "spiral of death" that leads to lower and lower FPS.

Note that I removed the meaning of the sentence

When you set it to 1 simulation will get max one step per Update, so on very
weak hardware physics will slow down instead of jumping.

because I would prefer to change this behaviour, see later comments.

This comment has been minimized.

Copy link
@and3md

and3md Dec 7, 2019

Author Contributor

Good point! "This" is the first word that goes to my mind when I try wrote something in English. I will try to remember about that :)

@@ -1776,6 +1776,8 @@ TSceneManagerWorld = class(TCastleTransform)
FCameraPosition, FCameraDirection, FCameraUp: TVector3;
FCameraKnown: boolean;
FEnablePhysics: boolean;
{ Buffer to not count physics step time per frame }
FPhysicsTimeStep: TFloatTime;

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Dec 7, 2019

Member

Since this must always be synchronized with TPhysicsProperties.FFrequency, I propose to move this to TPhysicsProperties private field, and set always together with FFrequency.


Inc(PhysicsTicksCount);

{ This helps to avoid spiral of death when physics step get more time than rendering. }

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Dec 7, 2019

Member

Can we avoid slowing down the physics?

When trying to avoid the spiral of death, we could do one last step with larger time than FPhysicsTimeStep. I realize that using "variable time" with physics is not nice, but maybe it's better than slowing down the simulation? Because in your code we just subtract some time from TimeAccumulator without "covering" this time with physics simulation.

Can you try something like this within a loop, and see does it still avoid the "spiral of death" in your tests:

FKraftEngine.StoreWorldTransforms;

Inc(PhysicsTicksCount);

if (TimeAccumulator >= FPhysicsTimeStep) and
   (PhysicsProperties.MaxPhysicsTicksPerUpdate > 0) and
   (PhysicsTicksCount = PhysicsProperties.MaxPhysicsTicksPerUpdate - 1) then
begin
  { The last step may cover a larger time than usual FPhysicsTimeStep.
    This way we avoid the "spiral of death" by covering more time
    with a single step, if necessary. }
    
  StepTime := FPhysicsTimeStep * Floor(TimeAccumulator / FPhysicsTimeStep)

  WritelnLog('Physics ticks in TSceneManagerWorld.Update() reached ' + IntTostr(PhysicsTicksCount) + '.');
end else
  StepTime := FPhysicsTimeStep;
  
FKraftEngine.Step(StepTime);
TimeAccumulator := TimeAccumulator - StepTime;

I looked at Bullet code of https://github.com/bulletphysics/bullet3/blob/master/src/BulletDynamics/Dynamics/btDiscreteDynamicsWorld.cpp stepSimulation, and they do it just like in your code (they just "forget" about the excessive time, when calculating clampedSimulationSteps they simply reject the additional time). So it's possible that my idea has a fundamental flaw and I don't see it :) Feel free to reject it if it doesn't really work :)

This comment has been minimized.

Copy link
@and3md

and3md Dec 7, 2019

Author Contributor

Hi, I was thinking about this too. But I came to the conclusion that, unfortunately, such a solution can lead to strange bugs in specific conditions. I will write more in the general comment to make it more visible (I think it may be useful in the future - when someone wants to change this behavior).

@and3md

This comment has been minimized.

Copy link
Contributor Author

and3md commented Dec 7, 2019

Why did I abandon the idea of ​​taking the last long step to better synchronize physics? (I thought I wrote about this on discord, but maybe I forgot.)

From my understanding of how the physics engine works (still many things are incomprehensible to me, so maybe I am wrong then please somebody correct me). When the physics step takes place, new positions of physical bodies are recalculated, then collisions are checked and possible positions/velocities corrected. So I think there can be a lot of issues when the time will be not fixed.
The easiest way to present this is a drawing than to describe in text, so I made a simple sketch in Inkscape.

E.g. if we have a ball and a brick (as in my game ;)
When the physics steps are short and with a fixed length of time it looks like this:

normal

There are no problems here, in the next steps of physics the ball approaches the brick and when their positions overlap the engine detects a collision.

But there may be problems with fast-moving bodies, engine may "miss" the collision because in the next frame of physics the ball will be far behind the brick:

fast_body

That's why, intermediate steps (continuous collision detection) are used for such objects:

substeps2

Engine calculates intermediate steps to detect collisions. When the period of time between the steps of physics is constant it is easy to tell at what speed the body should be set as continous.

BTW. We haven't implemented continuous collision detection yet, but Kraft has it and I plan to do it in the next PRs.

I think that with a long step of physics there will be same problem as with fast bodies. In some situatuions the engine can calculate new position of ball far away from the curent one. And strange errors could occur, e.g. falling out of the level, etc.

long_step

I think losing this time in physics is not such a big problem. Usually it happens when loading a level or when the system does something in the background. For example when you install 50 updates on android phone you can notice that :)

I checked Bullet source code later and treated their approach as a evidence for you ;)

@michaliskambi

This comment has been minimized.

Copy link
Member

michaliskambi commented Dec 7, 2019

But there may be problems with fast-moving bodies, engine may "miss" the collision because in the next frame of physics the ball will be far behind the brick:

OK, I understand that. I see in both approaches we have some disadvantages, the question is which is worse:

  • Desynchronize time of simulation with the time perceived by user. (If we choose the solution from you and Bullet.) This may matter sometimes -- if someone e.g. tuned the projectile shot to reach target after exactly 2 seconds, then our mechanism will break it, it may reach the target after 2.5 seconds.

  • Potentially missing some collision, if the body will effectively move too fast to notice them (if we go with my approach). Although this problem can occur always (with physics engines that don't have continuous collision detection), but my approach would make this problem more unpredictable.

I feel persuaded by your comments (and the fact that Bullet does it). So let's go with it, OK.

(At least until we can use Kraft's continuous collision detection. Then I think my solution would be better.)

Indeed you explained it very nicely in your comment. I suggest you place a link to it in the implementation, like

{ To avoid the spiral of death, we ignore some accumulated time 
  (we will not account for this time in the physics simulation,
  so physics simulation may be slower than time perceived by user,
  than non-physics animations etc.).
  An alternative approach would be to prolong the simulation step
  sometimes, but this could lead to unreliable collision detection.
  See description of this in 
  https://github.com/castle-engine/castle-engine/pull/144#issuecomment-562850820
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.