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

Fix crash on RigidBody _direct_state_changed #47485

Merged
merged 1 commit into from
Apr 23, 2021
Merged

Fix crash on RigidBody _direct_state_changed #47485

merged 1 commit into from
Apr 23, 2021

Conversation

rafallus
Copy link
Contributor

Fixes: #46003 for master branch

@rafallus rafallus requested a review from a team as a code owner March 30, 2021 06:44
@akien-mga akien-mga added this to the 4.0 milestone Mar 30, 2021
Copy link
Contributor

@madmiraal madmiraal left a comment

Choose a reason for hiding this comment

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

Three points:

  1. As mentioned in Executing RigidBody.new()._direct_state_changed(BoxShape.new()) crashes Godot #46003 (comment), unless there is a reason this method is exposed, the better solution would be to unexpose this method.
  2. This method also exists in KinematicBody, VehicleBody and the equivalents in 2D; so whatever solution is agreed on, those should be updated too.
  3. This will prevent the crash identified in Fix crash on RigidBody _direct_state_changed #47485 in debug mode only i.e. when run from the editor.

@rafallus
Copy link
Contributor Author

@madmiraal

  1. I agree that's a decision to be made.
  2. Thanks for pointing this out.
  3. The comment for the non-debug section says to trust it. So I just leave it as is. I guess for release you are supposed to have already tested your code.

@pouleyKetchoupp
Copy link
Contributor

I agree we should just unexpose the method on master. The method binding seems to be used only because of an internal call (see #46003 (comment)).

@rafallus If you're up for refactoring this fix, the proper changes are:

  • Modify body_set_force_integration_callback in the physics server to use a callable:
    virtual void body_set_force_integration_callback(RID p_body, Object *p_receiver, const StringName &p_method, const Variant &p_udata = Variant()) = 0;
virtual void body_set_force_integration_callback(RID p_body, const Callable &p_callable, const Variant &p_udata = Variant()) = 0; 
struct ForceIntegrationCallback {
	Callable callable;
	Variant udata;
};
  • Modify the call to use Callable::get_object() and Callable::call():
    Object *obj = ObjectDB::get_instance(fi_callback->id);
    if (!obj) {
    set_force_integration_callback(ObjectID(), StringName());
    } else {
    const Variant *vp[2] = { &v, &fi_callback->udata };
    Callable::CallError ce;
    int argc = (fi_callback->udata.get_type() == Variant::NIL) ? 1 : 2;
    obj->call(fi_callback->method, vp, argc, ce);
    }
  • Make the same changes in Bullet module and Godot Physics 2D
  • Use callable_mp macro in calls to body_set_force_integration_callback
  • Remove all the _direct_state_changed bindings

@rafallus rafallus requested a review from a team as a code owner April 9, 2021 18:53
@pouleyKetchoupp
Copy link
Contributor

@rafallus Thanks for refactoring the PR! I haven't reviewed or tested yet, but for info some checks are failing because of compilation errors in test code:

tests/test_physics_2d.cpp: In member function 'RID TestPhysics2DMainLoop::_add_body(PhysicsServer2D::ShapeType, const Transform2D&)':
tests/test_physics_2d.cpp:246:76: error: no matching function for call to 'PhysicsServer2D::body_set_force_integration_callback(RID&, TestPhysics2DMainLoop*, const char [12], RID&)'
  246 |   ps->body_set_force_integration_callback(body, this, "_body_moved", sprite);
      |                                                                            ^
In file included from tests/test_physics_2d.cpp:39:
./servers/physics_server_2d.h:480:15: note: candidate: 'virtual void PhysicsServer2D::body_set_force_integration_callback(RID, const Callable&, const Variant&)'
  480 |  virtual void body_set_force_integration_callback(RID p_body, const Callable &p_callable, const Variant &p_udata = Variant()) = 0;
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./servers/physics_server_2d.h:480:15: note:   candidate expects 3 arguments, 4 provided

It seems there are some more cases in test_physics_3d.cpp as well.

@rafallus rafallus requested a review from a team as a code owner April 9, 2021 19:21
@rafallus
Copy link
Contributor Author

rafallus commented Apr 9, 2021

Still there are some tests that haven't passed. I'll check that later

@rafallus
Copy link
Contributor Author

Ok, the PR is ready for review

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.

Looks good except for 2 things:

  • There are currently errors when starting the editor or game because of body_set_force_integration_callback in physics servers has different arguments now. The method binding needs to be updated here:

    ClassDB::bind_method(D_METHOD("body_set_force_integration_callback", "body", "receiver", "method", "userdata"), &PhysicsServer2D::body_set_force_integration_callback, DEFVAL(Variant()));

    (and the same in the 3d physics server)

  • The documentation for physics servers needs to be updated too:

    <method name="body_set_force_integration_callback">
    <return type="void">
    </return>
    <argument index="0" name="body" type="RID">
    </argument>
    <argument index="1" name="receiver" type="Object">
    </argument>
    <argument index="2" name="method" type="StringName">
    </argument>
    <argument index="3" name="userdata" type="Variant" default="null">
    </argument>
    <description>
    Sets the function used to calculate physics for an object, if that object allows it (see [method body_set_omit_force_integration]).
    </description>
    </method>

    (again, for the 3d version as well)

@rafallus rafallus requested a review from a team as a code owner April 14, 2021 01:08
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.

Suggested a couple additional cleanups, but looks great overall. Thanks!

@akien-mga akien-mga requested a review from madmiraal April 14, 2021 07:07
Copy link
Contributor

@madmiraal madmiraal left a comment

Choose a reason for hiding this comment

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

It looks good. Just, as suggested below, the original check this PR suggested either needs to be applied consistently or not at all.

scene/3d/physics_body_3d.cpp Show resolved Hide resolved
Removed _direct_state_changed bindings
Affects 2D and 3D nodes
Callbacks now use Callable
Tests were changed accordingly
@akien-mga akien-mga requested a review from madmiraal April 23, 2021 09:50
@akien-mga akien-mga merged commit 1a3d609 into godotengine:master Apr 23, 2021
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

@twocountriestrus Please don't spam PRs with "Thanks"

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.

Executing RigidBody.new()._direct_state_changed(BoxShape.new()) crashes Godot
5 participants