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.

Sign up for GitHub

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

Jump to bottom

[3.2] Optimized physics object spawn time #40188

Closed
wants to merge 3 commits into from
Closed

[3.2] Optimized physics object spawn time #40188

wants to merge 3 commits into from

Conversation

aaronfranke
Copy link
Member

Backported to 3.2 from pull request #39726 from AndreaCatania/add_body_impr_physics

Tested on the Voxel demo project from the Godot demo projects repo.

@aaronfranke aaronfranke added this to the 3.2 milestone Jul 7, 2020
@aaronfranke aaronfranke requested a review from AndreaCatania July 7, 2020 19:49
@aaronfranke aaronfranke force-pushed the 3.2-add_body_impr_physics branch from 7a7d2f1 to b72e70a Compare July 7, 2020 19:51
@akien-mga
Copy link
Member

FYI, when cherry-picking commits I prefer using git cherry-pick -x <commit hash> and then resolve the conflicts, which will preserve the commit log and author information, and add a reference to the original commit hash in the commit description.

@aaronfranke
Copy link
Member Author

I'll keep that in mind for next time. If the changes conflict too much, a lot of the time I just try to get through the conflict resolution tool as soon as possible, then I fix the code manually and squash. Perhaps it's not the best approach :P

@akien-mga
Copy link
Member

akien-mga commented Jul 8, 2020 •
edited
Loading

Yeah I don't use any conflict resolution tool or strategy myself, I just open all conflicting files in nano or kate and fix conflicts manually, and then git add the changes and git cherry-pick --continue.

@madmiraal
Copy link
Contributor

Note: #40311 is a regression in #39726 that needs to be resolved before merging this.

@akien-mga akien-mga mentioned this pull request Jul 28, 2020
Merged
@aaronfranke
Copy link
Member Author

aaronfranke commented Aug 3, 2020 •
edited
Loading

#40252 solved #40311, but it caused #40840 and #40848, and the pending fix for the former is #40886.

EDIT: I cherry-picked #40252 and #40886 (+ the suggested change) onto this PR.

@aaronfranke @AndreaCatania
[3.2] Optimized physics object spawn time
55b4f73 
Backported from pull request 39726 from AndreaCatania/add_body_impr_physics

Co-authored-by: Andrea Catania <[email protected]>
@aaronfranke aaronfranke force-pushed the 3.2-add_body_impr_physics branch from b72e70a to 47c195d Compare August 3, 2020 05:57
aaronfranke and others added 2 commits August 3, 2020 01:05
@aaronfranke
- Enhanced the flush mechanism by flushing only needed thing.
4e57e8b 
- Flushing Areas before anything else.
- Make sure to correctly fetch gravity when the integrate_forces function is used
- Lazy reload body when layer and mask changes
- Shapes are reloaded just before the physics step starts.
- Improved some other parts of the code.
- Added override keyword
f
@madmiraal @aaronfranke
Add call to Bullet pre_process() at start of dispatch_callbacks(),
24d50fd 
and add RigidBodyCollisionObjectBullet to flush queue when a shape
reload is needed.
@aaronfranke aaronfranke force-pushed the 3.2-add_body_impr_physics branch from 47c195d to 24d50fd Compare August 3, 2020 06:06
AndreaCatania
AndreaCatania suggested changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, just few things to change.

modules/bullet/collision_object_bullet.cpp
need_shape_reload = true;
if (space) {
space->add_to_flush_queue(this);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The shape should not be reloaded during the flush but during the pre_process. To fix the issue #40840 you need to insert p_body->pre_process(); at the beginning of the function test_body_motion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand what you mean. I already added p_body->pre_process(); in test_body_motion on the line you mentioned in this comment #40886 (comment) (see 24d50fd#diff-a61d01bcaef4874b6713c0b00e6d4571R1014)

modules/bullet/rigid_body_bullet.cpp
}
}

void RigidBodyBullet::dispatch_callbacks() {
RigidCollisionObjectBullet::dispatch_callbacks();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed. Check this: #40886 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

So you want me to delete the call to RigidCollisionObjectBullet::dispatch_callbacks() inside of RigidBodyBullet::dispatch_callbacks()? I don't understand what that comment has to do with this line.

@AndreaCatania
Copy link
Contributor

Would be nice port LocalVector, and use it here too. So to have feature parity between 3.2 and the 4.0. This should also remove the need to cast the value returned by size: for (uint32_t i = 0; i < uint32_t(areasWhereIam.size()); i += 1) {

@aaronfranke
Copy link
Member Author

If we need to add core engine features to backport this feature, then it might be better to just not backport this, and simply require that people wait for 4.0 if they want improved physics object spawn times.

@AndreaCatania
Copy link
Contributor

Port LocalVector is just matter of insert it into the core directory. Then, you can use it into the bullet. It's not super needed, but if done you can remove a lot of overhead.

Feel free to do it, or not.

@aaronfranke
Copy link
Member Author

Because of the numerous regressions caused by #39726, #40252, and maybe others, I'm closing this. It would have been nice to have in 3.2, since it significantly improves performance in the Voxel demo, but there are just too many regressions.

I'm just going to make a blanket statement that the dumpster fire known as Godot's physics is far too risky to take cherry-picks from, and from now on I would suggest all physics changes be kept as 4.0+ only.

@aaronfranke aaronfranke closed this Aug 5, 2020
@aaronfranke aaronfranke deleted the 3.2-add_body_impr_physics branch August 5, 2020 18:08
@AndreaCatania
Copy link
Contributor

After all the work done here, I would not close this. But well, is up to you I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreaCatania AndreaCatania AndreaCatania requested changes

Assignees
No one assigned
Labels
archived enhancement topic:physics
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.
4 participants
@aaronfranke @akien-mga @madmiraal @AndreaCatania