godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix false positive enum warnings when `BitField<T>` is used

Open dalexeev opened this issue 2 years ago • 5 comments

  • Treat BitField<T> as an int rather than an enum T in GDScript.
  • Change display of bitfields in Editor Help as BitField[T].

I highlighted the BitField with a different color and added a hint, since BitField is not a type in GDScript.

Closes #74593.

dalexeev avatar Mar 09 '23 07:03 dalexeev

Looks fairly good to me, though I'm a bit concerned about introducing the BitField[Enum] syntax which might be confusing, especially since it's not supported in GDScript.

For example, how would users know which type is actually returned by button_mask in GDScript? Is it MouseButtonMask, is it some BitField[] that would raise a syntax error if they try to use it as type hint, or is it a plain int?

akien-mga avatar Mar 09 '23 08:03 akien-mga

For example, how would users know which type is actually returned by button_mask in GDScript? Is it MouseButtonMask, is it some BitField[] that would raise a syntax error if they try to use it as type hint, or is it a plain int?

Now it is plain int, not SomeEnum.

I'm a bit concerned about introducing the BitField[Enum] syntax which might be confusing

Yes, I'm a little worried about this too, so I used a different color (same as void) and added a tooltip. A little confusion is unavoidable, but the current state is already confusing. ENUM_VALUE_A | ENUM_VALUE_B is int in GDScript. And the SomeEnum type hint raises a warning when an int is passed to it.

dalexeev avatar Mar 09 '23 09:03 dalexeev

It might look a bit heavy, but I wonder if we shouldn't still specify int as return type + additional info.

Like int (BitField[MouseButtonMask]), or maybe something less prone to confusion with typed array syntax, like int (MouseButtonMask bitmask).

Either option makes it a pretty long type hint and might lead to awkward formatting in the docs, but it might still be worth it to be explicit.

akien-mga avatar Mar 09 '23 09:03 akien-mga

Personally, I like BitField[T] better as it is more compact and this is what this feature could look like in GDScript if we implement it. It's just replacing <> with [], as with typed arrays. Also, the API documentation is not required to be valid GDScript.

As for possible confusion, I think it is a temporary problem when learning the engine, and cluttered documentation is inconvenient for all users. It seems to me that a visual difference and a clear tooltip will be enough.

But this is more a matter of personal preference, I do not insist. Let's see if there are other opinions on this.

dalexeev avatar Mar 09 '23 12:03 dalexeev

Online docs uses C++ style:

Images

We probably need to make this consistent with built-in help (in a separate PR).

dalexeev avatar Mar 09 '23 17:03 dalexeev

Rebased. Fixed bug with using BIND_ENUM_CONSTANT instead of BIND_BITFIELD_FLAG (now the error can be found by make_rst.py in static checks).

https://github.com/godotengine/godot/blob/28cca66d2cfe25d6d7bf5a2a26ab7bd366029669/scene/main/node.h#L709

https://github.com/godotengine/godot/blob/28cca66d2cfe25d6d7bf5a2a26ab7bd366029669/scene/main/node.cpp#L49-L52

Is there any reason why some VARIANT_*_CAST are in *.cpp while most are in *.h?

VARIANT_(ENUM|BITFIELD)_CAST in *.cpp - 11 matches

core/io/ip.cpp: 1
 38:  1: VARIANT_ENUM_CAST(IP::ResolverStatus);
core/io/xml_parser.cpp: 1
 37:  1: VARIANT_ENUM_CAST(XMLParser::NodeType);
core/os/time.cpp: 2
 55:  1: VARIANT_ENUM_CAST(Month);
 56:  1: VARIANT_ENUM_CAST(Weekday);
scene/2d/line_2d.cpp: 3
 38:  1: VARIANT_ENUM_CAST(Line2D::LineJointMode)
 39:  1: VARIANT_ENUM_CAST(Line2D::LineCapMode)
 40:  1: VARIANT_ENUM_CAST(Line2D::LineTextureMode)
scene/main/node.cpp: 4
 49:  1: VARIANT_ENUM_CAST(Node::ProcessMode);
 50:  1: VARIANT_ENUM_CAST(Node::ProcessThreadGroup);
 51:  1: VARIANT_BITFIELD_CAST(Node::ProcessThreadMessages);
 52:  1: VARIANT_ENUM_CAST(Node::InternalMode);

VARIANT_(ENUM|BITFIELD)_CAST in *.h - 625 matches

dalexeev avatar May 28 '23 12:05 dalexeev

Is there any reason why some VARIANT_*_CAST are in *.cpp while most are in *.h?

Moved to .h, did not notice any increase in compilation time.

dalexeev avatar Jun 15 '23 14:06 dalexeev

Thanks!

akien-mga avatar Jun 16 '23 08:06 akien-mga