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

fix some quirks in World.step when attempting to account for variable framerate #18

Closed
wants to merge 4 commits into from

Conversation

Platane
Copy link

@Platane Platane commented Apr 9, 2020

Limit accumulator growth as proposed in schteppe#392

Also fix what seems to be a mistake where this.time is increase in world.step and at the same time in world.internalStep . I am unsure about the implications, this value was used to handle sleep status in bodies.

This fix unfortunately does not address #16 as I was still able to reproduce the midair freezing cubes. I notice it seems to happens less frequently though. The investigation continues :)

@drcmda
Copy link
Member

drcmda commented Apr 9, 2020

@Platane if you know your way around cannon as you seem to do, would you want to have github access? if this lib can grow faster it would be awesome and i think it needs more hands.

@drcmda drcmda requested a review from codynova April 9, 2020 18:44
@Platane
Copy link
Author

Platane commented Apr 9, 2020

@drcmda I am not very familiar with the technical aspect of the engine and solver unfortunately. But I can certainly give some of my time and do my best :)

@@ -407,14 +405,16 @@ export class World extends EventTarget {
substeps++
}

const t = (this.accumulator % dt) / dt
// prevent the accumulator to build up delay to catch up. The logic being: if the step did not catch up at this frame, it is unlikely to catch up in the next one. Even if it does, it will result in a speed up simulation which is arguably worse that staying behind the real time.
this.accumulator = Math.max(this.accumulator, dt)
Copy link
Member

Choose a reason for hiding this comment

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

so how does this behave diifferently than setting this.accumulator = this.accumulator % dt? Just a question, can't really wrap my head around what would happen differently 😅

Copy link
Author

Choose a reason for hiding this comment

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

That's a good question!
To my understanding, the accumulator is useful to catch up a frame when for example the simulation runs at 50fps and the step is 1/60ms. The accumulator growing greater than dt is an unexpected behaviour.

afaik, once we agree to discard some simulation time, it does not matter much if we carry the modulo or not.

I arbitrary decide to use a hard limit because of the semantic, as using a modulo might be interpreted as being important in the interpolation after ( which is not, the interpolation does not make any sense in case the accumulator gets truncate )

…step.

which allows to pass timeSinceLastCalled=0 and have a noop
@codynova
Copy link
Member

codynova commented Apr 21, 2020

Closing until we can resolve the simulation speed and freezing body issues, see #16

@codynova codynova closed this Apr 21, 2020
@codynova codynova mentioned this pull request Jun 23, 2020
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.

4 participants