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

add consideration for 2d physics center_of_mass #29867

Closed

Conversation

Raphael2048
Copy link
Contributor

add consideration for 2d physics center_of_mass
This should close #12353

@Raphael2048 Raphael2048 requested a review from a team as a code owner June 18, 2019 11:52
@Raphael2048 Raphael2048 force-pushed the add_2d_center_of_mass branch 2 times, most recently from 1860c1e to 1bb74d7 Compare June 18, 2019 12:53
@akien-mga akien-mga added this to the 3.2 milestone Jun 18, 2019
@bojidar-bg
Copy link
Contributor

Doesn't calculating the center of mass from the shapes (instead of assuming it is at (0,0)) break compatibility?

@Raphael2048 Raphael2048 force-pushed the add_2d_center_of_mass branch from 1bb74d7 to ab2bc82 Compare June 19, 2019 03:17
@Raphael2048
Copy link
Contributor Author

It does, but in mostly cases, Rigidbody2D's center is at (0, 0).

@capnm
Copy link
Contributor

capnm commented Jun 21, 2019

There should be an explicit center of mass relative to the origin.
If set, this calculation shouldn't be done.

@Raphael2048
Copy link
Contributor Author

I'll try to add a method to do this after this commit is merged.
As this issue mentioned #7136

@capnm
Copy link
Contributor

capnm commented Jun 21, 2019

Could you perhaps include the first step and add a boolean option, e.g. 'relative to origin' by default true? That wouldn't break the compatibility.
I suspect that this is very error prone. Have you tested what happens when you change the scale or shape dynamically?

@Raphael2048
Copy link
Contributor Author

It's same as physic 3d's behavior. I think it's not needed to add it, because you will never want to shift your shape offset origin in fact. I have tested dynamically changing it.

@capnm
Copy link
Contributor

capnm commented Jun 21, 2019

Sorry, it's called origin in Blender, here it is the Rigidbody2D Transform. Until this PR, you could just use the Rigidbody2D transform to manually set the center of mass wherever you wanted, but you'll break this if you don't make it optional.
Personally, I think for non-trivial 2D objects, the homogeneous shape area is mostly useless here.

@hubbyist
Copy link

If options included "at origin", "at centroid", "at offset". When third is the default incompatibility may be avoided.

@Xrayez
Copy link
Contributor

Xrayez commented Jun 28, 2019

Well actually I've attempted to create a fix similar to this from 3D, fixed it to some extent, but the movement was weird, that's why I didn't make a PR in the first place, not sure what's different between this PR and:

https://github.com/Xrayez/godot/commit/207ea7d5be3c32df9055b341aeec8c278bc9543e

Seems like I didn't compute custom area, will need to test this then. 🙂

@Xrayez
Copy link
Contributor

Xrayez commented Jun 28, 2019

Tested, seems to work fine, not sure about the third case, perhaps the center of mass must be computed manually for polygon shapes?

center_of_mass_updated

If this is the case, having to override center of mass would be indeed useful.

center-of-mass-2d.zip

@capnm
Copy link
Contributor

capnm commented Jun 30, 2019

An another example where it went terribly wrong ;-)
baloon2

@Raphael2048 Raphael2048 force-pushed the add_2d_center_of_mass branch from ab2bc82 to 781bdeb Compare July 1, 2019 04:46
@Raphael2048
Copy link
Contributor Author

I have updated my PR to take polygon into consideration. @Xrayez

servers/physics_2d/body_2d_sw.cpp Outdated Show resolved Hide resolved
servers/physics_2d/body_2d_sw.cpp Outdated Show resolved Hide resolved
@Raphael2048 Raphael2048 force-pushed the add_2d_center_of_mass branch from 781bdeb to ba2364c Compare July 2, 2019 04:47
Comment on lines 641 to 630
real_t ConvexPolygonShape2DSW::get_area() const {

if (point_count < 3) return 0;

real_t signed_area = 0;
for (int i = 0; i < point_count; i++) {
Vector2 v1 = points[i].pos;
Vector2 v2 = points[(i + 1) % point_count].pos;
signed_area += v1.x * v2.y - v2.x * v1.y;
}
signed_area *= 0.5;
return ABS(signed_area);
}

Vector2 ConvexPolygonShape2DSW::get_shape_center_offset() const {

if (point_count < 3) return Vector2();

Vector2 centroid;
real_t signed_area = 0.0;
real_t a = 0.0;

for (int i = 0; i < point_count; i++) {
const Vector2 &v1 = points[i].pos;
const Vector2 &v2 = points[(i + 1) % point_count].pos;

a = v1.x * v2.y - v2.x * v1.y;
signed_area += a;
centroid += (v1 + v2) * a;
}

signed_area *= 0.5;
centroid /= (6.0 * signed_area);

return centroid;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@akien-mga it would be better if #30222 and #30251 could be merged and used here directly as Geometry methods instead of "hardcoding" here, @raphael10241024 would need to rebase later once merged. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should I convert points to a Vector?

Copy link
Contributor

@Xrayez Xrayez left a comment

Choose a reason for hiding this comment

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

Tested again with polygon center computations, fixes the third case on the GIF above.
I've also tested this on a project of mine with some heavy usage of polygons, works alright even with local (per project) center of mass workarounds, doesn't seem to break compat. The rigid bodies falling look even better subjectively. 🎉

@Xrayez
Copy link
Contributor

Xrayez commented Nov 3, 2019

The ability to change the center of mass is for another topic I guess, I'm not sure if changing the center offset is the proper way to simulate center of mass physics currently. I mean, if you do offset the shapes, you still get some weird simulation going on. If this does break compat, it might be the case for people already relying on the not-so-well-working-feature as a feature.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Nov 26, 2019
@Xrayez
Copy link
Contributor

Xrayez commented May 28, 2020

I wonder whether this has any chance to be merged at this point, because @reduz planned to rework this in 4.1 or so... (Waiting for Godot 😛).

I could work on salvaging this in case @raphael10241024 abandoned the PR.

@aaronfranke
Copy link
Member

@raphael10241024 Is this still desired? If so, it needs to be rebased on the latest master branch.

If not, abandoned pull requests will be closed in the future as announced here.

@aaronfranke
Copy link
Member

@raphael10241024 All of the CI builds failed because you are accessing a private variable.

servers/physics_2d/body_2d_sw.h: In member function 'virtual Vector2 PhysicsDirectBodyState2DSW::get_total_gravity() const':
servers/physics_2d/body_2d_sw.h:353:59: error: 'Vector2 Body2DSW::gravity' is private within this context
  353 |  virtual Vector2 get_total_gravity() const { return body->gravity; } // get gravity vector working on this body space/area
      |                                                           ^~~~~~~

@aaronfranke
Copy link
Member

@raphael10241024 Is this still desired? If so, it needs to be rebased on the latest master branch.

If not, abandoned pull requests will be closed in the future as announced here.

@YuriSizov
Copy link
Contributor

I'm going to close this as superseded by #49610. Thanks for the contribution nonetheless!

@YuriSizov YuriSizov closed this Sep 1, 2021
@YuriSizov YuriSizov removed the request for review from a team September 1, 2021 20:17
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.

Center of mass for RigidBody2D
8 participants