Physics 2D and 3D layer/mask in tscn file can include layers above 20

[winston-yallow]

Godot versions:
Godot 3.2.3 mono
Godot 3.2.3
Godot 3.2.4 RC1

OS/device including version:
Windows 10
Solus 4 x86_64 Linux (Kernel 5.10.7-168.current)

Issue description:
The value for a collision layer/mask in the tscn file can be above 1048575 (which would be all twenty bits set to 1). This was discovered while I helped someone to debug their project. It is unclear how these values ended up in the tscn files in the first place. The project where the issue was discovered was made in a Godot 3.2.3 mono version. No C# was used in the project itself. The error persisted when loading the already corrupted tscn files in 3.2.3 and 3.2.4 RC1.

Godot will load these files just fine. However, there are collision bits set that will not be shown in the editor.

Example:

• StaticBody with bits for layer 1 and 21 set
• RigidBody with bits for layer 2 and 21 set

The editor will only show layer 1 for the StaticBody and layer 2 for the RigidBody, suggesting that they won't collide. However, the bit for layer 21 is set in both and therefore they will collide in-game.

This happens in 2D and 3D.

Expected Behaviour/Solution:
I see a few possible improvements over the current behaviour:

• Show a warning when loading files that set an invisible bit but keep it
• Show a warning directly on the PhysicsBody if an invisible bit is set but keep it
• Make it impossible to set those bit's in the first place, showing a warning when importing a file that does

Also there is no way to unset invisible bits currently. If one of the first two solutions is preferred, then there should be UI to unset these bits without editing the tscn file manually.

For me personally the third suggestion sounds best.

Steps to reproduce:
It is unclear how these values ended up in the tscn files of the project I helped to debug. For the minimal reproduction project I simply edited the tscn files per hand as I was not able to find the underlying bug in the editor that allows creating such files in the first place. This probably needs some further investigation.

Minimal reproduction project:
The minimal project includes two scenes: one for 2D, one for 3D. Both have the setup as mentioned in the example above. You can notice in-editor how only layer 1 / 2 are shown for the bodies, suggesting they won't collide. In-game the rigid body will stop at the static body.
bug.zip

[Calinou]

Related to #41281. This should be fixed in 3.2.4 betas already.

[winston-yallow]

If I correctly interpret the fix for the linked issue then it will prevent invalid values from being written in the first place. Therefore I would like to focus this issue on the part of loading: There will be more projects out there with already corrupted tscn files. The editor should provide some way of warning the user as mentioned in the issue.

[pouleyKetchoupp]

Some users might need more than 20 bits for their collision layers, since apart for the mentioned bug it's possible to set up to 32 by script. So preventing these extra bits to be set or displaying warnings (which would be false positive in some cases) don't seem viable.

We could just allow up to 32 bits to be shown in the inspector so these bits wouldn't be invisible anymore. Maybe with an extend button if showing 32 squares by default makes it too crowded.

[winston-yallow]

So preventing these extra bits to be set or displaying warnings (which would be false positive in some cases) don't seem viable.

I would disagree about the false positive. The warning/info would be useful for people that do use up to 32 layers too. Maybe the word "warning" was badly chosen by me, what I meant was some kind of indicator to communicate to the user "hey, you have some layers that won't show up here". This is also useful for people that do use those extra bits, as it prevents people from forgetting about them.

That being said, I think I would prefer your solution of simply having an editor for the whole 32 bits if this is possible in a not-too-crowded way.

[winston-yallow]

Also, as I mentioned in the a comment, there will be people that have those layers set by accident due to #41281. So some form of indicator would help people that otherwise don't know that there can be "invisible" layers set.

[pouleyKetchoupp]

There's already a reset icon (↺) showing up when extra bits are set. After also adding a way to actually see which layers are set among the 32 bits in the editor, it should be enough information.

[jitspoe]

I'm not seeing a reset icon. Was it added after 3.2.3?

Also, physics objects with this mystery bit set collide with objects that do NOT have this set, so there's some incorrect behavior here outside of the bit being incorrectly set and not visible in the editor. Seems having this bit set might behave the same as -1 (collide with everything). Why are only 20 layers visible in the editor if 32 are usable?

Took many hours for me to track down what this issue was. I wasn't able to reproduce it setting all the same data (as visible from the editor) and finally had to use my actual project and delete stuff bit by bit to create a minimal repro:
test_collision_filter_broken.zip
Then finally looked at the tscn file to figure out why 2 things with exactly the same properties behaved differently.

So something with
collision_layer = 2
collision_mask = 2147483648

Collides with collision_layer 1

I don't know that it's a good idea to be able to set data that you can't see in the editor. Sure, you could set stuff in code at run time for custom flags you don't want to be mucked about with in data, but that doesn't get saved to the scene.

[Calinou]

Why are only 20 layers visible in the editor if 32 are usable?

This was probably done because "20 layers ought to be enough for everybody" 😉

We can expose 32 layers in both 3.x and 4.0, but this will make the UI more cluttered. We can't display too many squares on a single row, so we'll probably need 4 rows of layer checkboxes in a 8×4 layout.

[pouleyKetchoupp]

@Calinou

We can expose 32 layers in both 3.x and 4.0, but this will make the UI more cluttered. We can't display too many squares on a single row, so we'll probably need 4 rows of layer checkboxes in a 8×4 layout.

It would be probably fine with a little arrow icon to extend to the full grid of 32 layers, so users can at least see there's more data to show.

Maybe 8 x 2 by default (so 16 layers instead of 20), and 8 x 4 when extended.

@jitspoe

I'm not seeing a reset icon. Was it added after 3.2.3?

I can see the reset icons with 3.2.3 on most objects from your repro project:

I'm not sure why you're not seeing it in your case. It should be always showing whenever bits are not set to the default, it's the same reset button that shows on any modified property. It wouldn't show on an instantiated scene though, because in that case it would compare to the parent scene.

[Calinou]

It wouldn't show on an instantiated scene though, because in that case it would compare to the parent scene.

I wonder if the Reset icon should have a different color when comparing against a parent scene (compared to the default value). Right now, they look identical, which can be confusing.

For instanced scene properties, we could also have an icon that means "modified from the default value, but identical to parent scene". This icon wouldn't be clickable, though.

[Zireael07]

8x2 extending to 8x4 is a very good UI/UX idea imho

[Zaraka]

This also happened to me in 3.2.3
In one of the commits collision mask changed from
0b110000001001
to
0b10000000000000000000110000001001
godot editor showed "correct" mask and I spend long time reseaching that my collision mask got corrupted and collision do not work as they should.
Godot 3.3 didn't actually fix it until I click on the reset button and enter collision_mask again

[winston-yallow]

I would really like to advocate for any fix. Just the "revert" button is not enough of a indication that something is wrong. It will show for any modified layer setup, not only for ones with the mystery bit set. So it is not an indicator for "here is something wrong".

I don't know how hard it would be to just use the 8x4 UI, because that would be the ideal solution in my opinion. If that is not easy to do as a quick fix, maybe we can just have a plus icon indicating there are more layers set than visible?

This happened to me again when I debugged a scene from someone else despite me knowing of this possibility. It is just so easy to forget without any visual indication.

[Zaraka]

I would go further and say every time a collision_mask or collision_layer is being saved it should be converted to an exactly supported format and all excess bits should be discarded

[winston-yallow]

@Zaraka that was discussed above: The engine does support 32 layers and some projects do depend on this. Layers above 20 are an "exactly supported format" by the engine. Removing those layers would break projects. The real issue is that not all layers that are supported by the engine are shown in the editor. So it needs a twek on the UI side of things.

[winston-yallow]

closing as we now have 32 layers available in the editor (see #51039 and #51040)