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

Additional PinSlotJoint 2D implementation with DOF: 1 translation + 1 rotation #775

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

legendofa
Copy link
Contributor

@legendofa legendofa commented Dec 14, 2024

Implements additional GrooveJoint from to issue: #772
The joint is currently only implemented for 2d behind the feature flag "dim2".
This PR includes a test for the GrooveJoint.

If there is anything that i have overlooked, let me know :) I am also planning to update the documentation with this joint type if everything else looks fine.

@legendofa legendofa marked this pull request as ready for review December 31, 2024 12:06
@legendofa legendofa force-pushed the feature-groove-joint-2d branch from fe7284f to 1c62ac1 Compare January 17, 2025 14:10
@legendofa
Copy link
Contributor Author

Just rebased this PR on dimforge:master to fix the CI

@Vrixyz
Copy link
Contributor

Vrixyz commented Jan 20, 2025

Thanks for the PR!

I'm a bit skeptical on the naming, I think this behaviour refers more accurately a PinSlot, but that's a detail, if we're drawing inspiration from Godot though, I'd like:

  1. To be sure about what we're mimicking (Add support for 1Translation+ 1Rotation 2d Joint #772 (comment))
  2. Add a documentation cross-reference to godot to make sure intent is preserved.

Those are not blocking actions to be taken in the PR (1 is uncertain, 2 is trivial and can be done at merge-time), but hopefully helps understanding the current state of approval consideration.

I'm still approving this PR as I believe this brings value.

@legendofa
Copy link
Contributor Author

legendofa commented Jan 22, 2025

Thanks for the feedback,

it might be good to rename the joint from GrooveJoint to PinSlotJoint as i also think this is the more common naming, for example used by MathWorks: https://www.mathworks.com/help/sm/ref/pinslotjoint.html. PinSlotJoint could give a better hint to what the joint allows just by reading the name.

I will start working on the documentation today and reference the inspirations for this joint there.

@legendofa legendofa changed the title Additional groove joint 2D implementation with DOF: 1 translation + 1 rotation Additional PinSlotJoint 2D implementation with DOF: 1 translation + 1 rotation Jan 23, 2025
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.

2 participants