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

Initialize variables in servers/physics #52668

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

qarmin
Copy link
Contributor

@qarmin qarmin commented Sep 14, 2021

Part of #43636

@qarmin qarmin added this to the 4.0 milestone Sep 14, 2021
@qarmin qarmin requested a review from a team as a code owner September 14, 2021 13:06
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Thank you! I've double checked initialized values and left some comments about missing ones.

servers/physics_2d/physics_server_2d_sw.h Outdated Show resolved Hide resolved
servers/physics_2d/shape_2d_sw.cpp Outdated Show resolved Hide resolved
servers/physics_2d/joints_2d_sw.cpp Show resolved Hide resolved
servers/physics_2d/space_2d_sw.h Outdated Show resolved Hide resolved
servers/physics_3d/area_3d_sw.h Outdated Show resolved Hide resolved
servers/physics_3d/collision_object_3d_sw.cpp Show resolved Hide resolved
servers/physics_3d/collision_object_3d_sw.h Outdated Show resolved Hide resolved
custom_bias = 0;
configured = false;
}
Shape3DSW::Shape3DSW() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty constructors could be removed.

Copy link
Contributor Author

@qarmin qarmin Sep 15, 2021

Choose a reason for hiding this comment

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

I was only able to move empty constructors to .h files, because probably due virtual destructors, build fails when completely removing default constructor.

@qarmin qarmin force-pushed the cppcheck_servers_physics branch 3 times, most recently from 405a8e1 to ab26323 Compare September 15, 2021 05:03
Comment on lines 110 to 113
BroadPhase2DBVH::BroadPhase2DBVH() {
bvh.set_pair_callback(_pair_callback, this);
bvh.set_unpair_callback(_unpair_callback, this);
pair_callback = nullptr;
pair_userdata = nullptr;
unpair_userdata = nullptr;
}
Copy link
Member

Choose a reason for hiding this comment

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

This might be worth reviewing @pouleyKetchoupp @lawnjelly as the previous code seems a bit weird. We set callbacks in the bvh manager before setting only the pair_callback to nullptr and leaving unpair_callback uninitialized.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this is fine but the code is hard to read :)

It's just that there's _pair_callback (with underscore) passed to the bvh and pair_callback (without) which is initialized to nullptr.

unpair_callback was uninitialized though, so it's all good now.

@@ -163,7 +163,7 @@ class JacobianEntry3DSW {
Vector3 m_0MinvJt;
Vector3 m_1MinvJt;
//Optimization: can be stored in the w/last component of one of the vectors
real_t m_Adiag;
real_t m_Adiag = 1.0;
Copy link
Member

@akien-mga akien-mga Sep 15, 2021

Choose a reason for hiding this comment

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

Seems to have a fail condition if it's <= 0.0, maybe the default should be 0.0 as uninitialized state?

servers/physics_3d/joints/jacobian_entry_3d_sw.h
73:             m_Adiag = massInvA + m_0MinvJt.dot(m_aJ) + massInvB + m_1MinvJt.dot(m_bJ);
75:             ERR_FAIL_COND(m_Adiag <= real_t(0.0));
89:             m_Adiag = m_0MinvJt.dot(m_aJ) + m_1MinvJt.dot(m_bJ);
91:             ERR_FAIL_COND(m_Adiag <= real_t(0.0));
104:            m_Adiag = m_0MinvJt.dot(m_aJ) + m_1MinvJt.dot(m_bJ);
106:            ERR_FAIL_COND(m_Adiag <= real_t(0.0));
121:            m_Adiag = massInvA + m_0MinvJt.dot(m_aJ);
123:            ERR_FAIL_COND(m_Adiag <= real_t(0.0));
126:    real_t getDiagonal() const { return m_Adiag; }
166:    real_t m_Adiag;

CC @pouleyKetchoupp

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it would be good to check uninitialized cases, but we would also need a check in getDiagonal() to catch these cases (debug only to avoid potential performance hits in release), something like:

real_t getDiagonal() const
{
#ifdef DEBUG_ENABLED
    ERR_FAIL_COND_V(m_Adiag <= real_t(0.0), m_Adiag);
#endif
    return m_Adiag;
}

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth doing in a separate PR not to grow the scope of this one too much I guess.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Reviewed everything, looks great aside from the few comments I left. Should be mergeable once resolved.

@qarmin qarmin force-pushed the cppcheck_servers_physics branch from ab26323 to be048f6 Compare September 15, 2021 09:15
@qarmin qarmin marked this pull request as draft September 15, 2021 09:35
@qarmin qarmin force-pushed the cppcheck_servers_physics branch from be048f6 to 5cee6ea Compare September 15, 2021 17:40
@qarmin qarmin force-pushed the cppcheck_servers_physics branch from 5cee6ea to 91257c3 Compare September 15, 2021 17:41
@qarmin qarmin marked this pull request as ready for review September 15, 2021 17:43
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great!

@pouleyKetchoupp pouleyKetchoupp merged commit 062cff3 into godotengine:master Sep 16, 2021
@qarmin qarmin deleted the cppcheck_servers_physics branch September 16, 2021 16:16
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.

3 participants