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

Add C# Editor Documentation #40631

Draft
wants to merge 1 commit into
base: master
from
Draft

Conversation

@HaSa1002
Copy link
Contributor

@HaSa1002 HaSa1002 commented Jul 23, 2020

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

// C#
bool first_is_underscore = p_name[0] == '_';
String result = p_name.capitalize();
return (first_is_underscore ? "_" : "") + result.replace(" ", "");

This comment has been minimized.

@aaronfranke

aaronfranke Jul 23, 2020
Member

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

This comment has been minimized.

@Calinou

Calinou Jul 24, 2020
Member

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.

This comment has been minimized.

@HaSa1002

HaSa1002 Jul 24, 2020
Author Contributor

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 Geequlim commented Jul 24, 2020

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

@HaSa1002
Copy link
Contributor Author

@HaSa1002 HaSa1002 commented Jul 24, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants