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

Fix joint RID not being passed to _set in PhysicalBone #44535

Merged
merged 1 commit into from
Dec 23, 2020

Conversation

Demindiro
Copy link
Contributor

@Demindiro Demindiro commented Dec 20, 2020

The joint RID was never passed to the _set() function of the joint_data, which meant the joints on the PhysicalServer's side wouldn't be updated.

Attached is a reproduction project (just the platformer demo but with ragdoll physics). Press tab to toggle between stiff and pudding mode.

I think it would be best to remove the default RID() value for the RID j parameter. It's probably better to be explicit to prevent similar bugs in the future.

@Favkis could you check if this patch fixes your issue please?

platformer.zip
platformer.zip (works just well enough for Godot 4)

@Chaosus Chaosus added this to the 3.2 milestone Dec 20, 2020
@Chaosus
Copy link
Member

Chaosus commented Dec 20, 2020

Changes should be made for the master branch first - that's a general rule of our contributing pipeline. Could you please create a 4.0 version of this PR?

@Chaosus Chaosus changed the title Fix joint RID not being passed to _set in PhysicalBone [3.2] Fix joint RID not being passed to _set in PhysicalBone Dec 20, 2020
@Demindiro
Copy link
Contributor Author

Changes should be made for the master branch first - that's a general rule of our contributing pipeline. Could you please create a 4.0 version of this PR?

I will once @Favkis confirms whether this change fixes his issue or not.

@Favkis
Copy link

Favkis commented Dec 20, 2020

@Demindiro
It works now! Thank you so much!
image

Copy link

@Favkis Favkis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works. Now .set works for 6DOF and other types!
image

2020-12-20.15-45-04.online-video-cutter.com.mp4

As you can see on video, (upper torso is 6DOF), first run character watches up because torso is set to 89 min 90 max. Second time I comment the code and character falls relaxed.
Screenshot with values -125 and -126, works great, upper torso is bent.
image

@Demindiro Demindiro force-pushed the fix-joint-rid-not-passed branch from 6312b73 to a9e7a55 Compare December 20, 2020 17:40
@Demindiro Demindiro requested review from hpvb, reduz and a team as code owners December 20, 2020 17:40
@Demindiro Demindiro changed the base branch from 3.2 to master December 20, 2020 17:41
@Demindiro Demindiro force-pushed the fix-joint-rid-not-passed branch from a9e7a55 to 273f4e7 Compare December 20, 2020 17:45
@pouleyKetchoupp pouleyKetchoupp removed request for a team, hpvb and reduz December 20, 2020 17:46
@Chaosus
Copy link
Member

Chaosus commented Dec 20, 2020

You've redirect it to master? Never do that again - it's not a common practice + this will confuse other contributors which maintain the bug tracker, you should open a new PR that targets it.

@Chaosus Chaosus modified the milestones: 3.2, 4.0 Dec 20, 2020
@Chaosus Chaosus changed the title [3.2] Fix joint RID not being passed to _set in PhysicalBone Fix joint RID not being passed to _set in PhysicalBone Dec 20, 2020
@Demindiro
Copy link
Contributor Author

You've redirect it to master? Never do that again - it's not a common practice + this will confuse other contributors which maintain the bug tracker, you should open a new PR that targets it.

Okay, I'll keep it in mind for next time.

Copy link
Contributor

@madmiraal madmiraal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done. As you suggest:

I think it would be best to remove the default RID() value for the RID j parameter. It's probably better to be explicit to prevent similar bugs in the future.

If you make that change too, I think it will be good to go.

@madmiraal madmiraal added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 21, 2020
@madmiraal
Copy link
Contributor

I've added the cherrypick label, because, although the change is not cherrypickable, the change is easy enough to backport and it was originally created as a 3.2 PR.

@madmiraal
Copy link
Contributor

@Demindiro Thanks for the update. Please squash your commits into a single commit.

Also remove default RID() argument from JointData._set()
@Demindiro Demindiro force-pushed the fix-joint-rid-not-passed branch from ae0682d to 41e00b6 Compare December 22, 2020 12:21
@akien-mga akien-mga merged commit b233f23 into godotengine:master Dec 23, 2020
@akien-mga
Copy link
Member

Thanks!

@Demindiro Demindiro deleted the fix-joint-rid-not-passed branch December 23, 2020 15:09
@Favkis
Copy link

Favkis commented Dec 28, 2020

@Demindiro
Looks like that fixed only .set but not doing same through PhysicsServer:
image
image
image
image

@Demindiro
Copy link
Contributor Author

@Favkis Can you check if print(rid.get_id()) returns 0? I'm fairly certain what the issue is but I'd like to confirm.

@Favkis
Copy link

Favkis commented Dec 28, 2020

@Demindiro
image

@akien-mga
Copy link
Member

Cherry-picked for 3.4.

Looks like that fixed only .set but not doing same through PhysicsServer:

Was there a follow-up for this? If not, it would be good to open a dedicated issue so that it's not forgotten.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 29, 2021
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.

6 participants