Add support for frictionGravity #402
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -40,7 +40,7 @@ | |||
| "@types/three": "^0.139.0", | |||
| "@typescript-eslint/eslint-plugin": "^5.17.0", | |||
| "@typescript-eslint/parser": "^5.17.0", | |||
| "cannon-es": "^0.19.0", | |||
| "cannon-es": "^0.20.0", | |||
There was a problem hiding this comment.
I noticed that cannon-es is listed as a devDependency but it reads to me like it should be a dependency so I'm wondering what I'm missing.
There was a problem hiding this comment.
cannon-es is bundled into the worker as part of our build process and does not need to be installed separately by the people using use-cannon.
chnicoloso
force-pushed
the
frictionGravity
branch
from
1c5e32f
to
806bf71
Compare
|
Hi @isaac-mason, can I borrow your eagle eyes for another small one? 😄 |
|
@chnicoloso out of curiosity, is the demo you shared open-source (and using react-xr)? Look interesting, would love to take a look!
|
| @@ -65,7 +73,7 @@ export class CannonWorkerAPI extends EventEmitter { | |||
| quaternions: Float32Array | |||
| } | |||
|
|
|||
| private config: Required<CannonWorkerProps> | |||
| private config: Required<Omit<CannonWorkerProps, 'frictionGravity'>> & { frictionGravity?: WorldProps['frictionGravity'] } | |||
There was a problem hiding this comment.
Please don't make an exception for this, we send over the config with default values specified.
There was a problem hiding this comment.
Open to ideas on expressing that this an optional value without adding this exception - I admit, it is pretty ugly!
There was a problem hiding this comment.
This is an internal interface, we don't have any optional properties here. If an acceptable value is undefined, that is okay.
| @@ -42,6 +42,14 @@ export class CannonWorkerAPI extends EventEmitter { | |||
| this.postMessage({ op: 'setGravity', props: value }) | |||
| } | |||
|
|
|||
| get frictionGravity(): Triplet | undefined { | |||
There was a problem hiding this comment.
None of our interface is optional, it should always return a Triplet.
There was a problem hiding this comment.
In this case, a possible value for frictionGravity is undefined.
https://pmndrs.github.io/cannon-es/docs/classes/World.html#frictionGravity
Gravity to use when approximating the friction max force (mu * mass * gravity). If undefined, global gravity will be used. Use to enable friction in a World with a null gravity vector (no gravity).
There was a problem hiding this comment.
Right, thanks Isaac! @bjornstar I could return an empty array here if always returning a value is important - but keeping the worker api signature consistent with the World interface seems preferable to me.
| get frictionGravity(): Triplet | undefined { | ||
| return this.config.frictionGravity; | ||
| } | ||
| set frictionGravity(value: Triplet | undefined) { |
There was a problem hiding this comment.
It's not an optional value here.
Also, could you please keep the ordering of the properties alphabetical, this should go above gravity, I'm not sure why our linter is not complaining about it.
There was a problem hiding this comment.
Could you clarify what you mean by it not being optional here? My intention here was to get across that the user may define frictionGravity, and they may also reset it back to undefined.
| @@ -75,6 +83,7 @@ export class CannonWorkerAPI extends EventEmitter { | |||
| broadphase = 'Naive', | |||
| defaultContactMaterial = { contactEquationStiffness: 1e6 }, | |||
| gravity = [0, -9.81, 0], | |||
| frictionGravity, | |||
There was a problem hiding this comment.
Please specify a default value and keep the ordering alphabetical.
There was a problem hiding this comment.
Aye aye on the ordering! On providing a default: the expected default on cannon-es for this property is undefined. Would you like me to assign frictionGravity = undefined here to at least make it explicit to readers? Would a little comment be welcomed?
There was a problem hiding this comment.
Yes, please assign undefined as the default if that's what it is.
|
@Glavin001 thanks for the interest! I am not using react-xr. I don't have anything up yet but I'll let you know when I do! |
|
This looks good, just some minor stylistic issues from my side and we should be good to go. |
Co-authored-by: Bjorn Stromberg <[email protected]>
Co-authored-by: Bjorn Stromberg <[email protected]>
Co-authored-by: Bjorn Stromberg <[email protected]>
…to frictionGravity
|
Thank you @bjornstar! Updated |
| @@ -65,7 +73,7 @@ export class CannonWorkerAPI extends EventEmitter { | |||
| quaternions: Float32Array | |||
| } | |||
|
|
|||
| private config: Required<CannonWorkerProps> | |||
| private config: Required<Omit<CannonWorkerProps, 'frictionGravity'>> & { frictionGravity?: WorldProps['frictionGravity'] } | |||
There was a problem hiding this comment.
This doesn't need to change.
| private config: Required<Omit<CannonWorkerProps, 'frictionGravity'>> & { frictionGravity?: WorldProps['frictionGravity'] } | |
| private config: Required<CannonWorkerProps> |
There was a problem hiding this comment.
Hm, I think it does! Otherwise, TS would yell about this config assignment: https://github.com/pmndrs/use-cannon/pull/402/files#diff-e3f4dac018aec1d1b23bbbb9ea7d1e0edbdc2dcfee7f35d088d8e145754add61R101, since config would be typed with frictionGravity as required.
Let me know what you think I'm missing!
|
I ended up using null internally instead of undefined, it made things much more straight-forward. |
Howdy - updating the worker api to enable setting the world's
frictionGravity. This is a new property that was just added tocannon-es(pmndrs/cannon-es#156) to address schteppe/cannon.js#224 and allow friction to be optionally applied to worlds without gravity.