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

Arcade Collision Detection #1424

Closed
Jack0815 opened this issue Dec 8, 2014 · 37 comments
Closed

Arcade Collision Detection #1424

Jack0815 opened this issue Dec 8, 2014 · 37 comments

Comments

@Jack0815
Copy link

Jack0815 commented Dec 8, 2014

I just updated to 2.2.1 and was looking forward to new features and fixes. But with the new update I get a lot of trouble with the collision detection.
An example is just the breakout-example of phaser itself. When I play it for quite some time (besides the fact that the paddle is flickering aournd some times) some of the collisions are not recognized and the ball just flies through the bricks. I have similar issues with 2 other games. It happens often, if you have hits directly followed by each other (e.g. between two lines of bricks in breakout).

Setup:
Phaser 2.2.1
Tested on:
Desktop Browser: Firefox (up-to-date)
Cordova-Build: Nexus 5 (up-to-date)

EDIT: After turning my game into SlowMotion (to see the debug bounding boxes) it did not happen so far. BUT it did also not happen with earlier Phaser versions.

@pnstickne
Copy link
Contributor

@Jack0815 This may be related to the new fixed-step system. Which, should handle this case better and more predictably than before..

I suspect the core issue is that the ball moves /PAST/ the bricks (or at least more than half-way through) each time the physics is applied - the Arcade physics only checks the current positions, and not the paths.

Make sure that game.time.desiredFps is at high enough such that the ball cannot move more than halfway through a brick in "1 frame" (~16ms @ 60FPS).

I could not readily reproduce the ball-jump/tile-skip issue on my main desktop (which is "beefy"); I'll look at it more in depth later.


The paddle flicker sounds like another issue - please create a separate issue for it with more detail, thanks! (I've also experienced "paddle flicker" in Chrome, but it feels more like an input-related issue and unstable positioning.)

@pnstickne
Copy link
Contributor

Whelp, I can confirm this. When the actual fps is << target fps the breakout game goes "berserk".
(Chrome at 10FPS will "Game Over" immediately after the first click as the ball flies off the screen.)

I'm not sure exactly what the problem is; it could simply be an "excessive delta difference" in the ball velocity update functions.

@Jack0815
Copy link
Author

Jack0815 commented Dec 9, 2014

Thanks for the update. I realised that with a higher fps it does not happen that often in my game. But still it happens with desiredFps set to 90 ... I can not set the speed of the ball much slower (due to difficulty issues ;-) ) and a higher framerate seems to be a bit ridiculous to me. Should I switch to p2 or any other suggestions?

@photonstorm
Copy link
Collaborator

Fixed step can't deal with tunnelling. The old system at 10fps would exhibit the exact same behaviour. Pushing the fps higher than 60 is pointless unless you're running a 120Hz monitor and will lead to more miscalculations.

What is needed is a proper CCD system, but we don't have it in AP and aren't likely to add it any time soon, although I did have an idea how it could be implemented the other day, so may investigate if I get time.

@pnstickne
Copy link
Contributor

Shouldn't the fix-step prevent tunneling with fixed-physics calculations and a fixed (eg. dropped catch-up) cap?

The behavior I saw seemed "inexplicably" severe and can be reproduced when running a Chrome Heap Allocations profiler when trying to play breakout.

@photonstorm
Copy link
Collaborator

No of course not, an object can still be travelling too fast for even a fixed-step to pick up. What it would require is the objects previous position, calculated new position, and to then step every point between them doing full collision checks on the way.

Or it could cast a ray from the objects old position to its new destination, then intersect check each point of the ray against all possible objects, but that only works if the objects being compared are similar sizes.

@pnstickne
Copy link
Contributor

@photonstorm But with the fixed-step, wouldn't this require the object to be going fast enough to be missed at both the 0 and 1/TargetFPS logic checks?

The previous model would have checked at 0 and ElapsedMS, where ElapsedMS >= 1/TargetFPS (assuming TargetFPS ~ 60), and at a lower actual-FPS, ElapsedMS >>> 1/TargetFPS which should have made tunneling previously much more severe under lower actual-FPS.

With the fixed step model I would expect logic checks to always occur at 0 and 1/TargetFPS - and thus exhibit better stability and reduces tunneling characteristics.

Assuming the desired FPS is left at 60 this would always be stable ~16ms logic updates; yet the observed behavior is not consistent with the above expectations. (I could very well be missing a simple detail.)

@pnstickne
Copy link
Contributor

I would also expect with the new fixed-step model updating the target FPS to 120 should halve the tunnel-afflicted zone; as the physics calculations are done (possibly multiple times each RAF update) based on 1/TargetFPS.

This won't affect the "actual render FPS" past 60FPS and may result in game/physics-time slowdowns (due to update-skips if more than 3 are dropped per RAF and/or if spiraling kicks in), but the fixed step system should handle this case better (and more consistently) than 2.1, not worse as reported.

@pjbaron
Copy link
Contributor

pjbaron commented Dec 9, 2014

Yes you're correct, the fixed time step will help with this... and the breakout example ball doesn't go fast enough to cause tunnelling if the desiredFps is set at 60 because all time steps are exactly 1/desiredFps so the speed would have to be extremely large.
Skipped frames are another matter, and if you have a desiredFps of 60 and an actual fps of less than 30 it will almost certainly be constantly 'spiralling'. This is bad, which is why there's a call-back to warn you and let you fix it.
When the system spirals, all bets are off in regards to stability improvements from the fixed time step system... it's trying to catch up dropped frames and falling further behind each refresh so eventually (very quickly) it has no option but to throw away the dropped frames entirely.
@Jack0815 (or anyone else who can reliably recreate this problem), could you please add in the callback to detect spiralling... don't handle it, just drop in a console.log message with the suggestedFps it comes back with? Then run the breakout demo until you see one of these problems and take a look at the console to see if the message has appeared.
I'll try to reproduce here but even my 6 year old laptop is capable of running these demos without breaking a sweat and I haven't got a debug browser on any of my tablets (I don't root them because I need to know what the end-user of my games is experiencing). Maybe I can introduce some dead loops to increase the load...

@pjbaron
Copy link
Contributor

pjbaron commented Dec 9, 2014

BTW: if the system is already spiralling and you increase the desiredFps then you'll make it significantly worse. Increasing to 120 means it will try to do two logic updates for every render update instead of one. If the target system can't do one reliably there's no way it'll handle more...

@pnstickne
Copy link
Contributor

@pjbaron I can recreate "non playable behavior" - much worse than that described, even - by simply running a Chrome Heap Allocation profile. Works like a charm to load the system, even on a fairly beefy desktop.

Also while spiraling can delay physics calculations, it should not affect the consistency of physics calculations that are done - this includes Arcade Physics, Tweens, and Sprite lifetimes. (It does not include general Timers or other code which is based off of the general elapsed "real world"-time.)

At the "worst" it should cause a noticeable game-time (aka "physics/bullet-time") slow-down. Yet every calculation done is based on the 1/FPS (where it is stable in fixed-step) and should thus be predictable: they are done at 1/FPS (0 .. 3 per updateRender) before continuing to the next 1/FPS updateLogic calculation.

That is, in game-time (aka "physics/bullet-time"), a updateLogic is not skipped in relation to physics/body calculations; but it may be delayed to a subsequent game update/render.

Since the updates are at 1 / Desired FPS then any tunneling or collision-miss effect should be less pronounced and more consistent (especially when running at sub-60 FPS rendering) than in 2.1, as described above: but the opposite behavior is being reported.

While detecting spiraling is useful, as long as the time.desiredFps is constant/stable (Phaser suggests, but does not change it) then the physics should also be stable.

There is a bug here, somewhere. I believe the logic behind the fixed-step system is generally sound in reducing tunneling and making the physics more consistent. This problem needs a resolving exploration/fix and should not simply be brushed off.

@pjbaron
Copy link
Contributor

pjbaron commented Dec 9, 2014

I can't seem to get any effect like that even if I use the 'power saver' profile to reduce my maximum CPU power. The bat flicker is caused by bounds checking I believe, and Rich found an easy fix, just change paddle.body.x to paddle.x which is probably how it should have been done in the first place (adjusting body directly is dubious in all physics systems).

@photonstorm
Copy link
Collaborator

I don't believe the issue is anything to do with the fixed-step code itself. The logic is sound.

I will investigate further tomorrow, but I'm convinced it's because the logic and render steps are deeply coupled, when they really shouldn't be.

The Sprite relies on its world property being populated by worldTransform. This is fed into Body.preUpdate (and elsewhere) and used for all AP physics. The delta values are then passed back into Sprite.position.

However if the game doesn't render then Pixi never updates the worldTransform. The Sprites new position doesn't get merged with the transform or any children. Thus if physics updates say twice, or more, because of the fixed-step / spiralling, then I'd expect the values may start going mental pretty quickly. I've got a proper test harness here to step through it, which I'll do tomorrow, but I'm pretty certain this is the root of it.

photonstorm added a commit that referenced this issue Dec 9, 2014
@pnstickne
Copy link
Contributor

@pjbaron In any case, [extreme] reproduction is simple: profile Heap Allocations in Chrome. I've found several subtle bugs that only occur in less-than-ideal situations when doing so.

@pnstickne
Copy link
Contributor

@photonstorm With the updateTransform suggestion in place I can no longer reproduce any of the reported issues - the game is playable, the paddle no longer seems errant, and the ball does not pass through any tiles.

The game runs predictably, albeit not very smooth, at ~10 actual fps (60 target fps, 10 suggested fps).

@pjbaron
Copy link
Contributor

pjbaron commented Dec 10, 2014

Excellent, nice job Rich!
Now we're pretty certain what the problem is, we just need to find a good solution!

@Jack0815
Copy link
Author

Thanks for all the interest and feedback concerning this issue. Unfortunately, as I have not yet had the time to take a closer look into the phaser source code itself, I have no suggestion for a solution. Hope someone with more knowledge has an idea ;-)
Greetz from Tanzania (reason for some time delay)

@photonstorm
Copy link
Collaborator

Well it's good to know my hunch was right, but frustrating at the same time :)

PIXI.DisplayObject.updateTransform looks pretty expensive to me. That's a load of temp. vars being created each call, although at least it's just plain multiplication on them I guess. Even so it seems like something worth avoiding.

I'm wondering if there's an easier way - perhaps modifying the worldTransform directly, which would then be overwritten when Pixi renders anyway, but may serve its purpose for the sake of AP. My worry is if there are then any child/parent transform relationships that wouldn't take care of.

photonstorm added a commit that referenced this issue Dec 10, 2014
…d the Pixi version.

Added a boolean check, so it can be either updated from updateLogic or render without duplicating the process.
#1424
@photonstorm
Copy link
Collaborator

Ok I've just pushed up a new version that I hope will address this better.

The previous attempt would only work if the display list was flat, or the parent was always updated before the children, but this can't be guaranteed. So now it updates all transforms across the whole display list once per updateLogic, as it needs to to keep physics in sync, but skips doing this twice if render doesn't need it.

Would be good to know if this still works for you both. I'd assume so, but.. :)

@Jack0815
Copy link
Author

Just pulled and built it. Looks promising ;-)
Even in lower framerates in my game the ball no longer flies through bricks. Thanks for the real fast fix.

@photonstorm
Copy link
Collaborator

Awesome. I'll do some wider testing, but this looks good for 2.2.2.

@pnstickne
Copy link
Contributor

On a desktop browser, I've not observed updateTransform as a hot-spot or bottleneck from a performance standpoint, and I've run more than a few profiles from standard to "extreme" loads.. of course calling it twice per render/update may be wasteful (depending on the nuances of other update interactions), but it has little/no relevant impact when only called as often as a physics update.

While the method does have a good number of variables, it generates almost no objects (excluding on how browsers implement non-immediate numbers internally) and the math operations can be well optimized locally. Modern JIT browsers are highly optimized for this usage (even pre-ASM) and are able to play Quake / Unreal Tournament and even virtualize Linux (well, on non-mobile devices, anyway).

In my experience, local non-escaping variables (that are monomorphic) are cheap when in "optimizable functions": the impact from such is only when the method creates objects (assigned to said variables), especially if the new objects are not short-lived and persist outside the method call, or if the variables / function scope must be bound in a closure or otherwise de-optimized.

I've not much experience with working on or profiling mobile devices, but I expect they have similar characteristics of not spending much (relative time) in updateTransform.

(Any performance impact that there is from updateTransform is also marginalized by the increasing load on the rendering and physic sub-systems as expected in usage.)


That being said, none of the current mobile (ARM-based) processors have nearly the math (ALU or FPU) throughput that a modern x86 has, which may slightly skew the results. I've also little experience with Safari or the non-JIT JS engines (which is only pre-iOS7 these days?) such as JSC+LLInt. But I doubt applying updateTransform (even with a potentially unnecessary duplication) will be a bottleneck that needs addressing as such lesser performance / optimization is applied across the entire system.

@photonstorm
Copy link
Collaborator

Closing this off as I've had several validations from forum members that this version is working much better for them.

@rendermouse
Copy link

I just tried the minified Phaser 2.2.3 on the "Making your first game" tutorial. Setting a gravity of 900 on falling objects still causes physics fall-through in my project, at least in Chrome...

@pnstickne
Copy link
Contributor

@rendermouse
Is this a regression from 2.1.3? That is, which version(s) are affected? (I almost always play off a local "dev" branch, so 2.2.x only filters through secondarily.)

While the fixed-step should be better and more consistent overall (as reasoned above), it does not eliminate the issue of two objects "moving past" each other within a single step of some finite size.

If it's not a regression ("introduced bug" with the same code), it's just a limitation ("not suitable approach") for the given scenario - and being able to clearly identify which, e.g. providing a comparison against 2.1.x for the same code, can help track down the core issue.

@rendermouse
Copy link

Running this tutorial: http://www.photonstorm.com/phaser/tutorial-making-your-first-phaser-game
I am using 'part9.html' in the downloadable assets on a local http server. Stars fall from the top and should bounce on the top of the platforms.

I seem to get less pass-through on 2.1.3 than any 2.2.? release, but I can see the issue in each version, when gravity.y is turned up to 900. Sometimes the entire star collection fails as a group and simply falls through the map, which is odd. Sometimes a single platform will fail collision on all stars (see attached). Sometimes it is just individual stars that fall through.

The stars in this diagram fell through the platform, then bounced up from the ground. Perhaps this issue is related to the way the tutorial is coded, and not an issue with the physics engine itself, but as a new Phaser coder, I have no way to know.

image

@pnstickne
Copy link
Contributor

@rendermouse Thanks for the link and explanation across versions.

If the gravity makes the stars fall so fast (that they pass through a single brick layer) than that could cause the results if they "bounce" on an underneath layer or the world-bounds. The corrective action (assuming there is not a small issue with the "push out" logic) is to limit the speed at which the stars can fall.

In Phaser 2.2 the stars must move no more than "1/2 tile distance" in 1/desiredFps (usually 1/60fps or ~16ms) to get guaranteed results. (The rules with Phaser 2.1 - and Phaser 2.2 before the patch discussed above - were tied to the render update performance.)

I'll try to dedicate some time this evening to look at the particular example.

@pnstickne
Copy link
Contributor

@rendermouse Well, "duh" - there is no such fixed 2.2.3 release, you got me too - I thought there was a point release in :person_frowning:

The Phaser 2.2.1 (where is 2.2.3, means 2.1.3?) build does not have the important this.updateTransform(); update as discussed above and will thus exhibit incorrect and unpredictable behavior if there is ever a render frame skip.

Once the change described above is put into the Sprite postUpdate the physics behaves predictably - whereas 2.1.3 does not behave predictable, and 2.2.1 non-patched let stars fall (very easily, and not predictably) due to the missing world synchronization.

With the update there is still a maximum velocity, but it is above that which is obtained by the stars with "10000 gravity units"!

@rendermouse
Copy link

OK, I am sorry if I caused confusion. I started with the latest fixed release, which was 2.2.1, and had issues. I then went back and tried 2.1.3, hoping the example would work better in a previous version. I still had issues, and read the comments in this thread I thought there had been a recent dev fix, so I tried the code from the dev branch which was labeled 2.2.3 (though the comments in the code actually say 2.2.2).

@pnstickne
Copy link
Contributor

@rendermouse To continue happily using Phaser (with fixed physics), I'd used either the 2.2.1 or dev branches + the applied fix.

You'll have to rebuild the applicable .js file - but otherwise the change should fix the issue. Hope you can get on to more fun game making after that!

(I don't see the fix itself in the dev branch yet.)

@kabuhr
Copy link

kabuhr commented Dec 30, 2014

As per issue #1479, there's a problem with the proposed fix 028943b when there are multiple updates per render. In addition to the visual issues, I've been able to reproduce the original missed collisions problem even with the patch applied.

The problem is that the arcade engine sets the initial body.position based on body.sprite.world coordinates in Body.preUpdate then updates the sprite position by assigning to sprite.x and sprite.y in Body.postUpdate. Unless updateTransform is called after every postUpdate to update the world coordinates, the next preUpdate will re-use an old world coordinate position for physics calculations, so the above patch (which skips transformUpdates after the first in a "render set") allows a moving sprite to be checked multiple times at an old position before jumping ahead several frames for the next collision check (as I've verified with a toy ball-and-wall example).

I'd instead propose calling updateTransform on an as-needed bases on sprites that are actually moved by the engine in Body.postUpdate. That fixes the collision problem for me and limits the number of updateTransform() calls to the minimum necessary. (See pull request #1493)

@pnstickne
Copy link
Contributor

I concur that the updateTranform must be called after every "logic update".

Anyway, some observations about having the update transform done by the Physics:

  • updateTransform is relatively inexpensive (as in less than 0.1%, even with hundreds of particles). This is per desktop analysis; but I do not believe there is an overall performance impact with calling this operation as often as 1/desired_fps - and the relative performance is not significant concern.
  • This change needs to be applied for all Physic types, not just Arcade. Just something to keep in mind.
  • This would result in the current odd behavior is the user moved the sprite (not via Physics) during a "logic update".
  • This change results in a non-atomic view (of the transformation) during the "logic update". Not that there is a strong guarantee anyway.

I'm a fan of calling updateTransform after every postUpdate (or "logic update"), but allowing it to be skipped in the render ("render update") if there was a postUpdate ("logic update") for that game loop cycle. This would ensure the contract of calling it after each postUpdate is met, but avoid it being called twice when only a single logic update is applied - This is a slight simplification of the modification to each Sprite update that is also universal across all game objects and is similar to the original proposed fix (that most people seem to agree "works" for them).

@photonstorm
Copy link
Collaborator

The more I think about this the more I think it needs decoupling from physics or even Sprite.postUpdate at all, and just called on a global stage level basis every single logic update (in the core game loop). This would propagate the entire display list.

The reasoning is that while it makes sense that a Sprite should do it in its own postUpdate, this breaks if the Sprite is part of a Group that is itself being modified. For example a Group being tweened (ala a space invaders group of aliens) and yet each alien is needing its own physics updates applied too. If we only update the aliens in their own postUpdate then their coordinates will be wrong because the parent wasn't taken into consideration.

This actually simplifies everything - just calling it in the core game loop and should cover all bases fully. It's also the easiest to implement.

pnstickne added a commit to pnstickne/phaser that referenced this issue Jan 2, 2015
This change implements the original suggestion of using `updateTransform`,
but applies so globally instead of within a particular postUpdate
function.

Now the game loop calls `updateTransform` after each `updateLogic` call
unconditionally; it is updates that change the world that are accounted
for, not the rendering. This removes some previous checks that were
preventing correct behavior with the previous patch.

This makes the assumption that game objects (eg. Sprites) are only
modified within callbacks triggered before the completion of the
`postUpdate` walking of the scene graph.
- User code that runs outside of the "game update", such as a `setTimeout`
  timer, will need to explicitly update transformations so that the world
  is synced by the next `preUpdate`: but this is not the expected case and
  is already outside the Phaser update model.
- If this assumption does not hold or is too weak, the transformations
  could also be applied once at the start of every game update loop
  (before any render or update). This change would at most double the time
  spent on apply the transformations.

The constant application of `updateTransform` passes all reported failing
cases and resolves phaserjs#1424 just as the original proposal of having the
change performed in the Sprite postUpdate but will work more consistently
across all scene-bound game objects.

On a desktop Chrome browser the inclusion also has minimal relative impact
as shown by the summarized results. The percentages given are the summed
CPU time of relevant required operations along with that of the
updateTransform itself:

- 10,000 non-collision particles:
  - 12% pre/post update, 2.4% updateTransform
- 100 colliding particles:
  - 2% pre/post update & collision, 0.3% updateTransform
- 1000 colliding particles:
  - 40% pre/post update & collision, 1% updateTransform

With this patch the updateTransform time does creep up _slightly_ (vs just
in `Sprite.postUpdate`) but it is still dominated by required game
updates, and more so, by any actual work like Physics.
@pnstickne
Copy link
Contributor

Well, there is my proposal. It works with the failing cases reported and the assumption / rationale is outline in the PR.

The current dev code does not work correctly; I think it might be a slight logic issue, but I opted to forgo such checks after establishing the assumption stated within the PR.

@pjbaron
Copy link
Contributor

pjbaron commented Jan 2, 2015

Coming late (and poorly informed, and a little drunk) to this discussion (caveat emptor):

  • iirc updateTransform is local to the display object only, it doesn't traverse the parent tree
  • groups process individuals within themselves for various update stuff that might change their transform matrix
  • groups can be held within other groups (?) potentially causing complex and hard to trace update chains

If I'm correct in these three things then the most robust way to deal with the transforms is to process the entire display list, once per 'logic update', and in a single function call - basically what Rich said. Anything else seems guaranteed to fail given appropriate circumstances (e.g. parent manipulation by children, especially in call-backs outside the usual update loop).
If the transforms can be changed by the render logic too, then the whole tree starting from any such altered parent needs to be updated again to ensure correct behaviour and display... but I can't think of a valid justification for render logic to adjust the transforms so perhaps it doesn't.

I haven't had a chance to look at your new commit pnstickne (I'm still in Christmas vacation mode) but I've been following this thread and believe that there's really only one or two genuinely robust solutions to this problem so I'm throwing this out there for discussion in case you haven't already covered this ground :)

@pnstickne
Copy link
Contributor

@pjbaron Yup. That sounds right and is the justification I am using of the latest / revised approach (which is a simplification) and the recent PR change does just that - every "logic update" run by the core game loop starts a worldTransform from the Stage, which iterates the scene graph recursively visiting all connected display objects.

I've outline the assumption in PR.


Previously I asserted / hinted that the WT might also be done before a render, but I've since revised that with the observation that, normally, only updates can "move" things. This assumption/rule simplifies the logic and allows skipping a WT under spiraling catch up situations (where there are no update logic calls).

There should probably be an exposed function to allow the user to "syncWorld" if user/game code moves objects outside of the update phase - which would be in response to a direct browser event or setTimeout, etc. But this is a special case, not the norm.

I know of a few fringe cases where this occurs in Phaser (ie. ScaleManager.onFullScreenError), but I believe that the input system - which is the main concern for such external events - moves all the relevant callbacks into the update phase. Plus, Phaser encourages the design of checking input state within the game's update logic.

It would be prudent to review the various internal callbacks / sources and document (and possibly modify) any such cases that do not operate under the update phase; also, documentation for the above stated rule should also be included somewhere relevant ..

@rendermouse
Copy link

I built the latest from DEV, and I am no longer seeing physics pass-through issues. :)

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

No branches or pull requests

6 participants