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

Tidy Joints 3D #44146

Closed
wants to merge 2 commits into from
Closed

Tidy Joints 3D #44146

wants to merge 2 commits into from

Conversation

madmiraal
Copy link
Contributor

Currently, Joints 3D is spread across multiple files that don't fit with the general Godot file structure and the structure is significantly different from Joints 2D. Specifically:

  • Joints3D is in a file called scene/3d/physics_joint_3d; instead of scene/2d/joints_3d
  • Each of the Joints3DSW joints and its utility class JacobianEntry3DSW are in a separate file in a folder called joints, instead of together as is done in 2D.

This PR renames scene/3d/physics_joint_3d to scene/3d/joints_3d and moves all the 3D_SW Joints into joints_3d_sw, removes duplicate code, and makes it consistent with Joints 2D.

@madmiraal
Copy link
Contributor Author

Rebased following merge or #44164.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #44391.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #44859.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #45136 and #45167.

@akien-mga akien-mga requested review from a team and removed request for reduz and AndreaCatania January 16, 2021 14:23
@madmiraal
Copy link
Contributor Author

Rebased following merge of #37547 and #45564.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #45852.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #46578.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #47942.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #48050.

@TechnoPorg
Copy link
Contributor

If you need someone to test whether joints still work, I can do it after a rebase! If it's just waiting for someone's approval, this does seems like a cleaner and more consistent structure to me (Although I'll be the first one to say that I don't know much about Godot physics, so count that as approval at your peril 🙂).

@akien-mga
Copy link
Member

Joints3D is in a file called scene/3d/physics_joint_3d; instead of scene/2d/joints_3d

That part seems to have been fixed already in master, it's now scene/3d/joint_3d.h.

Each of the Joints3DSW joints and its utility class JacobianEntry3DSW are in a separate file in a folder called joints, instead of together as is done in 2D.

After discussion in a PR review meeting, we decided not to go in that direction. While it's true that Godot tends to have big files with multiple related classes, we tend to go away from that trend and often split files when they become too unwieldy to work with. Here for 2D the "megafile" is fairly short (less than 200 loc header and 500 loc cpp) so this is not a big issue, while in 3D each file in itself is big enough to be standalone. Moreover, the 3D joints are derived from bullet so it's kind of a library in itself.

@madmiraal
Copy link
Contributor Author

This PR renames scene/3d/physics_joint_3d to scene/3d/joints_3d and moves all the 3D_SW Joints into joints_3d_sw, removes duplicate code, and makes it consistent with Joints 2D.

This PR also removed a lot of duplicated code for example:

static void plane_space(const Vector3 &n, Vector3 &p, Vector3 &q) {
if (Math::abs(n.z) > Math_SQRT12) {
// choose p in y-z plane
real_t a = n[1] * n[1] + n[2] * n[2];
real_t k = 1.0 / Math::sqrt(a);
p = Vector3(0, -n[2] * k, n[1] * k);
// set q = n x p
q = Vector3(a * k, -n[0] * p[2], n[0] * p[1]);
} else {
// choose p in x-y plane
real_t a = n.x * n.x + n.y * n.y;
real_t k = 1.0 / Math::sqrt(a);
p = Vector3(-n.y * k, n.x * k, 0);
// set q = n x p
q = Vector3(-n.z * p.y, n.z * p.x, a * k);
}
}

static void plane_space(const Vector3 &n, Vector3 &p, Vector3 &q) {
if (Math::abs(n.z) > Math_SQRT12) {
// choose p in y-z plane
real_t a = n[1] * n[1] + n[2] * n[2];
real_t k = 1.0 / Math::sqrt(a);
p = Vector3(0, -n[2] * k, n[1] * k);
// set q = n x p
q = Vector3(a * k, -n[0] * p[2], n[0] * p[1]);
} else {
// choose p in x-y plane
real_t a = n.x * n.x + n.y * n.y;
real_t k = 1.0 / Math::sqrt(a);
p = Vector3(-n.y * k, n.x * k, 0);
// set q = n x p
q = Vector3(-n.z * p.y, n.z * p.x, a * k);
}
}

and
static _FORCE_INLINE_ real_t atan2fast(real_t y, real_t x) {
real_t coeff_1 = Math_PI / 4.0f;
real_t coeff_2 = 3.0f * coeff_1;
real_t abs_y = Math::abs(y);
real_t angle;
if (x >= 0.0f) {
real_t r = (x - abs_y) / (x + abs_y);
angle = coeff_1 - coeff_1 * r;
} else {
real_t r = (x + abs_y) / (abs_y - x);
angle = coeff_2 - coeff_1 * r;
}
return (y < 0.0f) ? -angle : angle;
}

static _FORCE_INLINE_ real_t atan2fast(real_t y, real_t x) {
real_t coeff_1 = Math_PI / 4.0f;
real_t coeff_2 = 3.0f * coeff_1;
real_t abs_y = Math::abs(y);
real_t angle;
if (x >= 0.0f) {
real_t r = (x - abs_y) / (x + abs_y);
angle = coeff_1 - coeff_1 * r;
} else {
real_t r = (x + abs_y) / (abs_y - x);
angle = coeff_2 - coeff_1 * r;
}
return (y < 0.0f) ? -angle : angle;
}

While it's true that Godot tends to have big files with multiple related classes, we tend to go away from that trend and often split files when they become too unwieldy to work with. Here for 2D the "megafile" is fairly short (less than 200 loc header and 500 loc cpp) so this is not a big issue, while in 3D each file in itself is big enough to be standalone.

The combined file in this PR is only 1946 lines long. If the reverse were true, would there be justification for splitting the 1946 long file into 5 or 6 files?

Moreover, the 3D joints are derived from bullet so it's kind of a library in itself.

Arguing to keep the files separate because the origin had them is separate files is a weak argument. Godot has copied the algorithms (and hence the attribution) not the code (e.g. git diff --no-index servers/physics_3d/joints/godot_cone_twist_joint_3d.cpp thirdparty/bullet/BulletDynamics/ConstraintSolver/btConeTwistConstraint.cpp) i.e. it's not a library. Note: The original files are much larger (e.g. 1116 vs 359) i.e. justifiably kept in separate files.

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.

4 participants