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

Update PinJoint2D API with angle limits and motor speed #81610

Conversation

Ughuuu
Copy link
Contributor

@Ughuuu Ughuuu commented Sep 13, 2023

servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
scene/2d/joint_2d.cpp Show resolved Hide resolved
scene/2d/joint_2d.cpp Show resolved Hide resolved
scene/2d/joint_2d.cpp Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
scene/2d/joint_2d.cpp Outdated Show resolved Hide resolved
scene/2d/joint_2d.cpp Outdated Show resolved Hide resolved
scene/2d/joint_2d.cpp Outdated Show resolved Hide resolved
scene/2d/joint_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 13, 2023

Took the motor part from here: https://github.com/slembcke/Chipmunk2D/blob/master/src/cpSimpleMotor.c
The joint limits part from here: https://github.com/slembcke/Chipmunk2D/blob/master/src/cpRotaryLimitJoint.c#L25

As I understood, the whole physics api now uses parts from chipmunk physics engine, which seems to be right.

Still need to write impl for calculating initial angle between objects. Right now the limits are for angle difference, but it works.

Screen.Recording.2023-09-13.at.19.55.13.mov

Also, one small issue is the angular limits seem to be kinda soft, not 100% sure why.

Let me know what you think.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 13, 2023

We should add a reference to COPYRIGHT.TXT to cover that since it was forgotten when that file was added, something like:

Files: ./servers/physics_2d/godot_joints_2d.cpp
Comment: Chipmunk2D Joint Constraints
Copyright: Copyright (c) 2007 Scott Lembcke
License: MIT

And add it just before the line Files: ./servers/physics_3d/collision_solver_3d_sat.cpp with one line of space between the entries

servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
@Ughuuu Ughuuu force-pushed the add-angle-limits-and-motor-to-pin-joint-2d branch from 2d6a66e to 9877f31 Compare September 13, 2023 21:12
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 13, 2023

Fixed the above issue, implemented feedback, review again and it's ready for merging I think, from my side at least. Works also same with implementation in box2d, well, almost (most notable difference is in box2d limits are harder, while with this implementation it seems limits are a bit softer).

@Ughuuu Ughuuu marked this pull request as ready for review September 13, 2023 21:15
@Ughuuu Ughuuu requested review from a team as code owners September 13, 2023 21:15
@Ughuuu Ughuuu force-pushed the add-angle-limits-and-motor-to-pin-joint-2d branch 2 times, most recently from 888e40e to 21fb91a Compare September 14, 2023 12:03
doc/classes/PhysicsServer2D.xml Outdated Show resolved Hide resolved
doc/classes/PinJoint2D.xml Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
COPYRIGHT.txt Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
servers/physics_2d/godot_joints_2d.cpp Outdated Show resolved Hide resolved
@Ughuuu Ughuuu force-pushed the add-angle-limits-and-motor-to-pin-joint-2d branch from 21fb91a to 0ded9c0 Compare September 14, 2023 13:33
@rburing
Copy link
Member

rburing commented Sep 16, 2023

Node properties are more commonly called *_enabled than enable_*, which I think make sense because enable_* sounds more like a function. So for the PinJoint2D node I propose having a motor_enabled property with setter set_motor_enabled and getter is_motor_enabled, and an angular_limit_enabled property with setter set_angular_limit_enabled and getter is_angular_limit_enabled. I also propose renaming the motor/target_velocity property to motor_target_velocity, and similarly replacing angular_limit/lower by angular_limit_lower and angular_limit/upper by angular_limit_upper. This makes the properties easier to write in scripts, and we can still make two groups in the inspector using the common prefixes, similar to how it is done in RigidBody2D here:

ADD_GROUP("Angular", "angular_");
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "angular_velocity", PROPERTY_HINT_NONE, U"radians,suffix:\u00B0/s"), "set_angular_velocity", "get_angular_velocity");
ADD_PROPERTY(PropertyInfo(Variant::INT, "angular_damp_mode", PROPERTY_HINT_ENUM, "Combine,Replace"), "set_angular_damp_mode", "get_angular_damp_mode");
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "angular_damp", PROPERTY_HINT_RANGE, "-1,100,0.001,or_greater"), "set_angular_damp", "get_angular_damp");

@Ughuuu Ughuuu force-pushed the add-angle-limits-and-motor-to-pin-joint-2d branch from 0ded9c0 to c55bef9 Compare September 16, 2023 09:20
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 16, 2023

Thanks for the idea, very good naming scheme. I like it better. Updated. Also, angular_limit sounds a little bit weird. Should it be angular_limits?

doc/classes/PinJoint2D.xml Outdated Show resolved Hide resolved
doc/classes/PhysicsServer2D.xml Outdated Show resolved Hide resolved
doc/classes/PhysicsServer2D.xml Outdated Show resolved Hide resolved
doc/classes/PhysicsServer2D.xml Outdated Show resolved Hide resolved
doc/classes/PhysicsServer2D.xml Outdated Show resolved Hide resolved
doc/classes/PinJoint2D.xml Outdated Show resolved Hide resolved
@Ughuuu Ughuuu force-pushed the add-angle-limits-and-motor-to-pin-joint-2d branch from c55bef9 to 39baabd Compare September 16, 2023 10:05
@Ughuuu Ughuuu force-pushed the add-angle-limits-and-motor-to-pin-joint-2d branch from aee7298 to 0889b55 Compare September 23, 2023 10:52
COPYRIGHT.txt Outdated Show resolved Hide resolved
@Ughuuu Ughuuu force-pushed the add-angle-limits-and-motor-to-pin-joint-2d branch from 0889b55 to e4167dc Compare September 25, 2023 09:23
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Sep 25, 2023
scene/2d/joint_2d.cpp Outdated Show resolved Hide resolved
scene/2d/joint_2d.cpp Outdated Show resolved Hide resolved
scene/2d/joint_2d.cpp Outdated Show resolved Hide resolved
add enabled methods for motor and angular limits
use correct name to get joint
update copyright
@Ughuuu Ughuuu force-pushed the add-angle-limits-and-motor-to-pin-joint-2d branch from e4167dc to 0fcfb07 Compare September 26, 2023 08:29
@akien-mga akien-mga merged commit eefe161 into godotengine:master Sep 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 26, 2023

Thank you!

@hackenshaw
Copy link
Contributor

Why is it a soft limit? Is that intentional? When I think of limits I expect it to be a hard limit (like a hinge)

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Oct 3, 2023

The code was taken from another physics engine that was initially used in Godot Physics 2d, chipmunk physics engine. And there it is implemented like that. I am not really an expert in 2d physics so working with an already implemented solution was easier.

I have added though in this PR also the licensing and the link where it was taken from.

If you want hard limits, in box2d for eg. they have hard limits. I guess it depends on the physics engine and how it's implement? If not we can think of adding another parameter that specifies how hard/soft are the limits?

@hackenshaw
Copy link
Contributor

This is definitely a good step. Thanks for your contribution. I'm also not a physics expert but I needed it for a client project. I'll take a look how it's implemented in box2d. Physics is always super finicky

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Oct 3, 2023

In the box2d one the angle limits are not yet implemented as this was just merged.
In any case, here is the box2d repo: https://github.com/appsinacup/godot-box2d

@mitsuhashish
Copy link

If you want hard limits, in box2d for eg. they have hard limits. I guess it depends on the physics engine and how it's implement? If not we can think of adding another parameter that specifies how hard/soft are the limits?

I think adding another parameter for the stiffness of the limits would help to provide both soft and hard limits

@cody82
Copy link
Contributor

cody82 commented Nov 10, 2023

With which torque does it drive the joint?

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Nov 10, 2023

For the angle limit or for the motor? Also the code is taken from the chipmunk engine, as can be seen, so I'm assuming it's similar to that.

@cody82
Copy link
Contributor

cody82 commented Nov 11, 2023

I meant for the motor. Looks like it can apply unlimited torque.

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Nov 14, 2023

I think so. If you want a different case I can make a proposal and add a maxTorque property maybe?

@cody82
Copy link
Contributor

cody82 commented Dec 10, 2023

Right. But actually the motor does not work for me at all. It just does nothing. I have a wheel which is connected to another body but nothing happens if I set motor_enabled to true and motor_target_velocity to 100. What could be wrong?

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Dec 10, 2023

Here is project for other people to test:

Screen.Recording.2023-09-18.at.22.23.26.mov

JointLimits.zip

Can you try this project that I tested this PR with and see if that works for you and try to work from that?

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Dec 10, 2023

Also I'm not sure if this kind of help support discussion should happen here, as this PR is closed already. If you need anything, open a new godot proposal or ask on discord or on chat or on forum better. This way it won't notify the main devs also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend the Pin Joint to have angle limits and motor
7 participants