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

Use zero-indexing for physics and render layer names #44809

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Dec 30, 2020

The first layer is now Layer 0 instead of Layer 1, and the last layer is now Layer 19 instead of Layer 20.

This helps reference physics and render layers from scripts since layers start from 0 there.

This breaks compatibility in a subtle way since layer name project settings will need to be modified. Also, this might confuse users during an upgrade so I'd suggest merging this only in master.

This closes godotengine/godot-proposals#2050.

The first layer is now Layer 0 instead of Layer 1, and the last
layer is now Layer 19 instead of Layer 20.

This helps reference physics and render layers from scripts since
layers start from 0 there.
@Calinou Calinou force-pushed the physics-render-layers-zero-index branch from 13f2f78 to f87d42f Compare December 30, 2020 15:03
Comment on lines +939 to +942
GLOBAL_DEF(vformat("layer_names/2d_render/layer_%d", i), "");
GLOBAL_DEF(vformat("layer_names/2d_physics/layer_%d", i), "");
GLOBAL_DEF(vformat("layer_names/3d_render/layer_%d", i), "");
GLOBAL_DEF(vformat("layer_names/3d_physics/layer_%d", i), "");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can use 2-digits padding so that things have the proper order in the docs? i.e. layer_00, layer_01, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively we could update the docs logic to use natural number comparison instead of lexographic.

Copy link
Member Author

@Calinou Calinou Jan 1, 2021

Choose a reason for hiding this comment

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

I just checked, both the class reference and property editors already seem to use natural sorting:

Class reference

Project Settings

Copy link
Member

Choose a reason for hiding this comment

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

Looks good. We should just fix the XML generator then so that it also looks good for doc writers (but that's not something that needs to be done in this PR).

@akien-mga akien-mga merged commit 96aff74 into godotengine:master Jan 1, 2021
@akien-mga
Copy link
Member

Thanks!

@putskan
Copy link

putskan commented Jan 2, 2021

You guys are great, thanks!

@Calinou Calinou deleted the physics-render-layers-zero-index branch March 31, 2021 09:42
@pouleyKetchoupp
Copy link
Contributor

I agree it's better to keep things consistent, but layer numbers are now hard to identify in the inspector and project settings (0-based numbered entries are not very intuitive when not in programming context).

This is even more obvious in the new layer grid I've just implemented:
#51039

3.x (intuitive numbers):
layer_grid_render

4.0 (hard to understand how blocks are arranged):
layer_grid_render_4 0

There could be a better way to solve this discrepancy that would be more intuitive and more friendly to non-programmer users.

I would rather rename the script functions to be more intuitive and make it 1-based, something like:

bool get_collision_layer_value ( int layer_number ) const
Returns an individual value on the collision_layer, given a layer number between 1 and 32.

void set_collision_layer_value ( int layer_number, bool value )
Sets an individual value on the collision_layer bitmask, given a layer number between 1 and 32. Use this if you only need to change one layer's value.

That would instead make the script side a bit more intuitive and user-friendly.

@Calinou
Copy link
Member Author

Calinou commented Jul 30, 2021

I would rather rename the script functions to be more intuitive and make it 1-based, something like:

Maybe, but we should make sure to print an explicit error message stating that layers are 1-indexed if the layer number argument is 0. This can quickly end up being confusing if you're using range() to set layers/masks.

@pouleyKetchoupp
Copy link
Contributor

@Calinou Yes, it would definitely help to raise errors on layer 0.

Actually, in order to make things very clear in scripts, we could even have two versions, one using layer bit, one using layer number. These methods are trivial so it's not too much to maintain, and with proper documentation it could avoid confusion either way.

Editor UI could then switch back to using 1-based layers to keep things intuitive (for 20 layers, first layer -> Layer 1, last layer -> Layer 20).

Would that work for you?

@Calinou
Copy link
Member Author

Calinou commented Jul 30, 2021

Actually, in order to make things very clear in scripts, we could even have two versions, one using layer bit, one using layer number. These methods are trivial so it's not too much to maintain, and with proper documentation it could avoid confusion either way.

I'm not sure about having two different methods, since this would introduce choice paralysis for people (and confusion when people ask questions with approximative wording).

I'd just go back to 1-indexing in the single method we have right now, and add a dedicated error message when the layer 0 is attempted to be set/get.

@pouleyKetchoupp
Copy link
Contributor

Alright, that makes sense!

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.

Clarify Collision Bits Access
4 participants