godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.83k stars 20.96k forks source link

set_length of GrooveJoint2D doesn't change its length #85144

Open stephanbogner opened 10 months ago

stephanbogner commented 10 months ago

Godot version

v4.1.3.stable.official [f06b6836a]

System information

Apple Ventura 13.5, Forward+

Issue description

Setting the length of a GrooveJoint2D doesn't work. The simulated length doesn't actually get updated. It keeps the initially set length.


My guess

It seems the joint's parameters are not actually changed, unlike in e.g. PinJoint2D::set_softness.

There is a pin_joint_set_param and a damped_spring_joint_set_param, but no groove_joint_set_param. If I can find the time I will try to add this function and integrate it into GrooveJoint2D::set_length.

Test setup:

func _ready():
    $Right/GrooveJoint.set_length(200)
Screenshot 2023-11-20 at 17 50 44

Expected result

Both rigid bodies should behave the exact same.

Result

The visualisation of the right groove joint is updated correctly (yellow line), but it doesn't fall lower than the initially specified 50px.

Screenshot 2023-11-20 at 17 58 00

PS: I seem to become the guy that opens all the 2D physics bugs πŸ˜…

Steps to reproduce

  1. Create a GrooveJoint2D connected to two bodies
  2. Create a script that changes the length of the GrooveJoint (neither length nor set_length does work)
  3. Watch the simulation play

Minimal reproduction project

groove-joint-bug.zip

AThousandShips commented 10 months ago

The required code is indeed missing, it does however require the joint to be reconfigured each time as there's no way to update it

stephanbogner commented 10 months ago

@AThousandShips Wow that was a fast reply. Thanks for investigating!

Good to know regarding the reconfiguration. I don't plan to update it dynamically, only to set it once in the beginning according to an exported variable. So at least for me that bug fix would be sufficient and great πŸ™‚

jsjtxietian commented 10 months ago

it does however require the joint to be reconfigured each time as there's no way to update it

I do feel it's a little stricter. Maybe we can do something about it, along with some more documentation.

@stephanbogner Take a look at my pr if it solves your problem.

stephanbogner commented 10 months ago

@jsjtxietian Thanks for looking into it! (This also made me compile Godot for the first time πŸŽ‰ )

A) Setting it once works really nicely!

Underwhelming screenshot but it works as expected πŸ˜…

Screenshot 2023-11-24 at 10 30 06

B) However making it longer later (e.g. after 1 second) makes it behave unexpectedly:

https://github.com/godotengine/godot/assets/7897006/cdfdc061-ade5-4ada-aebf-d312ef44e199

Code replacement for my original example:

extends Node2D

func _ready():
    $Right/GrooveJoint.length = 200
    await get_tree().create_timer(2).timeout
    $Left/RigidBody.apply_central_impulse(Vector2(100, -1000))
    $Right/GrooveJoint.set_length(205)
    await get_tree().create_timer(1).timeout
    $Right/RigidBody.apply_central_impulse(Vector2(100, -1000))

Summary: It seems to solve my problem of setting length initially, but it would require docs to clarify that it can't be changed during runtime (if that is not an easy fix).

jsjtxietian commented 10 months ago

Thank you for your test and congrats to your first compilation of Godot!

  • The ball stops down way further than it should and doesn't go up as far (it appears that the joint has been shifted downwards)
  • If I apply an impulse it swings which the unchanged groove joint does not (but it doesn't swing when I set it in A)

Upon test, yes that's indeed has these problems, other joint2d's usually have a interface for them to directly change the specific para that are set by user, while GrooveJoint2D do not, everytime you update the para it makes a new one, I guess that's where the problem came from.

However the code has not been changed for a while, and finding the real problem could be more time consuming than it seems.

stephanbogner commented 10 months ago

@jsjtxietian Thanks for clarifying. For me your changes would already be a huge improvement :)

FYI: I did some more tests (that I posted in the pull request) and it looks great.

RomanKoziolek commented 9 months ago

Hi all, thank you for the contributions! I will need to modify the GrooveJoint2D's length during runtime in order to control sail line length in a sailing game. Do you know of any workaround that I could use to get around this problem?

The required code is indeed missing, it does however require the joint to be reconfigured each time as there's no way to update it

Regarding comment above, I may not fully understand the difference between reconfiguration and update. Could you clarify?

AThousandShips commented 9 months ago

"Update" here means say to the physics engine "set the length to this", whereas "reconfigure" means "create a new joint with this configuration" πŸ™‚ as is explained further here https://github.com/godotengine/godot/issues/85144#issuecomment-1825504765

RomanKoziolek commented 9 months ago

thank you @AThousandShips.

If I understand this correctly, when this issue is resolved, one could change the length of the GrooveJoint2D by creating new joint with desired length and initial offset and reapply to objects while removing the old joint?

AThousandShips commented 9 months ago

No, that's not it, what I'm saying is that any solution would have to work around it, see the linked PR, this won't change anything from a user perspective (as in you don't have to do anything), just an implementation detail πŸ™‚

RomanKoziolek commented 9 months ago

ok, you're right, code changes won't solve the update in runtime problem. I think I'll create an issue and try figuring out how to work around it. Thanks for the help!

AThousandShips commented 9 months ago

An issue for what? This issue here is solved by the PR, in runtime

The problem of the joint not being updated is completely solved by this once added, no workaround by users needed, as I said πŸ™‚

sbsmirnoff commented 9 months ago

Hey guys,

Great job keeping stuff rolling! Any idea when this PR will be merged and released?

Thanks!