schteppe / p2.js

JavaScript 2D physics library
Other
2.64k stars 330 forks source link

Is p2 abandoned? #307

Closed ghost closed 6 years ago

ghost commented 7 years ago

Is p2 abandoned?

I've found some functions like runNarrowphase to be global instead in World class scope. It looks like mess and it's the master branch. Why?

schteppe commented 7 years ago

p2.js is an open source hobby project. Nobody is paid to work on it. I would not say it's abandoned, just less active at the moment... Is runNarrowphase global? Where did you find this?

photonstorm commented 7 years ago

👍 for all the work Stefan does on p2

runNarrowphase definitely isn't in the global scope. Isn't even hanging off the local (p2) scope actually.

ghost commented 7 years ago

@schteppe here u can find runNarrowphase as global function:

https://github.com/schteppe/p2.js/blob/master/src/world/World.js

it is the master branch, so I guess the latest "working-on" version.

jtenner commented 7 years ago

@Kamil93 that's because it's used in a commonjs environment. It's wrapped up into it's own context. Not global at all. When you run the library, the function is not leaked to the global context.

ghost commented 7 years ago

@jtenner: ya, but it's ugly as hell.

photonstorm commented 7 years ago

And that opinion is subjective as hell.

ghost commented 7 years ago

@photonstorm: no it isn't, when u mixed structural programming with oop, you made it wrong. Check out the Internet.

jtenner commented 7 years ago

@Kamil93 I don't think this is a place to argue how people create their personal projects, that they let you use for free. Thanks for your time!

ghost commented 7 years ago

@jtenner: You've misunderstood the place you are posting. As long as project is public and for public use and as long as issue is constructive, this is the place where you post it.

Besides of it that runNarrowphase is out of World class prototype scope - is definetelly wrong design. It prevents users from overriding it.

It's not critical issue but It's still issue. And as u can see, we are in issue tab :d

photonstorm commented 7 years ago

The issue isn't what you've posted about, it's the passive aggressive and derisory tone of it. It would have taken zero effort to ask it in a better manner:

"I really need to be able to override the runNarrowPhase function, but it appears to be inlined in World.js and outside of its class scope. Would you be willing to accept a PR to change this? Or is this project abandoned, as I haven't seen much activity here for a while and don't want to spend time on it if the project is over."

ghost commented 7 years ago

I dont need override runNarrowphase. I've forked project and made it in right place. If u think my posts are passive agressive may it be because ur offtopic that was unnecessary or my english that is not so well. Check the begin of issue. I've noticed design-bug so reported it. Thats all. Sorry if it hurts you. It wasn't my plan.

schteppe commented 7 years ago

I've quickly looked through the fork diff. Some of the work you've done will precisely undo optimizations I've done before. For example, the event objects. The reasons why I moved them from the World instances to the scope are:

An advantage of having the event objects on the world instances is that it will fix the following bug, when you have multiple Worlds:

worldA.on('beginContact', function(evt){
    // evt refers to things in worldA
    worldB.step();
    // evt now refers to things in worldB
});

Regarding the runNarrowphase function... I removed it from World.prototype in this commit, because of reasons similar to those above.

ghost commented 7 years ago

@schteppe: is it so necessary? I guess for events it's less important, but around 3 functions per class doesn't really do a difference in minifing. Maybe there are better minifing algorithms around.

Anyway backing to main subject. I would like to do something for your project, if u have some job to be done, expect physic-math, please type it here. I will do my best and then you can check the diff and accept or reject it.

schteppe commented 7 years ago

@Kamil93: I'd say those micro-optimizations is not necessary; there are other things that could make the build much smaller. For example, using modern ES6 and code bundlers such as rollup, or splitting out code into optional modules. (One should really move the Body#fromPolygon method into an own module, or perhaps a demo, as it pulls in the whole poly-decomp library by itself).

Have a look through the issues list to see if you can find a task that you find fun :)

ghost commented 7 years ago

@schteppe: I think it isn't big deal so I'm gonna ask here:

Is it necessary that body.integrate & body.setZeroForce are in two separated for loops?

https://github.com/schteppe/p2.js/blob/6d01338f6773b383db58d4d2fdf481dd6a6c18c5/src/world/World.js#L770

schteppe commented 7 years ago

@Kamil93 that is a very microscopic micro optimization... I'd not bother about it. If you want to make those loops more efficient, maybe you could refactor it so it doesn't have to check body.type all the time? And only reset force on dynamic bodies? Something like:

    // Step forward
    for(var i=0; i!==dynamicOrKinematicBodies.length; i++){
        var body = dynamicOrKinematicBodies[i];
        body.integrate(dt);
    }

    // Reset force
    for(var i=0; i!==dynamicBodies.length; i++){
        dynamicBodies[i].setZeroForce();
    }
cedric-h commented 6 years ago

Can we close this issue? The answer is evidently no.