Change physics disabled booleans to enabled #44141

madmiraal · 2020-12-06T10:40:31Z

To avoid double negatives, boolean names should be positive. Currently, a Shape defaults to not being disabled. Similarly, to enable collisions between the bodies attached to a Joint's we need to not exclude_nodes.

This PR makes physics boolean names positive, standardises their functions; and updates the documentation.

Part of #16863, specifically this comment and the following two comments.

Note: Includes a clarification of pickable being associated with InputEvents, specifically InputEventMouse events i.e. it has nothing to do with rays (other than a ray being used in 3D to detect objects under the mouse). Also ensures that 2D and 3D are the same (except for 2D being disabled by default and 3D enabled).

pouleyKetchoupp

Thanks for doing this necessary cleaning!

I've left some comments about details, mostly around boolean setters/getters naming.

doc/classes/CollisionObject2D.xml

doc/classes/CollisionShape2D.xml

pouleyKetchoupp · 2020-12-15T17:12:07Z

doc/classes/CollisionObject2D.xml

@@ -31,20 +32,22 @@
 				Creates a new shape owner for the given object. Returns [code]owner_id[/code] of the new owner for future reference.
 			</description>
 		</method>
-		<method name="get_rid" qualifiers="const">
-			<return type="RID">
+		<method name="enable_shape_owner">


It would be a bit clearer to call this method set_shape_owner_enabled since it can be used to disable as well. It can be a simple setter, without the default value.

pouleyKetchoupp · 2020-12-15T17:12:22Z

doc/classes/CollisionObject3D.xml

@@ -35,6 +36,17 @@
 				Creates a new shape owner for the given object. Returns [code]owner_id[/code] of the new owner for future reference.
 			</description>
 		</method>
+		<method name="enable_shape_owner">


Same for 3D.

pouleyKetchoupp · 2020-12-15T17:21:49Z

servers/physics_server_3d.h

@@ -590,8 +590,8 @@ class PhysicsServer3D : public Object {
 	virtual void joint_set_solver_priority(RID p_joint, int p_priority) = 0;
 	virtual int joint_get_solver_priority(RID p_joint) const = 0;

-	virtual void joint_disable_collisions_between_bodies(RID p_joint, const bool p_disable) = 0;
-	virtual bool joint_is_disabled_collisions_between_bodies(RID p_joint) const = 0;
+	virtual void joint_enable_collisions_between_bodies(RID p_joint, const bool p_enable = true) = 0;


There's no need for the default value (same for 2D and 3D), it's always called with an explicit argument (which is clearer anyway).

pouleyKetchoupp · 2020-12-15T17:24:15Z

doc/classes/PhysicsServer2D.xml

@@ -434,6 +434,34 @@
 				Creates a physics body.
 			</description>
 		</method>
+		<method name="body_enable_shape">


Same as enable_shape_owner, it would be more explicit with something like body_set_shape_enabled, without the default value.

pouleyKetchoupp · 2020-12-15T17:24:46Z

doc/classes/PhysicsServer2D.xml

+				Sets whether or not the body's shape specified by [code]shape_idx[/code] is enabled.
+			</description>
+		</method>
+		<method name="body_enable_shape_one_way_collision">


Same as enable_shape_owner, it would be more explicit with something like body_set_shape_one_way_collision_enabled, without the default value.

pouleyKetchoupp · 2020-12-15T17:26:02Z

doc/classes/PhysicsServer3D.xml

@@ -51,6 +51,19 @@
 				Creates an [Area3D].
 			</description>
 		</method>
+		<method name="area_enable_shape">


Same as for 2D with area_enable_shape and body_enable_shape.

pouleyKetchoupp · 2020-12-15T17:26:27Z

doc/classes/PhysicsServer3D.xml

@@ -429,6 +430,30 @@
 				Creates a physics body. The first parameter can be any value from [enum BodyMode] constants, for the type of body created. Additionally, the body can be created in sleeping state to save processing time.
 			</description>
 		</method>
+		<method name="body_enable_continuous_collision_detection">


This method could be body_set_continuous_collision_detection since it can be enabled or disabled.

pouleyKetchoupp · 2020-12-15T17:39:16Z

servers/physics_server_3d.cpp

@@ -231,10 +231,10 @@ void PhysicsShapeQueryParameters3D::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("set_exclude", "exclude"), &PhysicsShapeQueryParameters3D::set_exclude);
 	ClassDB::bind_method(D_METHOD("get_exclude"), &PhysicsShapeQueryParameters3D::get_exclude);

-	ClassDB::bind_method(D_METHOD("set_collide_with_bodies", "enable"), &PhysicsShapeQueryParameters3D::set_collide_with_bodies);


In order to make things more explicit, you can use set_colliding_with_bodies (without the default value), is_colliding_with_bodies, set_colliding_with_areas, is_colliding_with_areas.

See #33026 for the discussion about boolean setters/getters convention.

madmiraal · 2020-12-16T10:20:29Z

To avoid discussing boolean naming standards here, which would be off topic and lost when this PR is merged, I've created godotengine/godot-proposals#1994, where it can be discussed and agreed separately.

madmiraal · 2021-01-09T10:39:22Z

Rebased following merge of #44703.

madmiraal · 2021-01-30T11:33:07Z

Rebased following merge of #45519.

madmiraal · 2021-02-06T12:22:41Z

Rebased following merge of #45564.

madmiraal · 2021-02-14T07:59:15Z

Rebased following merge of #44630, #45775 and #45852.

madmiraal · 2021-02-27T11:49:04Z

Rebased following merge of #45783, #45863, #46148 and #46221.

madmiraal · 2021-03-13T11:49:11Z

Rebased following merge of ##46704.

madmiraal · 2021-03-21T10:43:51Z

Rebased following merge of #46917 and #46937.

KoBeWi · 2021-03-26T16:38:17Z

I think instead of just rebasing every few days you should also address the review comments 🤔

madmiraal · 2021-03-27T08:19:10Z

@KoBeWi As mentioned previously, the review comments were based on a different opinion about what the naming standards should be. I created proposal godotengine/godot-proposals#1994 to keep this discussion separate from the PR. There seems to be general agreement with that proposal; so I don't think there is any need to make any further changes.

madmiraal · 2021-04-06T08:00:17Z

Updated documentation with --doctool ..

madmiraal · 2021-04-17T12:51:56Z

Rebased following merge of #47846.

madmiraal · 2021-05-01T14:47:29Z

Rebased following merge of #42770 and #47485.

akien-mga · 2021-06-18T16:02:58Z

I think the review comments should be addressed to match the current status quo. There are some points still under discussion in godotengine/godot-proposals#1994, and especially the name of setters starting with enable_* for setters which can both set ON and OFF seems unlikely to become the standard IMO.

reduz · 2022-07-29T10:19:26Z

Save for some cases where wording is bad, I generally prefer the boolean as the exception rather the always positive, since it makes it easier to understand the intention and common use case of it.

Also in this case I don't think it can be considered a double negative as 'disabled' is the opposite, but not the negative of 'enabled'

fabriceci · 2022-08-26T09:24:07Z

What's the status on this?

YuriSizov · 2023-01-25T14:08:28Z

Since there is no consensus, there are unresolved requests for changes, this PR has been idle for over a year, and we're at the end of the beta stage, I'm going to close it. Thanks for your contribution nonetheless. Perhaps for Godot 5 we'll figure out a better way to consistently name these boolean properties.

madmiraal added enhancement topic:physics breaks compat labels Dec 6, 2020

madmiraal added this to the 4.0 milestone Dec 6, 2020

madmiraal requested review from AndreaCatania and a team as code owners December 6, 2020 10:40

akien-mga requested a review from pouleyKetchoupp December 6, 2020 12:37

madmiraal mentioned this pull request Dec 8, 2020

[TRACKER] Methods, properties and signals to consider for renaming in next planned compatibility breakage #16863

Closed

83 tasks

pouleyKetchoupp suggested changes Dec 15, 2020

View reviewed changes

madmiraal force-pushed the change-disabled-enabled branch from b5df8f3 to 4b673e8 Compare January 9, 2021 10:34

madmiraal requested a review from reduz as a code owner January 9, 2021 10:34

madmiraal force-pushed the change-disabled-enabled branch from 4b673e8 to 1d21245 Compare January 30, 2021 11:32

madmiraal force-pushed the change-disabled-enabled branch from 1d21245 to 92b5cd5 Compare February 6, 2021 12:22

madmiraal force-pushed the change-disabled-enabled branch from 92b5cd5 to 0baaacd Compare February 14, 2021 07:58

Calinou mentioned this pull request Feb 15, 2021

[3.2] Reduce lightmaps file size. #46054

Merged

madmiraal force-pushed the change-disabled-enabled branch from 0baaacd to a8ac549 Compare February 27, 2021 11:48

madmiraal requested review from a team as code owners February 27, 2021 11:48

madmiraal force-pushed the change-disabled-enabled branch from a8ac549 to 26d863d Compare March 13, 2021 11:48

madmiraal force-pushed the change-disabled-enabled branch from 26d863d to cc01559 Compare March 21, 2021 10:42

madmiraal force-pushed the change-disabled-enabled branch from cc01559 to d1dc168 Compare April 6, 2021 07:57

madmiraal force-pushed the change-disabled-enabled branch from d1dc168 to 442d457 Compare April 17, 2021 12:51

Change physics disabled booleans to enabled.

ac03f8d

madmiraal force-pushed the change-disabled-enabled branch from 442d457 to ac03f8d Compare May 1, 2021 14:47

YuriSizov closed this Jan 25, 2023

YuriSizov added the archived label Jan 25, 2023

YuriSizov removed this from the 4.0 milestone Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change physics disabled booleans to enabled #44141

Change physics disabled booleans to enabled #44141

madmiraal commented Dec 6, 2020

pouleyKetchoupp left a comment

pouleyKetchoupp Dec 15, 2020

pouleyKetchoupp Dec 15, 2020

pouleyKetchoupp Dec 15, 2020

pouleyKetchoupp Dec 15, 2020

pouleyKetchoupp Dec 15, 2020

pouleyKetchoupp Dec 15, 2020

pouleyKetchoupp Dec 15, 2020

pouleyKetchoupp Dec 15, 2020

madmiraal commented Dec 16, 2020

madmiraal commented Jan 9, 2021

madmiraal commented Jan 30, 2021

madmiraal commented Feb 6, 2021

madmiraal commented Feb 14, 2021

madmiraal commented Feb 27, 2021

madmiraal commented Mar 13, 2021

madmiraal commented Mar 21, 2021

KoBeWi commented Mar 26, 2021

madmiraal commented Mar 27, 2021

madmiraal commented Apr 6, 2021

madmiraal commented Apr 17, 2021

madmiraal commented May 1, 2021

akien-mga commented Jun 18, 2021

reduz commented Jul 29, 2022 •

edited

Loading

fabriceci commented Aug 26, 2022 •

edited

Loading

YuriSizov commented Jan 25, 2023

Change physics disabled booleans to enabled #44141

Change physics disabled booleans to enabled #44141

Conversation

madmiraal commented Dec 6, 2020

pouleyKetchoupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madmiraal commented Dec 16, 2020

madmiraal commented Jan 9, 2021

madmiraal commented Jan 30, 2021

madmiraal commented Feb 6, 2021

madmiraal commented Feb 14, 2021

madmiraal commented Feb 27, 2021

madmiraal commented Mar 13, 2021

madmiraal commented Mar 21, 2021

KoBeWi commented Mar 26, 2021

madmiraal commented Mar 27, 2021

madmiraal commented Apr 6, 2021

madmiraal commented Apr 17, 2021

madmiraal commented May 1, 2021

akien-mga commented Jun 18, 2021

reduz commented Jul 29, 2022 • edited Loading

fabriceci commented Aug 26, 2022 • edited Loading

YuriSizov commented Jan 25, 2023

reduz commented Jul 29, 2022 •

edited

Loading

fabriceci commented Aug 26, 2022 •

edited

Loading