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.

Sign up for GitHub

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

Jump to bottom

Added collision visibility modes #47204

Closed
wants to merge 1 commit into from
Closed

Added collision visibility modes #47204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions doc/classes/TileMap.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,8 @@
<member name="occluder_light_mask" type="int" setter="set_occluder_light_mask" getter="get_occluder_light_mask" default="1">
The light mask assigned to all light occluders in the TileMap. The TileSet's light occluders will cast shadows only from Light2D(s) that have the same light mask(s).
</member>
<member name="show_collision" type="bool" setter="set_show_collision" getter="is_show_collision_enabled" default="true">
If [code]true[/code], collision shapes are shown in the editor and at run-time. Requires [b]Visible Collision Shapes[/b] to be enabled in the [b]Debug[/b] menu for collision shapes to be visible at run-time.
<member name="show_collision" type="int" setter="set_show_collision" getter="is_show_collision_enabled" enum="TileMap.CollisionVisibilityMode" default="2">
Show or hide collision shapes. See [enum CollisionVisibilityMode] for possible values.
</member>
<member name="tile_set" type="TileSet" setter="set_tileset" getter="get_tileset">
The assigned [TileSet].
Expand Down Expand Up @@ -377,5 +377,14 @@
<constant name="TILE_ORIGIN_BOTTOM_LEFT" value="2" enum="TileOrigin">
Tile origin at its bottom-left corner.
</constant>
<constant name="COLLISION_VISIBILITY_MODE_ALWAYS_SHOW" value="0" enum="CollisionVisibilityMode">
Always show collision shapes in editor and game.
</constant>
Comment on lines +380 to +382
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by these options. There doesn't seem to be a way to show collision shapes in editor, but not in game? That would seem like the most common use case to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

This property is mostly a switch to debug Tilemap collision in both editor and game, with a default option that allows to switch it off on all tilemaps at the same time, but it's true that a 4th option for editor only could be useful too.

Copy link
Member

Choose a reason for hiding this comment

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

To me the expected behavior would be that it behaves like other collision shapes by default:

  • Always visible in the editor
  • Visible in game if "Visible Collision Shapes" is toggled

And then since it can be annoying to have tilemaps always show collision shapes in the editor, or in game with Visible Collision Shapes, there needs to be a way to turn it off.

So IMO a boolean as done initially should have been enough, but maybe its logic was not correct?

  • show_collision = true: collision shapes visible in editor, visible in game if collision debugging is enabled, otherwise not visible
  • show_collision = false: collision shapes not visible in editor nor in game, regardless of debug settings

Is there any other use case worth supporting via an enum?

Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior is what #46623 was originally doing, but it didn't allow hiding all tilemaps at once in the editor when several of them are used as layers, which could cause some problems (see #46623 (comment)).

This PR implements the solution proposed by @groud in #46623 (comment), which is to have more options instead of a boolean, so by default it can be controlled using "Visible Collision Shapes" option for all tilemaps.

A gizmo could be a less confusing alternative to enable/disable collision shapes in the editor, but I don't know if it's possible to have one that would affect only the collision from tilemaps, it seems you can have only one gizmo per node type.

Copy link
Member

Choose a reason for hiding this comment

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

The "Visible Collision Shapes" option is used to configure what is visible at runtime, not in the editor. Collision shapes are always visible in the editor, regardless of whether that option is enabled or not.

So IMO it makes things more confusing if this option starts being used to show/hide tilemap collision shapes in the editor, while still leaving non-tilemap collision shapes always visible.

If "Visible Collision Shapes" is turned off, tilemap collision shapes should never be visible at runtime (but the current documentation suggests for the enum values otherwise).

#46623 (comment) is a misunderstanding of what "Visible Collision Shapes" is intended for. For this user's use case, they should just disable collision shapes visibility for all the tilemaps they don't want to see.

OR we simply switch the boolean to false by default, and let users enable it for the tilemaps where they do want to see visible collision shapes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree again. IMO its special use case(#46623 (comment))

Copy link
Contributor Author

@Janglee123 Janglee123 Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@akien-mga Old pr has this problem, let me know if we are not merging this one, I need to update the old one.

Copy link
Member

Choose a reason for hiding this comment

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

My idea for this would be to make the "Show debug collision shape" settings smarter. And change it to a 3-options too:

  • Hide,
  • Show in editor,
  • Show in editor and in game,

The question is what to do with the shapes. If the idea is to override this setting, we might simply have instead those 4 options per shape:

  • Use global settings,
  • Hide,
  • Show in editor,
  • Show in editor and in game.

I believe this is kind of the most expected way to set all of this, as those 3 options (plus one to "inherit" settings) are basically 3 levels of debug. From the "In don't want to debug physics" to the "I have issues with physics, show me shapes in game".

Copy link
Contributor

Choose a reason for hiding this comment

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

OR we simply switch the boolean to false by default, and let users enable it for the tilemaps where they do want to see visible collision shapes.

Maybe this is actually the easiest option. This way it's disabled by default in both editor and game (which solves #46623 (comment)), and then users can set things individually:
-show_collision=false: always disabled in both game and editor
-show_collision=true: always enabled in editor, controlled by "Visible Collision Shapes" in game

So the only thing that would not be possible for now is to switch collisions on and off for all tilemaps at once in the editor, but that could be handled later with an improved gizmo system.

Copy link
Member

Choose a reason for hiding this comment

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

I opened #47382 to switch it to opt-in.

<constant name="COLLISION_VISIBILITY_MODE_ALWAYS_HIDE" value="1" enum="CollisionVisibilityMode">
Always Hide collision shapes in editor and game.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Always Hide collision shapes in editor and game.
Always hide collision shapes in editor and game.

</constant>
<constant name="COLLISION_VISIBILITY_MODE_USE_DEBUG_SETTING" value="2" enum="CollisionVisibilityMode">
Use [b]Visible Collision Shapes[/b] in debug menu to control visibility. Only works in game.
</constant>
</constants>
</class>
24 changes: 16 additions & 8 deletions scene/2d/tile_map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,11 @@ void TileMap::update_dirty_quadrants() {
Color debug_collision_color;
Color debug_navigation_color;

bool debug_shapes = show_collision && (Engine::get_singleton()->is_editor_hint() || (st && st->is_debugging_collisions_hint()));
bool debug_shapes = false;

#ifdef DEBUG_ENABLED
debug_shapes = (collision_visibility == COLLISION_VISIBILITY_MODE_ALWAYS_SHOW) || (collision_visibility == COLLISION_VISIBILITY_MODE_USE_DEBUG_SETTING && (st && st->is_debugging_collisions_hint()));
#endif

if (debug_shapes) {
debug_collision_color = st->get_debug_collisions_color();
Expand Down Expand Up @@ -1793,13 +1797,13 @@ String TileMap::get_configuration_warning() const {
return warning;
}

void TileMap::set_show_collision(bool p_value) {
show_collision = p_value;
void TileMap::set_collision_visibility_mode(CollisionVisibilityMode p_value) {
collision_visibility = p_value;
_recreate_quadrants();
}

bool TileMap::is_show_collision_enabled() const {
return show_collision;
TileMap::CollisionVisibilityMode TileMap::get_collision_visibility_mode() const {
return collision_visibility;
}

void TileMap::_bind_methods() {
Expand Down Expand Up @@ -1837,8 +1841,8 @@ void TileMap::_bind_methods() {
ClassDB::bind_method(D_METHOD("set_compatibility_mode", "enable"), &TileMap::set_compatibility_mode);
ClassDB::bind_method(D_METHOD("is_compatibility_mode_enabled"), &TileMap::is_compatibility_mode_enabled);

ClassDB::bind_method(D_METHOD("set_show_collision", "enable"), &TileMap::set_show_collision);
ClassDB::bind_method(D_METHOD("is_show_collision_enabled"), &TileMap::is_show_collision_enabled);
ClassDB::bind_method(D_METHOD("set_collision_visibility_mode", "mode"), &TileMap::set_collision_visibility_mode);
ClassDB::bind_method(D_METHOD("get_collision_visibility_mode"), &TileMap::get_collision_visibility_mode);

ClassDB::bind_method(D_METHOD("set_centered_textures", "enable"), &TileMap::set_centered_textures);
ClassDB::bind_method(D_METHOD("is_centered_textures_enabled"), &TileMap::is_centered_textures_enabled);
Expand Down Expand Up @@ -1911,7 +1915,7 @@ void TileMap::_bind_methods() {
ADD_PROPERTY(PropertyInfo(Variant::INT, "cell_half_offset", PROPERTY_HINT_ENUM, "Offset X,Offset Y,Disabled,Offset Negative X,Offset Negative Y"), "set_half_offset", "get_half_offset");
ADD_PROPERTY(PropertyInfo(Variant::INT, "cell_tile_origin", PROPERTY_HINT_ENUM, "Top Left,Center,Bottom Left"), "set_tile_origin", "get_tile_origin");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "cell_y_sort"), "set_y_sort_mode", "is_y_sort_mode_enabled");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "show_collision"), "set_show_collision", "is_show_collision_enabled");
ADD_PROPERTY(PropertyInfo(Variant::INT, "collision_visibility", PROPERTY_HINT_ENUM, "Always Show, Always Hide, Use debug settings"), "set_collision_visibility_mode", "get_collision_visibility_mode");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "compatibility_mode"), "set_compatibility_mode", "is_compatibility_mode_enabled");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "centered_textures"), "set_centered_textures", "is_centered_textures_enabled");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "cell_clip_uv"), "set_clip_uv", "get_clip_uv");
Expand Down Expand Up @@ -1946,6 +1950,10 @@ void TileMap::_bind_methods() {
BIND_ENUM_CONSTANT(TILE_ORIGIN_TOP_LEFT);
BIND_ENUM_CONSTANT(TILE_ORIGIN_CENTER);
BIND_ENUM_CONSTANT(TILE_ORIGIN_BOTTOM_LEFT);

BIND_ENUM_CONSTANT(COLLISION_VISIBILITY_MODE_ALWAYS_SHOW);
BIND_ENUM_CONSTANT(COLLISION_VISIBILITY_MODE_ALWAYS_HIDE);
BIND_ENUM_CONSTANT(COLLISION_VISIBILITY_MODE_USE_DEBUG_SETTING);
}

void TileMap::_changed_callback(Object *p_changed, const char *p_prop) {
Expand Down
13 changes: 10 additions & 3 deletions scene/2d/tile_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ class TileMap : public Node2D {
MODE_CUSTOM
};

enum CollisionVisibilityMode {
COLLISION_VISIBILITY_MODE_ALWAYS_SHOW,
COLLISION_VISIBILITY_MODE_ALWAYS_HIDE,
COLLISION_VISIBILITY_MODE_USE_DEBUG_SETTING
};

enum HalfOffset {
HALF_OFFSET_X,
HALF_OFFSET_Y,
Expand Down Expand Up @@ -80,7 +86,7 @@ class TileMap : public Node2D {
CollisionObject2D *collision_parent;
bool use_kinematic;
Navigation2D *navigation;
bool show_collision = true;
CollisionVisibilityMode collision_visibility = COLLISION_VISIBILITY_MODE_USE_DEBUG_SETTING;

union PosKey {

Expand Down Expand Up @@ -278,8 +284,8 @@ class TileMap : public Node2D {

void update_dirty_quadrants();

void set_show_collision(bool p_value);
bool is_show_collision_enabled() const;
void set_collision_visibility_mode(CollisionVisibilityMode p_value);
CollisionVisibilityMode get_collision_visibility_mode() const;

void set_collision_layer(uint32_t p_layer);
uint32_t get_collision_layer() const;
Expand Down Expand Up @@ -360,5 +366,6 @@ class TileMap : public Node2D {
VARIANT_ENUM_CAST(TileMap::Mode);
VARIANT_ENUM_CAST(TileMap::HalfOffset);
VARIANT_ENUM_CAST(TileMap::TileOrigin);
VARIANT_ENUM_CAST(TileMap::CollisionVisibilityMode);

#endif // TILE_MAP_H