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

Added show_collision property for tilemap node. #46623

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

Janglee123
Copy link
Contributor

If true, collision shapes are shown in the editor and at run-time. Requires Visible Collision Shapes to be enabled in the Debug menu, for collision shapes to be visible at run-time.

NOTE: The pr is opened against 3.2 and not master. The feature will probably be included in master by #45278.

scene/2d/tile_map.h Outdated Show resolved Hide resolved
@pouleyKetchoupp pouleyKetchoupp requested a review from a team March 3, 2021 17:25
If true, collision shapes are shown in the editor and at run-time.
Requires Visible Collision Shapes to be enabled in the Debug menu,
for collision shapes to be visible at run-time.
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!

@akien-mga akien-mga merged commit cad3771 into godotengine:3.2 Mar 4, 2021
@akien-mga
Copy link
Member

Thanks!

@ChainedLupine
Copy link
Contributor

ChainedLupine commented Mar 18, 2021

Can we revisit this? If you use a lot of TileMaps (like I do), the fact that it shows collision shapes by default now (regardless of the Editor->Debug/Visible Collision Shapes menu setting) causes a massive frame rate drop in the Editor if you have a lot of TileMaps.

The fact I have to disable this, per TileMap, makes this change particularly annoying. I'd rather it still be an Editor-related global toggle (as Debug/Visible Collision Shapes checkmark currently is) and not something I have to exclusively turn off PER TileMap.

@Janglee123
Copy link
Contributor Author

Using lots of tilemap seems a specific case IMO. But if want a global option to disable/enable the show_collision shape, you can create a plugin that updates all tilemaps in the scene. Or if you have a few tilemaps you can select each by ctrl+click and then change the option in inspector dock for all. :)

@groud
Copy link
Member

groud commented Mar 18, 2021

I kind of agree it's annoying to have to disable TileMap one by one when you have to debug collision.
When you want to debug collisions, you should not have to go through all tilemaps (remember we use TileMaps as tilemap layers, by design this means somone may have several tilemaps) to display collision shapes. It's annoying and kind of defeats the purpose of a single "show debug collision shape" option.

In my opinion, we should go with smarter solution. Likely, for each TileMap, an option with 3 value possible. Namely: "Use debug setting", "Force show" and "Force Hide". This would keep the possibility change TileMap's one by one, but would keep the debug setting useful.

@pouleyKetchoupp
Copy link
Contributor

This drop-down solution seems like a good compromise. It's a bit different from the way CollisionShape uses "Visible Collision Shapes" debug setting (affects game only), but it makes sense in the case of tilemaps.

@ChainedLupine
Copy link
Contributor

Using lots of tilemap seems a specific case IMO. But if want a global option to disable/enable the show_collision shape, you can create a plugin that updates all tilemaps in the scene. Or if you have a few tilemaps you can select each by ctrl+click and then change the option in inspector dock for all. :)

I cannot do this because my TileMaps are heavily isolated in independent scenes. On average, I have 4-6 TileMaps per level object, and in my level there will be 10-20 level objects. Worse yet, the TileMaps are generated by a Godot plugin and therefore any changes made in the Editor are temporary. I could incorporate setting "collision_show=false" to the plugin, but then I have a dependency on 3.3.x going forward.

@pouleyKetchoupp
Copy link
Contributor

@Janglee123 Let me know if you would like to implement the show collision modes yourself, otherwise I'll make a PR on Monday.

@Janglee123
Copy link
Contributor Author

@pouleyKetchoupp no worries I will do this weekend

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.

6 participants