Issue 3191 expose isbullet #5905

otsakir · 2021-06-28T14:17:06Z

Introduces 'bullet' property to collisionobject. See #3191.

Implementation

Property added only for 2D physics - Box2D. In 3D physics if you try to get the 'bullet' property the following warning/error is displayed

WARNING:GAMESYS: 'bullet' property not supported in 3d physics mode
ERROR:SCRIPT: /main/bullet.script:38: '/go#collisionobject' does not have any property called 'bullet'
stack traceback:
  [C]:-1: in function get
  /main/bullet.script:38: in function </main/bullet.script:26>

Testing

The feature is tested by getting and setting the 'bullet' property and asserting it is valid. No test code is written to test actual behavior of a bullet sprite and whether this is accomplished. This is assumed to be part part of box2d testing. Also, you'll notice that i've renamed Sleeping2DCollisionObjectTest to CollisionObject2DTest. The idea is to use this test class as the base for generic high-level collision object testing in gamesys module. Also, properties.script is supposed to be the test script where elementary testing is done for physics properties.

I've also built the editor with bullet property support, created a new project that plays with the property and assured it is shown and initialized properly while getters setters also work. Everything seems to work ok.

Editor

After the core implementation (writing code that makes the calls to box2d and provide the lua api) and test, some further changes were needed to make the property work seamlessly, allow initialization from a .collisionobject file, get displayed in the editor. I used 'Locked Rotation' property as a guide.

defold#3191

… ref defold#3191

that is, besided interfacing with the box2d engine

britzl · 2021-06-28T22:46:02Z

Fantastic! Is the feature ready for review?

otsakir · 2021-07-02T08:50:46Z

@JCash , please tell me if you need any more help with this PR. I noticed there is a test failing. I'll try merging from dev in case something is left out.

JCash

Looks good.
Missing test + implementation for null implementation.

JCash · 2021-07-02T09:02:25Z

engine/gamesys/src/gamesys/components/comp_collision_object.cpp

@@ -1160,6 +1164,15 @@ namespace dmGameSystem
                out_value.m_Variant = dmGameObject::PropertyVar(dmPhysics::GetMass2D(component->m_Object2D));
            }
            return dmGameObject::PROPERTY_RESULT_OK;
+        } else if (params.m_PropertyId == PROP_BULLET) {
+            if (physics_context->m_3D) {
+                // TODO - display not-implemented warning


You've done the TODO, so you can remove the comment :)

engine/gamesys/src/gamesys/test/test_gamesys.cpp

otsakir · 2021-07-04T08:20:45Z

@JCash , i've added "implementation" in physics_2d_null.cpp locally. I still need to understand how this works.

Is there some sort of test i need to add ? I think you mentioned something about test_physics_2d.cpp. Maybe something that just creates a collision object and accesses Bullet* operations ?
In the test build directory i noticed that there are both test_physics_2d and test_physics_2d_null binaries produced. Why have a "test_physics_2d_null" ? What is the behavior i should expect when running these ? I mean, there is a single test_physics but two test binaries produced...

JCash · 2021-07-06T06:27:23Z

@otsakir As I mentioned in the Discord chat, our users use the physics_null and physics_2d_null libraries too.
So if we don't implement the functions there too, they will get Undefined symbols errors when building the game.
An example of a fix for it is here.

Since it's easy to forget, we also added a unit test specifically for this. they are called test_physics_null and test_physics_2d_null.

These tests link against the physics_null and physics_2d_null libraries respectively. So, if you're using a new function in a test, but forget to add the null implementation, it is automatically caught and you'll get a linker error (Undefined symbol).

Try it!
Comment out your null implementation, add a test using the physics function, then build the physics tests, and you should get a linker error.

…efold#3191 Also removed obsolete comment

JCash · 2021-07-12T08:41:48Z

engine/physics/src/physics/test/test_physics_2d.cpp

+    typename TypeParam::CollisionShapeType shape = (*TestFixture::m_Test.m_NewBoxShapeFunc)(TestFixture::m_Context, Vector3(0.5f, 0.5f, 0.0f));
+    typename TypeParam::CollisionObjectType dynamic_co = (*TestFixture::m_Test.m_NewCollisionObjectFunc)(TestFixture::m_World, data, &shape, 1u);
+
+    (*TestFixture::m_Test.m_IsBulletFunc)(dynamic_co);


Is there a reason you don't check the return values of the function?

I thought the test was mostly about calling the functions and assuring they don't produce a link error. Let me fix...

JCash · 2021-07-12T08:42:41Z

engine/physics/src/physics/test/test_physics_2d.cpp

+    typename TypeParam::CollisionObjectType dynamic_co = (*TestFixture::m_Test.m_NewCollisionObjectFunc)(TestFixture::m_World, data, &shape, 1u);
+
+    (*TestFixture::m_Test.m_IsBulletFunc)(dynamic_co);
+    (*TestFixture::m_Test.m_SetBulletFunc)(dynamic_co, false);


SInce the default is false, isn't it better to test to set it to true?

I thought the test was mostly about calling the functions and assuring they don't produce a link error. Let me fix...

otsakir · 2021-07-13T05:59:24Z

@JCash , ready for review!

* Added 'bullet' property to go and underlying glue code with box2d - ref defold#3191 * Issue 3191: getter/setter test for collision-object bullet property - ref defold#3191 * Issue 3191: proper support for 'bullet' property in the engine that is, besided interfacing with the box2d engine * Issue 3191: Added 'bullet' property to editor - ref defold#3191 * Issue 3191: Added bullet tests for null physics implementation - ref defold#3191 Also removed obsolete comment * Issue 3191: Small fix for IsBullet() test - ref defold#3191

otsakir added 4 commits June 16, 2021 13:53

Added 'bullet' property to go and underlying glue code with box2d - ref

934ea7a

defold#3191

Issue 3191: getter/setter test for collision-object bullet property -…

7b98ccf

… ref defold#3191

Issue 3191: proper support for 'bullet' property in the engine

dff1fa1

that is, besided interfacing with the box2d engine

Issue 3191: Added 'bullet' property to editor - ref defold#3191

5c87e7b

Merge branch 'dev' into issue-3191-expose-isbullet

15e7bc4

JCash requested changes Jul 2, 2021

View reviewed changes

Issue 3191: Added bullet tests for null physics implementation - ref d…

65008a6

…efold#3191 Also removed obsolete comment

JCash requested changes Jul 12, 2021

View reviewed changes

Issue 3191: Small fix for IsBullet() test - ref defold#3191

4f1eacb

JCash merged commit b3ac504 into defold:dev Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 3191 expose isbullet #5905

Issue 3191 expose isbullet #5905

otsakir commented Jun 28, 2021

britzl commented Jun 28, 2021

otsakir commented Jul 2, 2021 •

edited

JCash left a comment

JCash Jul 2, 2021

otsakir commented Jul 4, 2021 •

edited

JCash commented Jul 6, 2021

JCash Jul 12, 2021

otsakir Jul 13, 2021

JCash Jul 12, 2021

otsakir Jul 13, 2021

otsakir commented Jul 13, 2021

Issue 3191 expose isbullet #5905

Issue 3191 expose isbullet #5905

Conversation

otsakir commented Jun 28, 2021

Implementation

Testing

Editor

britzl commented Jun 28, 2021

otsakir commented Jul 2, 2021 • edited

JCash left a comment

Choose a reason for hiding this comment

JCash Jul 2, 2021

Choose a reason for hiding this comment

otsakir commented Jul 4, 2021 • edited

JCash commented Jul 6, 2021

JCash Jul 12, 2021

Choose a reason for hiding this comment

otsakir Jul 13, 2021

Choose a reason for hiding this comment

JCash Jul 12, 2021

Choose a reason for hiding this comment

otsakir Jul 13, 2021

Choose a reason for hiding this comment

otsakir commented Jul 13, 2021

otsakir commented Jul 2, 2021 •

edited

otsakir commented Jul 4, 2021 •

edited