Conversation
ccff3af to
e3ccab6
Compare
1a6171e to
64e8136
Compare
leoetlino
left a comment
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @notyourav)
lib/hkStubs/Havok/Physics2012/Dynamics/Constraint/hkpConstraintListener.h line 31 at r2 (raw file):
class hkpConstraintListener { public: virtual ~hkpConstraintListener() {}
= default; instead of {}
src/KingSystem/Map/mapObject.h line 213 at r1 (raw file):
gdt::FlagHandle getRevivalGameDataFlagHash() const { return mRevivalGameDataFlagHash; } void setRevivalGameDataFlagHash(const gdt::FlagHandle& value) {
nit: just pass the handle by value since it's an integer
src/KingSystem/Map/mapObjectLink.cpp line 286 at r1 (raw file):
} bool ObjectLinkData::sub_7100D4EC40(Object* obj, ObjectLink* link, Object* dest) {
propagatePropsFromIncomingLinks? or something like that
ideally we shouldn't have any sub_xxx name
src/KingSystem/Map/mapObjectLink.cpp line 382 at r1 (raw file):
} bool ObjectLinkData::sub_7100D4EF30(act::ActorConstDataAccess& accessor) {
IMO a placeholder name that describes what the function does is better than a sub_xxxx name
src/KingSystem/Physics/Constraint/physConstraint.h line 24 at r2 (raw file):
public: SEAD_ENUM(ConstrainedType,Parent,Child)
Consider adding spaces after each comma
src/KingSystem/Physics/Constraint/physConstraint.h line 26 at r2 (raw file):
SEAD_ENUM(ConstrainedType,Parent,Child) typedef void (*ConstraintCallback)(void*, Constraint* constraint, bool a2);
using Callback = void (*)(void*, ...)
src/KingSystem/Physics/Constraint/physConstraint.cpp line 18 at r2 (raw file):
: mConstraint(constraint), mCsOption(cs_option) { mDirection = quat->m_vec[0]; mSleepingBodies[ConstrainedType::Parent] = parent;
is this mismatching because of the volatile mess from the ConstrainedType enum? If so that likely means they had a getSleepingBody(ConstrainedType) getter. getSleepingBody(ConstrainedType::Parent) = parent; would construct a ConstrainedType from the ConstrainedType::ValueType value, and then the enum to int volatile cast would happen inside the getter
src/KingSystem/Physics/Constraint/physConstraint.cpp line 21 at r2 (raw file):
mSleepingBodies[ConstrainedType::Child] = child; mActiveBodies[ConstrainedType::Parent] = System::instance()->getWorldRigidBody();
same question for mActiveBodies
src/KingSystem/Physics/Constraint/physConstraint.cpp line 31 at r2 (raw file):
} void Constraint::sub_7100F69FF0() {
pushToSystemIfNotSleeping?
src/KingSystem/Physics/Constraint/physConstraint.cpp line 323 at r2 (raw file):
bool active = mFlags.isOn(Flag::Active); if (active && mConstraint->getOwner() != nullptr) { if (active) {
I think we have a ScopedWorldLock RAII class or something like that to lock the world conditionally
src/KingSystem/Physics/Constraint/physConstraint.cpp line 326 at r2 (raw file):
System::instance()->lockWorld(ContactLayerType::Entity); } mSpinLock.lock();
this might be a sead::ScopedLock as well
src/KingSystem/Physics/Constraint/physConstraint.cpp line 351 at r2 (raw file):
if (attach_to_world && mActiveBodies[ConstrainedType::Child] != System::instance()->getWorldRigidBody()) { if (active) {
this also smells like a RAII guard
src/KingSystem/Physics/Constraint/physConstraintListener.h line 9 at r2 (raw file):
// TODO class ConstraintListener : public hkpConstraintListener { virtual ~ConstraintListener() = default;
~ConstraintListener() override = default;
(actually, do we need to override explicitly here?)
This change is