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

Display BitField[Enum] in docs to distinguish from Enum #74641

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Mar 9, 2023

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

@akien-mga
Copy link
Member

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?

@dalexeev
Copy link
Member Author

dalexeev commented Mar 9, 2023

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 dalexeev force-pushed the fix-bitfield-enum-warnings branch from 76a0648 to 4e5852f Compare March 9, 2023 09:27
@dalexeev dalexeev requested a review from a team as a code owner March 9, 2023 09:27
editor/editor_help.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

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.

@dalexeev dalexeev force-pushed the fix-bitfield-enum-warnings branch from 4e5852f to 68ea7b5 Compare March 9, 2023 12:50
@dalexeev
Copy link
Member Author

dalexeev commented Mar 9, 2023

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 dalexeev force-pushed the fix-bitfield-enum-warnings branch from 68ea7b5 to aa8ab24 Compare March 9, 2023 15:05
editor/editor_help.cpp Outdated Show resolved Hide resolved
@dalexeev dalexeev force-pushed the fix-bitfield-enum-warnings branch 2 times, most recently from 160c994 to 469698b Compare March 9, 2023 16:16
@dalexeev
Copy link
Member Author

dalexeev commented Mar 9, 2023

Online docs uses C++ style:

Images




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

@dalexeev dalexeev force-pushed the fix-bitfield-enum-warnings branch from 469698b to 17783f3 Compare March 9, 2023 17:39
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Code looks good to me. I think the current style isn't too bad – we may have to iterate on it later, but it needs testing in real world conditions first 🙂

@dalexeev dalexeev force-pushed the fix-bitfield-enum-warnings branch 3 times, most recently from f197062 to 7512a76 Compare May 28, 2023 12:34
@dalexeev
Copy link
Member Author

dalexeev commented May 28, 2023

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).

VARIANT_ENUM_CAST(Node::DuplicateFlags);

VARIANT_ENUM_CAST(Node::ProcessMode);
VARIANT_ENUM_CAST(Node::ProcessThreadGroup);
VARIANT_BITFIELD_CAST(Node::ProcessThreadMessages);
VARIANT_ENUM_CAST(Node::InternalMode);

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 dalexeev force-pushed the fix-bitfield-enum-warnings branch 3 times, most recently from 2fd22bc to c10a3d6 Compare May 29, 2023 06:51
@akien-mga akien-mga requested a review from vnen May 29, 2023 07:43
Copy link
Member

@akien-mga akien-mga 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 to me. Needs a rebase.

@dalexeev dalexeev force-pushed the fix-bitfield-enum-warnings branch from c10a3d6 to eb391d3 Compare June 15, 2023 14:26
@dalexeev dalexeev requested a review from a team as a code owner June 15, 2023 14:26
@dalexeev
Copy link
Member Author

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.

@akien-mga akien-mga merged commit d101244 into godotengine:master Jun 16, 2023
@dalexeev dalexeev deleted the fix-bitfield-enum-warnings branch June 16, 2023 08:50
@akien-mga
Copy link
Member

Thanks!

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.

5 participants