-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Document Jolt built-in module in 4.4 #10526
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
Conversation
0a6005d to
b69582d
Compare
|
I don't love the idea of the PR description becoming the user-facing documentation for the Jolt integration in general. It was only ever meant to create a technical "paper trail" of the notable differences that I had observed (and could actually remember) during my time working on the extension, so that there could be buy-in from any reviewers/maintainers that were interested enough to read it all. I would likely have written something user-facing differently. Unfortunately, as much as I'd love to commit to writing something more appropriate for user-facing documentation in time for 4.4-stable, it took me several days to write that PR description, and I don't foresee myself having that kind of time available in the near future, so that puts me in a bit of an awkward position with regards to pushing back on this. If there's a strong enough desire to have something like this merged then I can maybe see about taking some time to go through this, but frankly there's quite a lot still in here that I don't think makes much sense to include, so I'm not sure what it will look like in the end. |
|
For my part, this seems better than nothing. If we can guarantee that new user-facing documentation will be written between now and 4.5stable releasing (which is presumably the earliest that Jolt will be marked not experimental), it's fine to wait for that documentation rather than merging this PR. But if it's unclear that @mihe would have time for that, either, I'm mildly in favor of including some form of this current page with the knowledge that it can be streamlined or removed later. |
|
I do not want 4.4 releasing without some kind of Jolt documentation because I can see that leading to a lot of confusion. If you think too much of this shouldn't be user facing @mihe then just tell me what chunks of it to remove and I'll do that. Alternatively, If you could give me bullet point lists of what's missing in terms of feature parity compared to the extension and GodotPhysics3D I'd be fine with adding those, and removing everything else except for the overview section I wrote, the renamed project settings subsection, and the subsection on thread safety. |
|
I've removed the roadmap section |
Sure, just give me some time to compile a more appropriate list, along with some very brief descriptions. It might be a week or two. |
|
@mihe any update on that list? |
|
No, sorry, not yet. It's been in the back of my mind, but time has been scarce. I'll try and make some time for it this weekend. |
|
Thank you for the feedback, @jrouwe. I was in the middle of typing up some of my own thoughts, but they're a bit more abstract, as I've struggled a bit with distilling this down to something more minimal, like I said I would. I figured I would at least share my thoughts on the broader topics and see if maybe we could work our way down from there, but it got messier than I'd hoped. Here they are anyway: Things that probably should be included
Things I'm not sure should be included
Things I don't think should be included
I can turn this into review comments if you'd all prefer. My hope was still that we wouldn't necessarily base the documentation on my PR description to begin with, but I guess that's where this is headed, and I can't really justify a rephrasing for a lot of these topics anyway. |
|
Applied changes from tetrapod's review |
mihe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a rough pass on what's there currently. Apologies for not formatting the hard wrapping correctly in my suggestions, as well as not formatting the references correctly. I wasn't sure what the exact format was.
Outside of the review comments, I have some other general nitpicks:
- There seems to be a mix of referring to Godot Physics as either "Godot Physics" or "GodotPhysics3D". I would probably suggest we pick one or the other. I don't believe it has any truly official canonical name though. "GodotPhysics3D" is what's shown in the list when picking the physics engine, but there are several places where the engine is referred to as "Godot Physics", including the documentation.
- There's a couple of places referring to "this module". Talking about engine modules in general seems a bit like "contributor speak" to me. Maybe it's worth rephrasing it to be "the Jolt Physics integration" instead or something?
- There's a mix of different casings for the headings, where some have the first word capitalized (e.g. "Joint properties") but some are all capitalized (e.g. "Thread Safety").
tetrapod00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the "Godot Physics" or "GodotPhysics3D" discrepancy too. I initially thought that would be okay to defer to later since both are correct enough.
Existing usage is split both on this page and the rest of the docs. I would just pick one here, my preference would be "Godot Physics".
ae3996c to
d99a9d6
Compare
|
Applied changes from @mihe and tetrapod's reviews. Waiting for Mihe to review jrouwe's comments on bugmarite before changing anything. GodotPhysics3D is now Godot Physics everywhere, and section name casing has been standardized. In regards to the use of "this module" Maybe it would be better to just say Jolt? |
mihe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't done a full pass on it, but there are a number of comments from the previous review that haven't been addressed (maybe because GitHub collapsed them as hidden?).
I went through and resolved the comments that were addressed (or stale).
| Setting this project setting to ``1.0`` will resolve penetration in 1 simulation | ||
| step, resulting in a simulation that more closely resembles Godot Physics, but which | ||
| is also more unstable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it all that useful to mention the 0.0 case? As far as I can tell it just prevents the bodies from moving at all.
Clearly the statement of "more closely resembles Godot Physics" was incorrect on my part, but I must admit I'm a bit confused by this notion that 0.0 would be what disables it, as opposed to 1.0. I would have thought that a 100% error correction would be the default case in any solver, and that anything less is what constitutes the effect of sacrificing correctness for stability, but perhaps I've got that all wrong.
The Baumgarte constant is multiplied by the position error. The position error of a contact constraint is basically how much the two bodies penetrate. So multiplying by 0 means that the system is off, multiplying by 1 means the error is fully corrected. The bodies should still move with a value of 0, they will just not be able to be pushed apart anymore once they penetrate. I mentioned 0 because the value can be between 0 and 1 and I thought it would be useful to specify what the extreme values do. |
Right. I see what you mean. Perhaps I'm reading too much into the name/description of the constant.
Yes, I guess what I meant to say was "prevent the bodies from depenetrating at all".
Fair enough! |
3e4b141 to
8fc8f70
Compare
|
Just went through and applied changes from all the reviews. Apologies about the ones I missed before. |
jrouwe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
mihe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nitpicks left, but what's there looks fine otherwise.
|
I've fixed the nitpicks |
|
@tetrapod00 if you think we're good on reviews now please merge this, there's nothing else I want to change |
|
Thank you, and thank you everyone for the thorough reviews! |
This whole PR is largely the information from the PR that added Jolt (godotengine/godot#99895) copy and pasted, and then heavily edited for proper formatting, adding links, rephrasing and cutting out content that only makes sense for the PR ("maybe we should do this in the future" stuff).
There are not links for the project settings listed at the bottom for name changes as that didn't seem necessary, but I'll add them if people think they should be there.
I've looked over the Jolt PRs merged since this initial PR was merged and I believe the information here is accurate.
@mihe let me know if there's anything at all you wanted added, changed or removed