-
Notifications
You must be signed in to change notification settings - Fork 561
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
Refactor - Gravity, Twoway, DeltaTime based Movement, Attach to moving platforms, Abstract intersect, Crafty.defineField #708
Conversation
Does anyone know why we are using a seperate method for detecting ground collisision in the |
Motion component will be redone to have standard linearVelocity, angularVelocity, etc. methods. This way I can also include #649 |
Just a quick couple of notes until I have time to look at it later:
I think that's the main reason -- it might also be a little more performant, since it searches a smaller total area for collisions. Knowing if two components are "touching" each other is probably common enough that we might want to abstract it. |
Thank you for your input!
EDIT |
I had no success implementing |
Right, but at each timestep you first increase x using that equation, and then increase v. It can make a noticeable difference at low v. It's also exactly correct (well, up to rounding issues) for constant acceleration. Agreed we don't want to use anything more complicated than that, though. I still haven't looked in depth, but I don't think the math.js stuff is necessarily optimized very well. I'd avoid using it as much as possible. You shouldn't treat I'd not worry about handling active rotation right now. It's basically orthogonal to linear motion, and a lot of games wouldn't care about it in the first place. |
So you propose this About the Vectors:
|
I think these are enough modifications for a single PR :) Check out the JSFiddle how the stuff works. I think this PR is almost ready, what do you think? Be sure to read the updated, original (first) post again, I wrote a concise description of anything relevant in there |
Javascript at its best -.- |
I still have a bunch of concerns with this, to be honest. I'll leave them as separate comments.
In a fundamental way, a vector is defined by how it transforms under rotation. Using rotation as a third component of a 3D vector is a horrible, horrible abuse of the concept -- the only reason to do so would be if it offered a performance improvement, which doesn't seem to be the case here. :) As your perf result indicates, we're probably better off not using them at all. The additional level of indirection (needing to access There's also an overhead that comes by worrying about the rotation in the first place -- it places a cost on entities that are moving around but not rotating, which would actually be pretty common. Better to have them handled by separate components. |
I'm not sure, but it doesn't matter -- they produce different results. In the case of constant acceleration, using the exact equations will be limited only by floating point errors, while the second will introduce an additional set of errors, systematically overestimating the distance traveled each time step by an amount of |
if(isNaN(this._jumpSpeed)) this._jumpSpeed = 0; //set to 0 if Twoway component is not present | ||
|
||
this.bind("EnterFrame", this._enterFrame); | ||
startGroundDetection: function(ground, canLandFn) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this doesn't just hook into the landed/lifted events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "GroundDetector" is a passive component, that does not do anything until Gravity calls startGroundDetection / stopGroundDetection. This also means that it is backward compatible, because the user does not know anything about "GroundDetector"; he uses "Gravity" just like before.
Suggest me a nice & concise way to set & get 6 properties without cluttering the API :) Think vector as list, not vector in mathematical sense :)
You are right. I will adapt the code accordingly. |
Well, this is where we developers have to suffer so the users have better performance. :) Internally, the component should just use top level properties like Totally apart from that, I really do think that the case of rotation should be handled by another component. Imagine the stereotypical platformer: In Mario or Braid you bounce up and down and run left and right, but there's never any rotation. If the Motion component has to worry about rotation, that adds a cost that the user might not want/need to pay. |
I have updated the motion component in order to combine the best of all worlds:
Is there something else that should be changed? I am fairly confident about the maturity of this PR. EDIT: |
Splitting angular from linear motion just incured one more "EnterFrame" event. |
So, I think this is something that probably needs lots of testing and design, but maybe the best way forward is just to merge it into develop and play with it. :) I have a pretty similar system I used in a platformer game; once this is merged I can try to implement the additional capabilities on top of this system and see how that works. (Some things I found particularly tricky to get right: frictional forces, midair collisions.) Could you squash the commits down a bit? Certainly all the "fixes" and "add test" ones. :) When doing an interactive rebase you can reorder commits if it makes sense to, squashing two together even if there's an unrelated commit between them. (This actually has a practical impact when using |
EDIT: ah ok rebased them somehow, I hope it's good now |
@starwed does this PR look good to you? |
@mucaho Honestly I haven't been able to take the time to go through it carefully -- there a lot of changes lumped together here. I think it would take building a small demo game on top of them for me to be sure i grokked everything. Looking over it again, is the 3D vector stuff even used in the rest of the patch now? Could it be split out into a separate PR? |
I managed to split all commits with surgical precision :) :
The other commits (which are not related to this PR) were split into seperate PRs
|
Thanks, that's much easier to read through now! I still need to build something with it, but here's a couple of technical notes:
|
The performance of a function call is horrible on Chrome compared to hard-coding it. There is no difference on FF. Should I revert it to like it was before? |
Don't revert it just yet -- what matters most is if it takes up a measurable fraction of the total game loop time. Chrome and Firefox can report exactly how much time is spent in a given function, so that'll give us a better "real world" indication of whether it makes a difference. (And certainly using a function is way more readable.) (But we do that comparison often enough that it might actually matter.) |
So, how would we go about measuring the fraction of the total game loop time it takes? Or do we just "assume" it is irrelevant for now? |
Basically, run a few Crafty games with that change, and use Chrome/Firefox's dev tools to measure the time spent in that particular function. I have a couple simple "benchmark" apps that just bounce a bunch of blocks around, with and without collision detection. In Firefox it's the "Profiler" option -- you run it while the game is running, and it reports a lot of detailed information from the js engine. Here's an example -- we can see that while the manager was running, 10% of the time was spent inside |
Thanks for your input! I agree with your suggestions.
That's why
Interesting and great approach. Each code block can decide for itself if entity can land -> decouples code! |
Aha, missed that somehow. There'd be a crazy corner case still where, if you hit a platform at the very top of your arc, you could still get teleported above it. (Since vy is altered after position.) But using the smaller hitbox fixes that anyway. A meta point: it might be worthwhile to split the gravity/ground detector patch off from the linear/angular motion patch? Then we could focus on landing the latter. (Up to you!) |
That is the same behaviour as before and is actually the desired behaviour in my opinion. (Keep in mind the hibox is shifted +1 px in y direction, so top pixel of the player will not be considered for collision detection). pawurb's quote from #592:
I have had bad experiences when I tried to alter the hitbox's height to 1px. The ground collision was detected sometimes and sometimes not. We can always change that later after we have tested that properly. |
|
Btw you can always submit a PR against my branch. I have to admit that rewriting the same code over and over again is very boring :) |
Ok, I implemented the suggestions.
|
c00210e
to
0f8d861
Compare
Ok I rebased this PR onto the new changes from develop branch. The only remaining "issue" is that users should set the Other than that, I hope this bad boy is ready for deployment, what do you think? |
Cool, I'll try to take a look at this again tomorrow. (It's been a while since I've reviewed the code!) I think that even if there turn out to be issues, we should go ahead and land it soon, just so people benefit from all the work you've done! Then we can fix odd corner cases or revamp APIs as necessary. |
0f8d861
to
a6f9d5d
Compare
Nice! In the meantime I added a Also changed |
* @param rectB - An object that must have the `_x, _y, _w, _h` values as properties | ||
* @return true if the rectangles intersect; false otherwise | ||
*/ | ||
intersect: function (rectA, rectB) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there actually a reason for this right now? (i.e., is there any reason to have both this and rectManager.overlap?)
Ok, looks basically fine to me. Once it lands we can get a better feel for how the various new components will play in different custom scenarios, and maybe modify them down the road based on that feedback. (I'll probably try to write a tiny platformer as a test.) I left a couple of comments; I really would prefer everything related to be attached to |
Thanks for taking a look at it!
Sounds good, will adapt.
Yep, I also thought about it, I will try to assign the function to a local variable where applicable. That should give us some improvement in loops. If overhead of function call will be too large after profiling, we can revert.
I actually have a small demo and will upload it to the |
a6f9d5d
to
0018e7a
Compare
Added suggested changes and merged manually into |
The demo is in the |
Current state
next PR will have:
Motivation
In my opinion, the sharing and modification of 3-4 properties between Twoway and Gravity was hard to read & maintain and did not allow for easy extendability. Future components that would like to use velocities & acceleration just have to
require("Motion")
and apply the motion through it's methods.Changes
I introduced a new component called GroundDetector which does what Gravity did before: It registers if the entity lands on the ground for the first time or if the entity lifts off from the ground for the first time. GroundDetector also snaps the entity to the ground upon landing.
Gravity manages start/stop of GroundDetector and specifies the function which GroundDetector calls to check if the entity is allowed to land (e.g. currently entity must be falling to be able to land). The functionality was split from Gravity so that the to be implemented component GroundAttacher (#601) can also listen for these events.
The next component is called Motion. Both Gravity and Twoway use that component to add acceleration (gravity constant) or to add velocity (jump speed) in the y direction. "Motion" has a method
linearDelta()
which returns the relative change that was introduced by all linear motion. The relative change in the y direction is used in order to determine if the entity is falling or rising (jumping) - this function is called in GroundDetector each time the entity wants to land. "Motion" also allows deltaTime based movement of entities as described in #649.A recent forum post indicates that users want to configure the ability to jump/land to their preferences. I added two methods
Crafty.e("GroundDetector").canLand()
andCrafty.e("Twoway").canJump()
. Users can override the default behaviour of these two methods to their liking.The common intersect check (that was being manually duplicated all over the library) was extracted into the global method
Crafty.intersect(rectA, rectB)
.Crafty.e("2D").intersect()
was streamlined to operate on_x, _y, _w, _h
properties rather thanx, y, w, h
- this is the default for similar methods (isAt
,contains
, ...). Is there a specific reason it deviated from the rest?Crafty.defineField
andCrafty.e.defineField
were added.Crafty.e.setter
was adapted to use these two methods. They add a get callback and disable configuration (= deletion & reconfiguration) of the property.Backwards Compatibility
Crafty.e("2D").intersect()
was changed to operate on _x, _y, _w, _h. User code may break if it called intersect.Any input would be greatly appreciated! I'm still vague about the component names. Do you have any ideas?