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

replace some constants with configurable properties for raycast-vehicle #65

Merged

Conversation

tomo0613
Copy link

Greetings!

I'd like to use some constants from the RaycastVehicle class [especially fwdFactor] as configurable properties, in the WheelInfo class.
Tweaking that value makes it possible to achieve more realistic handling for a raycast-vehicle.

I really like the vehicle behaviour in Sketchbook, and I want to improve my project as well without using a custom cannon lib.
I also slightly modified the example, imho the original demo feels a bit weird when braking [press B] (but it could be reverted)

@tomo0613
Copy link
Author

tomo0613 commented Jan 13, 2021 via email

@swift502
Copy link

@tomo0613 yeah sorry I deleted that comment because I realised it's something different than just exposing parameters. Maybe that's for a different PR.

Your current changes should (I believe) definitely be merged though.

@tomo0613
Copy link
Author

tomo0613 commented Jan 13, 2021 via email

@marcofugaro marcofugaro self-requested a review January 13, 2021 22:56
@marcofugaro
Copy link
Member

@tomo0613 Nice! I agree they would make the vehicle configuration better. We should expose them.

I don't think we should expose the variables this way though. They come from bullet, and probably there is a reason for them to have that value.

Instead we should do it like this:

const x = wheel.forwardImpulse * fwdFactor / wheel.forwardAcceleration
const y = wheel.sideImpulse * sideFactor / wheel.sideAcceleration

forwardAcceleration and sideAcceleration should both default to 1. There is a division because that way, if the value is greater, the car goes faster in the forward direction. The same is for the sideAcceleration.

Could you update your PR with this?

I also slightly modified the example, imho the original demo feels a bit weird when braking [press B] (but it could be reverted)

True, it actually feels better 👍, keep the friction at 1.4. Don't change the forwardAcceleration and sideAcceleration though, leave them as default.

@marcofugaro
Copy link
Member

@swift502 what was your proposition?

The new version of Sketchbook looks nice btw! Did you notice we integrated one of your original PR to cannon in 2970232?

@tomo0613
Copy link
Author

I have updated the branch based on the suggestions.

@swift502
Copy link

swift502 commented Jan 14, 2021

@marcofugaro I proposed changing the default frictionSlip value in the WheelInfo class to something smaller (In Sketchbook I use 0.8, and I gather @tomo0613 uses 1.4?). A value of 10000 seems excessively unrealistic and I believe it's what makes a lot of vehicle demos online feel very unnatural. I don't really know why @schteppe made 10000 the default.

As for 2970232, yeah I noticed. 🙂 I planned on switching from cannon to cannon-es in Sketchbook, but never got around to it for some reason.

@marcofugaro
Copy link
Member

@swift502 probably an oversight, it is 10.5 in bullet by default.

https://github.com/bulletphysics/bullet3/blob/afa4fb54505fd071103b8e2e8793c38fd40f6fb6/src/BulletDynamics/Vehicle/btRaycastVehicle.h#L45

Thanks @tomo0613, will merge it and publish tonight

@swift502
Copy link

swift502 commented Jan 14, 2021

@marcofugaro Good idea to compare with Bullet. Even 10.5 would imo serve as a much better default value.

@marcofugaro marcofugaro merged commit d5683cd into pmndrs:master Jan 14, 2021
@tomo0613
Copy link
Author

I'm happy to see the changes in live, thank you all!

@tomo0613 tomo0613 deleted the raycast-vehicle-configurable-props branch January 14, 2021 23:58
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

Successfully merging this pull request may close these issues.

3 participants