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

Improve GDScript documentation generation & behavior #72095

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

anvilfolk
Copy link
Contributor

@anvilfolk anvilfolk commented Jan 26, 2023

Rework of GDScript classes' documentation generation (docgen).

Changes:

• Removes docgen code & datastructures from gdscript_compiler.cpp and gdscript.cpp
• Docgen uses analyzer datatypes instead of less expressive runtime datatypes
• Enums return/parameter types docgen'd properly instead of being Variant or int. Fixes #71117
• Makes hyperlinks to unopened classes generate their documentation on-demand rather than show blank screen. Fixes #72406
• Signals parameter types are docgen'd correctly instead of being Variant
• Added same-line docstrings for variables/constants
• Show external class specifiers e.g. TileSet.CellNeighbor instead of CellNeighbor, OtherClass.Enum or InnerClass.Enum instead of just Enum
⚠️ ⚠️ ⚠️ Remove unnecessary class specifier path, e.g. InnerClass.Enum instead of CurrentClass.InnerClass.Enum or Inner3.Enum instead of Class.Inner1.Inner2.Inner3.Enum, when documented inside Inner2⚠️ ⚠️ ⚠️ this one might be slightly contentious ⚠️ ⚠️ ⚠️
• Created a fairly comprehensive set of gdscript files with all/most combinations of types to make sure this works properly (linked below)

Minor changes:

• Removes unused p_scroll parameter in EditorHelp::go_to_class() and p_vscr in EditorHelp::_goto_desc()
• Removed unused DocData::EnumDoc
• Normalize docstrings in EditorHelp source code

Enums and class specifiers everywhere!

Before (boooooo):
image

After (yaaaaaay):
image

Inline docstrings!

image

Possibly contentious class specifier prefix elimination!

image

(see below for gdscript class & enum types)

Notice how we are within Doc1.IC, and therefore in the highlighted areas only relevant class specifier prefixes are shown. We don't need Doc1.IC, just IC. We don't need Doc1.IC.IE or IC.IE, just IE. We don't need Doc1.IC.IIC or Doc1.IC.IIC.IIE, just IIC or IIC.IIE.

Test files!

gdscript_docgen.zip

These are the files I used to test. They are organized as follows:

  1. Two outer classes, Doc1 and Doc2
  2. Doc1 and Doc2 have an inner class IC, which has an inner inner class IIC.
  3. Each class, outer or inner, has corresponding enums, E, IE, and IIE.
  4. Doc1 and inner classes, have methods containing all combinations of accessing classes and enums I could think of, both as parameters and return types.

As far as I can tell, every type name is as I thought it made the most sense (i.e. remove prefixes which are "obvious"), and click-linking works also.

@akien-mga akien-mga added this to the 4.0 milestone Jan 26, 2023
@anvilfolk anvilfolk force-pushed the gd-docs branch 3 times, most recently from 21b1990 to decf8cf Compare February 7, 2023 03:45
@aaronfranke aaronfranke modified the milestones: 4.0, 4.1 Feb 9, 2023
@anvilfolk anvilfolk force-pushed the gd-docs branch 2 times, most recently from 4fede92 to a6d487d Compare March 15, 2023 13:56
@anvilfolk anvilfolk force-pushed the gd-docs branch 4 times, most recently from 6f7dd77 to 5fb295c Compare March 22, 2023 18:01
@anvilfolk
Copy link
Contributor Author

Also, for the folks following, I updated the original post with all the things happening in this PR :)

Let me know if there are any other wants that I could include here!

@anvilfolk anvilfolk changed the title Refactor GDScript documentation generation out of compiler, make it better Improve GDScript documentation generation & behavior Mar 25, 2023
@anvilfolk anvilfolk force-pushed the gd-docs branch 7 times, most recently from 712be4c to fb759c2 Compare April 3, 2023 22:58
@anvilfolk anvilfolk marked this pull request as ready for review April 3, 2023 23:00
@anvilfolk anvilfolk requested review from a team as code owners April 3, 2023 23:00
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

The editor side of things and the overall approach make sense to me. Needs a review from @godotengine/gdscript still.

Removes documentation generation (docgen) from the GDScript compiler to
its own file. Adds support for GDScript enums and signal parameters and
quite a few other assorted fixes and improvements.
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Reviewed during a PR meeting of the GDScript team.

Great job ocean, you did a really good job. It was a long time coming.

@vnen said that he would review the PR more in depth, but is ok with it at glance.

Comment on lines +87 to +88
// ResourceLoader is used in order to have a script-agnostic way to load scripts.
// This forces GDScript to compile the code, which is unnecessary for docgen, but it's a good compromise right now.
Copy link
Member

Choose a reason for hiding this comment

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

This makes me think that perhaps the ScriptServer should be aware of documentation and just as the script language to provide documentation data. This would allow not only GDScript and other languages to optimize the doc gen process.

Of course this is just something to discuss later, no need to worry about it for this PR. I agree that loading the resource is a good compromise for now.

Copy link
Member

@vnen vnen 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. Thanks for working on this!

@YuriSizov YuriSizov merged commit 26fb911 into godotengine:master Apr 26, 2023
@YuriSizov
Copy link
Contributor

This is amazing! Thanks a lot!

@anvilfolk anvilfolk deleted the gd-docs branch April 26, 2023 15:36
@anvilfolk
Copy link
Contributor Author

Thanks everyone for your time, comments and support 😄 ❤️

@atirut-w
Copy link
Contributor

Not sure if this is relevant, but I'd like the LSP to be integrated with docgen so we can get consistent docgen behavior.

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