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

Jump to bottom

Add C# Editor Documentation #40631

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add C# Editor Documentation #40631

wants to merge 1 commit into from
+138 −44

Conversation

HaSa1002
Copy link
Contributor

The user can select, how the reference should be rendered in the editor settings.
text_editor/help/class_reference_language: GDScript
grafik

text_editor/help/class_reference_language: C#
grafik

The conversion to PascalCase is done automatically.

This is moved out of #40613

@Calinou Calinou added this to the 4.0 milestone Jul 23, 2020
aaronfranke
aaronfranke reviewed Jul 23, 2020
View reviewed changes
editor/editor_help.cpp
// C#
bool first_is_underscore = p_name[0] == '_';
String result = p_name.capitalize();
return (first_is_underscore ? "_" : "") + result.replace(" ", "");
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 should make a method on String to do this? I think it would be more efficient, we could make a method that doesn't involve string splitting and re-concatenation - we can simply capitalize every letter after _ (and at the beginning) and then afterwards do a find/replace "_" with "" (except keep one at the beginning if there was one).

Copy link
Member

@Calinou Calinou Jul 24, 2020 •
edited

Choose a reason for hiding this comment

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

I'd try to benchmark that code to see if it's an actual bottleneck first (even using good old OS::get_singleton()->get_ticks_usec()). Adding more code to the String class should be done cautiously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, but this code is not executed in a performance critical part of the engine. I can test wheter or not a iteration above the whole string is faster

@Geequlim
Copy link
Member

Geequlim commented Jul 24, 2020 •
edited

Please add C# related macro checking, this features should not exist in the engine without C# support.

@HaSa1002
Copy link
Contributor Author

There's a MODULE_MONO_ENABLED define (defined in modules/module_defines.gen.h or similar).
But more than hardcoding this in editor code, it would be better to query the available script languages drop the engine to see which are included in the build.
It could additionally be made configurable via an editor setting whether you want to see GDScript, C# (when available), or all.
was the statement by akien on the other issue. I would vote for the option of having it setable in the EditorSettings and make default if C# is existent. It's always a runtime feature and it's generated from the GDScript docs.

@neikeq
Copy link
Member

neikeq commented Jul 25, 2020

I'm not necessarily against this, but it's not the ideal way:

  • While identifiers are converted to PascalCase, there are exceptions like when there's a conflict with another name, in which additional alterations are applied (like suffixing with Enum or an underscore _).
  • This only includes Godot Object derived classes, not other classes or structs. It also doesn't include C# specific methods/properties we add to Godot Objects that are not present in ClassDB.
  • No info about namespaces either. There's a high chance some classes will be moved to a different namespace in the future, like File/Directory and Thread/Mutex/etc.
  • No .NET specific members like events.

IMO, the best would be to generate these from the information we get from the API assemblies and their generated xml doc file.

@neikeq
Copy link
Member

neikeq commented Jul 25, 2020

Also -not a blocker either as this can be done incrementally in future PRs- but it would be better if this wasn't hard-coded in EditorHelp. Instead EditorHelp should allow third-parties to inject documentation providers.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Calinou Calinou Calinou left review comments

@aaronfranke aaronfranke aaronfranke left review comments

@akien-mga akien-mga Awaiting requested review from akien-mga

@neikeq neikeq Awaiting requested review from neikeq

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
documentation enhancement needs work topic:dotnet topic:editor
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@HaSa1002 @Geequlim @neikeq @Calinou @aaronfranke @YuriSizov