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

Disable joints when process mode is set to disable. #81792

Closed
wants to merge 1 commit into from

Conversation

Ughuuu
Copy link
Contributor

@Ughuuu Ughuuu commented Sep 17, 2023

Also needed if we want breakable joints, as right now there is no good way to disable a joint.

@AThousandShips
Copy link
Member

You will have to add it to the 3D nodes as well before CI can pass, the documentation won't be correct otherwise

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 17, 2023

How would I go about disabling the joint? For physics bodies, they get added and removed from the space. For joints, I can call clear_joint and then set it again after that.

Also, I am not getting the notifications called, I am assuming since the individual objects have the _notification callback implemented(eg. PinJoin gets called, but not Joint base class). Should I make a helper and call from each one, or make the Pin call into base?

@AThousandShips
Copy link
Member

To avoid code style errors try using clang-format locally

@Ughuuu Ughuuu marked this pull request as ready for review September 17, 2023 12:09
@Ughuuu Ughuuu requested review from a team as code owners September 17, 2023 12:09
@Ughuuu Ughuuu requested review from a team as code owners September 17, 2023 12:57
@Ughuuu Ughuuu force-pushed the add-joint-disable branch 3 times, most recently from bc4dff6 to d861002 Compare September 17, 2023 13:24
@AThousandShips AThousandShips changed the title Disable joints when process mode is set for node to disable. Disable joints when process mode is set to disable. Sep 18, 2023
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 18, 2023

Ok, this seems to be working fine. It's ready for another review. Thanks

scene/2d/joint_2d.cpp Outdated Show resolved Hide resolved
scene/3d/joint_3d.cpp Outdated Show resolved Hide resolved
@mihe
Copy link
Contributor

mihe commented Sep 18, 2023

I haven't tested this myself, and this might be more of a subjective issue than an objective one, but...

By utilizing _update_joint(true) for this (when using DISABLE_MODE_REMOVE) you're obviously removing the underlying joint from the server altogether, and thereby also discarding the stored reference frames (relative transforms) for the two bodies. I assume then that once you re-enable the joint the bodies will instead be constrained at whatever reference frames they have at that point in time, rather than the original ones?

I guess it becomes a question of what disabling/enabling a joint really means, or more specifically what the expectation is when enabling a previously disabled joint.

If you did the disabling on a server level instead you would presumably not end up with that "problem", since the underlying joint would be kept intact and instead just omit the impulses that are usually applied. I'm not entirely sure how you go about doing that in Godot Physics, but I would assume it's as simple as just doing an early-out in the solve method of each joint.

(This also happens to line up better with how Jolt behaves, which allows you to just omit the impulses through its Constraint::SetEnabled method, and is what you'd use for breakable joints, so I'm a bit biased here.)

@mihe
Copy link
Contributor

mihe commented Sep 18, 2023

I'm not entirely sure how you go about doing that in Godot Physics, but I would assume it's as simple as just doing an early-out in the solve method of each joint.

I suppose you could also take a similar approach to what the collision objects do with regards to being added/removed from the space by adding (on a server level) the ability for joints to add/remove themselves from the bodies they constrain.

So you'd turn this into a method instead:

for (int i = 0; i < get_body_count(); i++) {
GodotBody3D *body = get_body_ptr()[i];
if (body) {
body->remove_constraint(this);
}
}

... and I guess you'd need to turn this into a method on every joint:

A->add_constraint(this, 0);
B->add_constraint(this, 1);

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 18, 2023

Not sure tbh what is expected when a joint is enabled after it's disabled. I wouldn't have high hopes for that to do much.

If the objects are by now further away, and you use the initial configurations for it, it will result in physics instability.

If you use the current distance and probably the old configs for the joint, it will work, but the joint will now have a new distance between the nodes. Which will result in more physics stability, but probably not what the user expects.

I think if the joint gets disabled and reenable, it's as if you created the joint from scratch. But it all depends on what your use case is for the joint, I guess, and if you can do both things with what is proposed.

If the joint is always recreated with current bodies transforms, you could just move the bodies to the old positions, and have point 1 solved. But otherwise, if you want to reset the transforms after disable/enable, you have to clear the joint and then set again the old properties.

So yeah, either way we go for, it's gonna be something that we should document I guess, and agree that it's the most obvious choice.

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Sep 18, 2023

Let's take specific cases I guess.

If a bridge breaks, and then the bridge goes from:

A)

o------o

To
B)

o.     o
|.     |
|.     |

Now, if you reenable the joint, should the bridge try to go back to state A? I would think if that happens, the game developer would want to at least reset the bridge to its initial position.

Or should it, if you enable it and the objects are in weird positions, have a very long joint. It's not ideal, but it's they get what they see. If they want a short joint, they can always teleport the objects close to another and recreate state A.

@Ughuuu Ughuuu force-pushed the add-joint-disable branch 8 times, most recently from 3095bcb to 4c609f4 Compare October 31, 2024 22:11
@Ughuuu Ughuuu requested a review from rburing November 1, 2024 09:55
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Nov 1, 2024

Rebased

fix joint disable cases

fix copy from

update docs

add missing enum

update feedback

remove disable mode clear

upd

fix

update feedback

update feedback part 1

fix build

typo

fix

Update godot_joint_3d.h

Update physics_server_2d_wrap_mt.h

Update joint_3d.h

Update physics_server_3d_extension.h

upd

Update joint_3d.cpp

whitespace diff

lint

Delete sandbox

lint
@Ughuuu Ughuuu force-pushed the add-joint-disable branch from 4d7e4dd to 4b789f9 Compare November 1, 2024 10:15
Copy link
Member

@rburing rburing left a comment

Choose a reason for hiding this comment

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

This looks fine to me now. Thanks for working on this!

Physics server GDExtensions should override the new virtual methods for 4.4.

If you use an older physics server GDExtension which does not override these virtual methods then if you disable joints with the default disable mode in 4.4 you will get error messages about those virtual methods not being overridden, and the behavior will be the same as before this PR.

@rburing rburing modified the milestones: 4.x, 4.4 Nov 3, 2024
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Nov 4, 2024

Forgot to post a video of it working, here it is:

Screen.Recording.2024-11-04.at.13.46.38.mov

Code is:

extends PinJoint2D

func _on_timer_timeout() -> void:
	if process_mode == Node.PROCESS_MODE_ALWAYS:
		process_mode = Node.PROCESS_MODE_DISABLED
	else:
		process_mode = Node.PROCESS_MODE_ALWAYS

With a timer that times out every 1s.

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jan 12, 2025

For further references, last word I got is that this PR still needs work, but I decided not to dedicate any of my time on maintianing it, but instead just adding it myself on a fork of Godot and using it there. Sorry for wasting time.

Edit:

For context. I started this PR in 2023, as the PR says. I implemented it, asked for feedback, etc. But it just seems stuck. I wanted to use this to make breakable and rejoinable softbodies for a cooking game, but instead of doing that I mostly fought the engine or trying to add my features here. If I would have done the game alone, I wouldn't have even wanted to add it to the engine, but since I work with an artist, I wanted her to also be able to use this without needing a custom engine build. Now it's 2025, and I really want to focus on the game, and whatever else is a blocker, is a blocker and will always be it seems. This PR just doesn't want to find it's way to Godot mainstream engine, for things that are outside of my powers.

@lucasmaffazioli
Copy link

Thanks for the proposal Ughuuu, was looking forward this PR. Is the fork Blazium, Redot, or private?

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jan 12, 2025

Blazium, will add it to it these following days and merge it there. Thanks for asking. I am a contributor there and they usually review things pretty quickly. I am the only physics expert there, so there is probably less bureaucracy and chances are that it will get accepted in 1-2 days.

@hpvb
Copy link
Member

hpvb commented Jan 13, 2025

For further references, last word I got is that this PR still needs work, but I decided not to dedicate any of my time on maintianing it, but instead just adding it myself on a fork of Godot and using it there. Sorry for wasting time.

That's not entirely true, you came to ask what was up, I said that the status of the pr said that it needed review, but that I suspected that it was just an oversight.

I suggested you post in this PR so someone would look at the status, but you closed the PR instead, and wrote a much longer message.

I'm a little confused about this turn of events to be entirely honest.

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jan 13, 2025

Thanks for the message @hpvb, but the care about this PR specifically is not here, not sure it is, but it is just neglected by Godot team. As I said above, my PR has last updates from Nov (and it is already a very old PR, 2023). It isn't merged yet, I have no clue as for what happens. New year up, new dev version, my PR is not there. In my eyes, I have 0 visibility as to what happens, I just have to wait and pray that my free code I contribute for free gets maybe accepted into the engine.
It's not two ways at all, I contribute all the code, I put all the effort, and I am then ignored.
Secondly, about your response message, saying my message is not entirely true. Please know that all of this is a communication issue from Godot team, as I said, I have no clue what happens to my PR from Nov. The normal way things go with this PR so far is. It's reviewed, then someone else merges something, I need to rebase and fix conflicts. Always. It never gets merged, I never get an estimate, no nothing. Now you wanna come along saying what I said is not true, but is it then false? Did I ever get an estimation on the review, a what is going on? I even put an update gif showing the feature working, expecting some kind of update.
Jolt for example, which is also a physics feature, got a preferential treatment. This PR is already reviewed and approved, but it is not merged. Looking objectively at the message Godot sends to me is:

  • PR reviewed
  • PR approved
  • (now what?)
    There is 0 communication on this. I wait and see other PR's getting merged, why others are getting merged and not this?

And you say what I said is not entirely true. Then tell me the truth. What is going on, why can't I get any kind of response to my PR. Again I contribute for free my time to this, but my time is ignored.

And for my last point on what I wrote on the chat (which btw, having the rocket chat, if you ask me, just complicates things even more). I asked there in the past for reviews on this, but last time I was ignored also for some time, and I asked, I got messages from @Faless saying I should stop it with the name calling. Sure maybe I was a bit heated about this being ignored, but if you ask me, it's because I cared. So at that point, as I said on the chat, I was thinking, oh, if i write there again, I might get banned, who knows. Also, how do you think I feel, keep asking about this PR getting reviews over and over. When I asked sometime there also, again some other maintainer said I should be careful of contributors (@rburing) time, as they are doing this out of the greatfulnes of their heart. So again, sorry for taking time from everyones busy schedule.

But please do not say what I said is not true. If I am coming to a chat, asking for status after months, and you say the status is needs review, and it is not, then better not say anything. Also, instead of admitting to a mistake, you chose to say I am wrong/lying and saying things that are not true.

I have now checked the conversion and I see @rburing answered after you saying that what you said is not true. So what I said not being true (a lie) is not correct. What I said in the moment I saw what you wrote is correct.

I believe what happens here, including the likes the Godot team choses to stay behind and not write anything, perfectly sums up the experience with Godot. I take my time to write a fully working PR, adding correct message, everything.
And then the Godot team takes 1 minute to look at my stuff, and say something incomplete that causes confusion. Not the same amount of dedication/effort put in from both sides. And it shows. Had I not commented on the chat, this would have not been merged ever, and it will not be merged ever now since I closed it. But form Nov when was last update, no new word, nothing. I believe easily feb, march, etc. Could have come and no new update on this, easily. And this is not the only PR that suffers from this, this is just something that affects me because I thought if I write the code and wait enough, it might get merged sometime. But aparently not.

Edit:
Also notice how ur choice of words is what i said is not entirely correct, instead of u saying sorry for telling you something that was not entirely correct, shifting blame onto me. One goes to discredit me and what i said, increasing the conflict, and choosing to make you and/or godot team look good, not even considering me and why i even said what i said.

@hpvb
Copy link
Member

hpvb commented Jan 13, 2025

@Ughuuu the PR got approved, but the person who approved it forgot to change the status of the PR in the merge queue. This is why it wasn't merged, someone forgot something.

The thing that I was replying to is simply that I, personally, told you this on rocket.chat but instead of understanding that someone made a mistake in November that we can fix you got mad.

You deleted the thread on rocket.chat in which you asked the question, and got the reply, and now you're here in public accusing us of not caring about your PR.

I'm very sorry that a mistake happened here, genuinely that sucks you put a lot of work in it.

But I don't understand your reaction here, they happened after I personally told you that it was a mistake.

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jan 13, 2025

Addressing previous comments:

About the thread that got deleted, sorry about that, it got deleted the moment I deleted my rocketchat account(got frustrated and deleted it). I did that because of the problem that arose here, and the reasons I listed above. My intent was not to hide what I wrote, if anything I still have some screenshots of what I wrote.

Thanks for the apology, I understand the fact that it was a mistake, I just felt the whole thing was very much not caring about me one bit, thats why my reaction. Thats it. I got a little upset and ur response just made me more angry towards the whole thing.

I'm sorry people that receive what I said with negativity or dislike, it's just my thoughts at the moment, cannot change what I think.

Addressing PR approval state

Even thought it is this close to getting finished, I am sure once I start the process there will be other stuff needed, and so on.

My whole problem with this whole thing is, and always has been, lack of communication, of processes, of expectations. It is specifically on the physics side of godot. There is no meetings about it, no transparency, no direct response, no nothing. On the godot-cpp side, for eg. the things just work where the excelent @dsnopek is in charge. On physics side, there is no maintainer, no meeting, no direct answer, no nothing, so things are just taking longer. On the rocket chat almost always for important stuff the answer was (well, but we need to wait for a maintainer for this, for that, etc). But I would rather not contribute my time to this unless I see a full time physics maintainer on godot side that is taking things serious. As I said above, I have taken this seriously, and not as a joke, and I believe not the same amount of seriousness is there from Godot side.

Addressing my view on the whole situation

If anyone wants to take this PR and put it into godot, fine. But it won't be me. As I said I really want to use this feature, so waiting another few months is not something I want (I was really really hoping to get it in the latest dev build, but no, sadly, so many dev builds where I just hoped maybe this time, it might get in).

If/When godot team does want to take this seriously, and not block everything for no reason, I might be re-interested to contribute. But in meantime, it is unrealistic to believe that I will just be like, oh, i'll jsut instantly forget all that happened and continue on. No, some things need to change. I know I am just 1 person and I am not important in the grand scheme of things, but this whole experience has made me less willing to contribute with godot. It's sad but in my eyes, its just this great community and code, but it's all gatekept by some people where everything they do they do, but everything someone else does is met with ignorance.

@hpvb
Copy link
Member

hpvb commented Jan 13, 2025

Hello @Ughuuu I understand your frustration with things taking a while. As we said earlier, we currently don't really have a physics lead and that can lead to delays.

Again this sucks, and I do understand why you feel bad about it.

However none of this was because people didn't want the code, or were trying to spite you, or a lack of seriousness. We are all people, the project is ran by people, largely in their free time. Sometimes things go wrong.

I'm not entirely sure what you mean by the blocking of people. The only people blocked from contributing on GitHub were people who created spam issues.

@akien-mga
Copy link
Member

I understand the pent up frustration after working on a PR for over a year and seeing that it's still not merged, even after it has been approved two months ago.

There was however nothing personal in it taking a long time to review and merge.
Your frustration is understandable, but the anger and negativity addressed towards contributors is unconstructive. I would suggest to take a step back if you're upset.

Now about the lifecycle of this PR specifically:

Physics is an area where we have very few active contributors, and fewer maintainers with the knowledge to evaluate all aspects of proposed physics changes. That's why this PR took so long to reach a first approval.

An approval makes a PR a candidate for merging, and it was tracked as such. The reason it hasn't been merged yet was that we were focusing on the integration of Jolt, following our project priorities. I evaluated this as a change that may impact the Jolt integration too, so I wanted @mihe to also give his approval as he had been involved in discussing the solution. He hadn't gotten to it yet due to working on the Jolt integration and bugfixing.

It was my mistake not to communicate clearly in the PR that I asked for that review, and that it was (a priori) the last requirement before merging the PR.

We're aware that both the PR review process, and our communication on the status of PRs, are lacking. We have a much bigger volume of incoming contributions than contributors available to review them, so there's naturally a bottleneck here - yet we merge around 20 PRs per day, but that's less than what gets opened so some PRs do fall through the cracks and take a longer time to review and merge.

There are no easy solutions there, but we're consistently working on improving both our workflows and our communication on issues, proposals, and PRs.
We understand that the current state of our workflows is not yet "good enough" (neither for contributors who wait too long nor maintainers who are spread too thin), and it's totally fair for some to decide that their time is best spent otherwise.

We're thankful for the work done so far, and understand if you don't want to keep championing this PR. We do think this change addresses a valid issue, so we hope that another physics contributor can take it over (and credit you as co-author).

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jan 13, 2025

Thank you for the nicely written response. I am grateful that the time was taken for this and that this problem was at least addressed/discussed. Thanks for shedding some light over the process and what happened to this PR and why it was in what state it was (had I known it before probably wouldn't have reached this point for me).

I am looking forward to what you wrote about this whole thing improving from the not "good enough" state to hopefully a state that is acceptable.

For me personally,I will try seeing this as a temporary state in which Godot is, but one in which it is nevertheless, but also be understanding this is something that they actively try to get out of. I am referring to the physics team lack of people and PR's adding up, which as you hinted is not maintainable long term probably. In any case, I am hopeful that the day will come when such a change/improvement will happen, and am rooting for Godot to get into that state.

In any case I am the only one with this problem, then sorry for causing a lot of distress. I really wanted my concerns voiced, and while some of them may not have been constructive towards the Godot community, the answers I got today were helpful towards me, so thanks for the carefully crafted response.

As a final metaphor I leave the following comparison I see in my head as to how I see this situation (I know some cultures see this things differently, thats why I exaggerated times here) and how I felt about it:

  • I invite a friend to a restaurant at 2PM. They agree. I arrive at the restaurant, then I order food. I stay there and wait 2 hours, but they do not arrive. I call them, they do not pick up. After 3 hours they show up (at 5PM), the meal is cold and the restaurant is angry towards me for waiting so long on the table.

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.

If you disable a joint, it doesn't get disabled and it still runs.
8 participants