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

Move worker logic into react agnostic 'cannon-worker' package #327

Open
isaac-mason opened this issue Jan 12, 2022 · 28 comments
Open

Move worker logic into react agnostic 'cannon-worker' package #327

isaac-mason opened this issue Jan 12, 2022 · 28 comments

Comments

@isaac-mason
Copy link
Member

I recall seeing a discussion/issue about separating the worker logic in this package into a cannon-worker package that isn't tied to react. I can't find that discussion anymore, but I recall reading that the maintainers only really used cannon with react-three-fiber, so it wasn't deemed important.

I have a use case where it would be great to have a cannon worker package, but I'm not looking to use react.

I'm hoping to reopen the discussion and see if there is any interest for contributions creating this standalone cannon-worker package to power this package.

I have been working on a POC for taking logic out of this package and creating that react-agnostic cannon-worker package, and I'm looking to see if there is any interest in that being developed under the pmndrs roof.

I have a lot of interest in this and time for contributing, and putting this under the pmndrs roof could de-duplicate some of the efforts I'm making with the efforts here.

@drcmda
Copy link
Member

drcmda commented Jan 12, 2022

why not, the only thing is, a worker cannot be published through npm because workers are accessed by url, and you can't fetch from node_modules because it doesn't exist in production. in this project it's being bundled into a url blob to work around that issue.

if the worker were a separate lib i guess that could still be done but i wonder how anyone else would use it? or maybe it's getting bundled like that as well with a small interface to communicate with it.

@isaac-mason
Copy link
Member Author

Great! 🙂

I think bundling the worker in the separate cannon-worker package with an interface to communicate with it could be a good way to go?

Almost all of the functionality here could be moved into cannon-worker. This package could then use it in hooks & components, essentially becoming a simple wrapper. Just spitballing of course! These are very significant changes.


For context, here is the POC I am working on now: https://gitlab.com/rapidajs/rapida/-/tree/develop/packages/cannon-worker

Note that this is the WIP result of a very messy rework of this repo - it wasn't built from the ground up. Some things straight up don't work yet. But it's just a POC - I'd want to either make some more significant improvements to what is there now or just start over.

I tried not to touch the api too much when doing the rework. It's usage right now looks like this:

import { CannonPhysics, BodyType } from '@rapidajs/cannon-worker';

// ... other three imports ...

// ... create three renderer, scene, camera, etc ...

// create physics world
const physics = new CannonPhysics({
  gravity: [0, -10, 0],
  maxSubSteps: 5,
  delta: 1 / 60,
  // ... other world params ...
});

// create a three box mesh
const mesh = new Mesh(new BoxGeometry(1, 1, 1), new MeshBasicMaterial());

// create a box - more or less the same API as in here
const { api } = physics.create.box({
  mass: 1,
  args: [1, 1, 1],
}, mesh);

// step the physics world, providing the time elapsed
physics.step(1 / 60);

The Provider component we have now could then instantiate a new physics world via cannon-worker, and then hooks could use cannon-worker to create bodies, constraints, etc.

e.g. Provider could become something like this

export default function Provider({
  /* ... props ... */
}: ProviderProps): JSX.Element {
  const [physics] = useState<CannonPhysics>(() => new CannonPhysics({ /* ... props ... */ }));

  useFrame((state, delta) => {
    physics.step(delta);
  });

  // all the other stuff :)
  // ...
}

Let me know what you think of this as a potential approach.

@drcmda
Copy link
Member

drcmda commented Jan 12, 2022

sure, so i can create a cannon-worker repo on here and add you. i'd suggest better start from scratch so that we don't loose functionality by accident and i'd also suggest doing both use-cannon and cannon-worker at the same time for easier profiling and testing. is that good?

@isaac-mason
Copy link
Member Author

isaac-mason commented Jan 12, 2022

That would be great. I'm happy to start from scratch and incrementally build things up, sounds good.

I'll put both cannon-worker and the use-cannon package in the one repo with yarn workspaces, the same as I've seen in other pmndrs repos.

This might be a good opportunity make some additions such as exposing shape-specific APIs, as I saw discussed in #35. I will keep the current usage backwards compatible though. I'll aim to use the current use-cannon demos without any alteration, just a different import in the new temp repo for my rewrite of use-cannon. Doing so would make profiling and comparing the two easier as well.

One other thing - are you happy for me to just push my incremental work to that repo? Looking at the fantastic work joergjaeckel has been doing with pmndrs/use-p2, it looks like you folks are happy with that workflow in the infancy of a project. I'm happy with whatever though.

@drcmda
Copy link
Member

drcmda commented Jan 12, 2022

alright, let's try. i've added you into the pmndrs/physics team

@isaac-mason
Copy link
Member Author

Awesome, I'll reach out to the others in there shortly. Keen to get this done! 🙂

@isaac-mason
Copy link
Member Author

I'm going to make a start on this tomorrow.

I've created the cannon-worker repo, and I'll add a readme shortly with some context and my planned roadmap for building this out - will share once it's there. Keen to hear any thoughts on what is included there & any other things people would like to see.

@isaac-mason
Copy link
Member Author

Making progress on this over at https://github.com/pmndrs/cannon-worker

As I reintroduce features & hooks, I'll be adding examples direct from the use-cannon repo to help regression test. Right now I've just got the Cube Heap and the Sphere Debug examples - will be able to add many more once I re-add hooks for constraints.

I'd love to get some thoughts from the current contributors of this repo on what's being done over there, especially thoughts on the split of logic between cannon-worker and use-cannon.

Just picking some frequently appearing names on this repos commit history, @bjornstar, @Glavin001, @marcofugaro it would be good to get some thoughts from you folk at some point! (and any anyone else!)

@bjornstar
Copy link
Member

It looks like a hard fork of the project, I'm not sure what we're supposed to say. I feel that we don't need to throw away the work that people have done on this codebase by copy/pasting it piecemeal into a new project. This could easily be done within the context of this project by adding an API layer between the worker and react portions.

Here's a quick attempt, totally needs more work, but you can see how it could go -- https://github.com/bjornstar/use-cannon/tree/cannon-worker-api

@isaac-mason
Copy link
Member Author

isaac-mason commented Jan 17, 2022

That's a very fair comment @bjornstar.

I do understand that moving code into a new project like this totally comes across as undermining the efforts of this repos contributors, and that's definitely not my intent! Just a not-so-great looking side effect of my enthusiasm. My bad haha

My motivation for doing this is to create an easy way of running cannon in a web worker that isn't only usable in the r3f ecosystem. In my mind, it made sense for there to be a first-class standalone cannon-worker npm package for this. It also seemed like it would be worthwhile recreating that from the ground up to make sure that the non-react API made sense, make sure nothing breaks along the way, add types everywhere, etc.

I'd be happy to give it a go contributing along the lines of your example if you're happier with that approach and you think it's valuable. That API layer could be built-in use-cannon, then maybe at some point once the layer is fully built, an independently usable cannon-worker package could be split out?

Another option could just be for a cannon-worker package to go off and exist on its own without being tied to changing how use-cannon works, but to me it seems worthwhile de-duplicating the two.

@marcofugaro
Copy link
Member

marcofugaro commented Jan 17, 2022

My motivation for doing this is to create an easy way of running cannon in a web worker that isn't only usable in the r3f ecosystem.

Hey Isaac, I approve the idea. In the vanilla.js world, I've seen people writing the cannon code in a worker and the three.js one in the main thread (just like worker ping-pong example or the SharedArrayBuffer example), but this doesn't work well if the user wants to split up the project in multiple files.

But if you want to make an API that handles this syncronization behind the scenes, I'd suggest you make the public API as similar to the cannon-es one as possible, since that's what users are familiar to.

Something like this:

import { World, Box } from 'cannon-worker';

// other three imports ...
// create three renderer, scene, camera, etc ...

// create physics world
// this spawns a worker and creates a world in it
const world = new World({
  gravity: [0, -10, 0],
  // other world params ...
});

// create a three box mesh
const mesh = new Mesh(new BoxGeometry(1, 1, 1), new MeshBasicMaterial());

// create a box
// the returned object is the "api" object from use-cannon
const box = new Box({
  mass: 1,
  shape: [1, 1, 1],
});
world.add(box); // .add() sends a message to the worker

box.sync(mesh) // keeps the position and quaternion of the mesh in sync

// step the physics world
world.step(1 / 60); // this sends a message to the worker as well

or if you want to make things 1:1, consider mocking also the shapes classes

const box = new Body({
  mass: 1,
  shape: new Box(1, 1, 1),
});

@marcofugaro
Copy link
Member

marcofugaro commented Jan 17, 2022

Regarding the use-cannon/cannon-worker thing, I assume you are keeping the use-cannon examples just to test that everything works in the short-term.

In the long-term, it doesn't make sense to duplicate the same examples over there, which is what I think @bjornstar was pointing to. There shouldn't be any react in that repo.

@bjornstar
Copy link
Member

I'd be happy to give it a go contributing along the lines of your example if you're happier with that approach and you think it's valuable. That API layer could be built-in use-cannon, then maybe at some point once the layer is fully built, an independently usable cannon-worker package could be split out?

I'd be happy to help with that approach, I was hoping that people would see the use-cannon repo as active and responsive to PRs and would try to contribute here.

@isaac-mason
Copy link
Member Author

isaac-mason commented Jan 17, 2022

Thanks for the responses, all!

Those examples were just being added to cannon-worker short-term to test, yep. I wasn't looking to steal and re-home them 🙂


Re: building out cannon-worker API layer in use-cannon

@bjornstar sounds like the start of a plan then!

I'll halt my cowboy work over at cannon-worker now and shift my focus to helping do this in use-cannon 🙂

How can I best help with pushing this? Are you happy for me to work on a PR for use-cannon that sets up the start of that API layer, following your example? Or would you be more comfortable creating that base and having me work off that? Happy with either!


Re: the cannon-worker public api

@marcofugaro mirroring the public API for cannon-es is a good call! Makes sense. I also like the idea of mocking the shape classes to make it closer to the cannon-es API.

Maybe it could look something like this?

// we could only provide `shape` if the body is not a compound shape
const box = new Body({
  mass: 1,
  shape: new Box({
    args: [1, 1, 1],
    position: [1, 0, 0],
  }),
});

// ---

// we could also support calling `addShape(s: Shape)` to create compound bodies?
const box = new Body({
  mass: 1,
});
box.addShape(new Box({
  args: [1, 1, 1],
  position: [1, 0, 0],
}));
box.addShape(new Sphere({
  args: 1,
  position: [-1, 0, 0],
}));

// ---

// or provide `shapes` in the constructor for compound bodies 
const box = new Body({
  mass: 1,
  shapes: [
    new Box({
      args: [1, 1, 1],
      position: [1, 0, 0],
    }),
    new Sphere({
      args: 1,,
      position: [-1, 0, 0],
    }),
  ],
});

Another question is how instancing would look. Maybe something like this?

// If `Body.sync` is called with an InstancedMesh before adding to the world, instance the body.
// Offer two kinds of syntax - one being just providing the props as an object, an other being providing 
// the props as a GetPropsByIndex function, as in `use-cannon`
const boxWithoutFnSyntax = new Body({
  mass: 1,
  shape: new Box({ args: [1, 1, 1] }),
});
boxWithoutFnSyntax.sync(someThreeInstancedMesh);

// ---

const boxWithFnSyntax = new Body((index?) => ({
  mass: 1,
  shape: new Box({ args: [1, 1, 1] }),,
}));
boxWithFnSyntax.sync(someThreeInstancedMesh);

@marcofugaro
Copy link
Member

marcofugaro commented Jan 18, 2022

Maybe it could look something like this?

I would avoid using the args keyword since it's a r3f thing, little did you know that cannon-es body already has support for a shape argument, so the simple body would look the same:

const box = new Body({
  mass: 1,
  position: [1, 0, 0],
  shape: new Box(1, 1, 1),
});

If you want to add a shape and a position relative to the body center of mass, the position goes actually as a second argument, so it would look something like this:

box.addShape(new Box(1, 1, 1), [1, 0, 0]);

but yes, as an enhancement it could be accepted in the constructor as well if we want, since the body already has those properties:

const box = new Body({
  mass: 1,
  shapes: [
    new Box(1, 1, 1),
    new Sphere(1),
  ],
  shapeOffsets: [
    [1, 0, 0],
    [-1, 0, 0],
  ],
});

Regarding instancing, I believe you still have to tell how many bodies to create (the Body constructor should handle sending a creation message to the worker, the sync just subscribes the mesh pos/quat to the body).

So it could be simplified like this:

// loop, you have to create a body per instance
for (let i = 0; i < count; i++) {
  const box = new Body({
    mass: 1,
    shape: new Box(1, 1, 1),
  });
  box.syncInstanced(instancedMesh, i);
  world.add(box);
}

where syncInstanced would invoke mesh.setMatrixAt instead of setting position/quaternion, example here

@isaac-mason
Copy link
Member Author

isaac-mason commented Jan 18, 2022

Nice!

Avoiding args makes sense, sure. And that syncInstanced syntax example makes the expected behaviour much clearer to users as well.

Thanks, @marcofugaro!

@isaac-mason
Copy link
Member Author

I'll get working shortly on making a PR, following @bjornstar's example, for a World worker API. I'll try and keep the change small, probably best to raise PRs for this in bite-sized chunks.

Once we've got that, as well as an accepted example of how we'd write one category of APIs e.g. bodies and shapes, I imagine it will be relatively quick work to build out the rest 🙂

@bjornstar
Copy link
Member

I'll get working shortly on making a PR, following @bjornstar's example, for a World worker API. I'll try and keep the change small, probably best to raise PRs for this in bite-sized chunks.

Once we've got that, as well as an accepted example of how we'd write one category of APIs e.g. bodies and shapes, I imagine it will be relatively quick work to build out the rest 🙂

Looking forward to your PR. I'll try to be responsive and you can always ping me on discord if you want some realtime feedback.

@isaac-mason
Copy link
Member Author

Awesome, thanks!

@isaac-mason
Copy link
Member Author

isaac-mason commented Jan 21, 2022

Hey @bjornstar (and others), I think in my earlier stab at this in pmndrs/cannon-worker I was structuring things wrong. I was essentially trying to build a full vanilla js cannon worker api that people could directly use, then make a version of use-cannon that was just a wrapper around that. This made some parts of use-cannon a little messy around three object refs.

I've rethought this, and I'm just dumping my current plan below. It might just seem obvious with your example in mind, but I'm just looking to make sure I'm on the same page anyway!


This is my planned path forward:

1. Refactor use-cannon to wrap up all interactions with the web worker in an api layer

This API layer should just wrap interactions with the web worker, and wouldn't be meant for direct use by users.

e.g. instead of this

this.worker.postMessage({ op: 'setGravity', props: value })

we would use the api layer

// `op` becomes the method name on this worker API wrapper
this.worker.setGravity(value)

and that API simply just calls worker.postMessage

This way, use-cannon still defines it's own API e.g. APIs returned by hooks

2. Create a separate package cannon-web-worker (name?) to house the newly created web worker api layer

Once we've refactored here, we can move this into it's own package so it can be reused.

3. Create a cannon-worker package that uses cannon-web-worker to provide a familiar API for those using cannon-es

The API for vanilla js users we've discussed above can then be built out using the same web worker logic behind the scenes.

Let me know if you have any concerns. Hoping to get time this weekend for no.1 🙂


EDIT: See comment for latest plan: #327 (comment)

@bjornstar
Copy link
Member

I'd love to help getting this moving, do you think you could raise a PR with what you've got so far?

@isaac-mason
Copy link
Member Author

isaac-mason commented Feb 12, 2022

Hey @bjornstar, yep sure! Will do shortly.

Edit: Done!

@isaac-mason
Copy link
Member Author

My PR for an API layer between hooks and the web worker has been marked as ready for review - no longer a draft: #343

If we're good with this, I think the next step is to start work on separating the web worker into it's own package.

Are we happy to house all packages in the use-cannon repo? (web worker, react package, non react package)
It might make working on this easier, avoiding the need to make changes across repos.

@bjornstar let me know what you think!

@bjornstar
Copy link
Member

The name use-cannon implies react, we can start creating new packages here and figure out how to name them.

@joergjaeckel
Copy link

We could also align the repository names with npm organization namespace. react-three-cannon and -p2 gives a clear hint on what the repository contains.

@bjornstar
Copy link
Member

We could also align the repository names with npm organization namespace. react-three-cannon and -p2 gives a clear hint on what the repository contains.

That's a good idea.

@isaac-mason
Copy link
Member Author

isaac-mason commented Mar 6, 2022

Discussed our path forward with bjornstar:

Our next step is to move the worker API as it is now into @pmndrs/cannon-worker-api, and have @react-three/cannon use that.

Then we'll look at giving @pmndrs/cannon-worker-api a more ergonomic cannon-es like API for people to use in non-react three.js projects.

There is still some glue to work out for marrying three objects to physics objects, but at this stage, there won't be a third @pmndrs/cannon-worker package for the cannon-es like API.

We'll be working towards a v5 release that will include the worker API as a separate package.

@isaac-mason
Copy link
Member Author

isaac-mason commented Mar 14, 2022

Now that @pmndrs/cannon-worker-api has been released (🎉 thanks for the help @bjornstar) I'm keen to look at making @pmndrs/cannon-worker-api more ergonomic and dev-friendly.

As a first step, I'll create a PR for renaming the body props args property to shape just in @pmndrs/cannon-worker-api, removing that r3f pattern from the non-react package.

Beyond that, I'll discuss with some people what a good end-state API would look like for @pmndrs/cannon-worker-api and will help to get us there.

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

5 participants