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 documentation for GDScript warnings #10627

Open
Meorge opened this issue Feb 5, 2025 · 3 comments · May be fixed by #10635
Open

Add documentation for GDScript warnings #10627

Meorge opened this issue Feb 5, 2025 · 3 comments · May be fixed by #10635
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features enhancement topic:gdscript

Comments

@Meorge
Copy link

Meorge commented Feb 5, 2025

Your Godot version: 4.4

Issue description:

Related to #368 .

There is currently no documentation for GDScript warnings in the Godot manual. I believe it may be helpful to create pages describing each warning in more detail, along with examples of code that causes them and an explanation of how to fix them.

Microsoft's official C# documentation has pages describing exceptions that can be thrown, with similar sections: https://learn.microsoft.com/en-us/dotnet/api/system.nullreferenceexception?view=net-9.0

For reference, here is my shot at writing a page for the GET_NODE_DEFAULT_WITHOUT_ONREADY warning:

GET_NODE_DEFAULT_WITHOUT_ONREADY

The warning that appears when a node's default value is set to a location in the scene tree without using the @onready annotation.
The message will say something like

The default value is using "$" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.

When this warning occurs

In GDScript, instance variables can be set by declaring them outside of a method. Additionally, they can be given a default value using =:

extends Area2D
var my_num = 3

This way, the variable my_num will always start out with a value of 3 in each instance of this class.
GDScript also has methods to retrieve specific nodes from the scene tree: namely, the get_node method, and its shorthand versions $ (and % for unique nodes). Thus, if you want to have an instance variable default to a child of the node with a script, it may be tempting to write something like the following:

extends Area2D
var my_collision_shape = $CollisionShape2D

However, class instance variables' default values are evaluated and assigned before the scene tree is set up. This means that at the time of assigning the default value, the node may not be in the scene tree, and the variable will be set to null instead.

How to fix this warning

The most straightforward solution is to add the @onready annotation before your variable declaration:

extends Area2D
@onready var my_collision_shape = $CollisionShape2D

Now, the default value of the variable will not be assigned until the scene tree has been initialized, and the target node will be present.

Alternatively, you can set the value of the variable at the beginning of your _ready method:

extends Area2D
var my_collision_shape

func _ready():
    my_collision_shape = $CollisionShape2D

While I tried to keep everything accurate to the best of my knowledge, there might be some inaccuracies in this write-up. Of course, if this proposal were to move forward, we would want the content of the pages to be reviewed by multiple people before going live.
Hopefully, though, this example conveys the spirit of what I'm proposing.

The editor could potentially even provide a link to the corresponding page when a user encounters a particular warning, so that the user can read more about it.

@tetrapod00 tetrapod00 added content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features topic:gdscript area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Feb 5, 2025
@tetrapod00
Copy link
Contributor

To me this makes sense to implement more or less as is. Some thoughts:

  • All the individual warning pages should be under an index page called GDScript warnings, placed after GDScript warning system in the table of contents. See the C# diagnostics page for reference. It may be the case that we want to combine the index page and the existing GDScript warning system page, but I would let that discussion happen in the PR review.
  • Since each warning will need some review for the "when" and "how to fix", but not so much for the basic "what", we can implement this in stages:
    • Initial PR approving the overall structure of warning pages and including one example implementation, along with stub pages for all other warnings
    • Followup PRs to implement the full pages for each other warning, which may be distributed to other contributors if you personally don't have time for all of them.

To move forward I would recommend making that initial PR that implements the single warning page you've already written, the index page for the warnings overall, and possibly a stub page for each other warning (but maybe even that should wait). Then we can discuss the exact scope and structure in the PR.

@Meorge
Copy link
Author

Meorge commented Feb 6, 2025

Thanks, I'm glad you like it!

I can certainly get started with the example implementation I wrote up here, and the index page to act as their parent. Would the PR with just those make sense to merge, or would it be better to keep it as a draft until we could get at least initial "what" pages ready for each warning? I'd be a little unsure about having one written-up page and then empty pages for the rest of the warnings, but if you and/or others think that's best, then I'd be fine with it.

@tetrapod00
Copy link
Contributor

tetrapod00 commented Feb 6, 2025

Either way we need to validate and review the overall structure of the page, how detailed it is, the names of the section headers, etc. So at a minimum it makes sense for the first PR to include the index and a single full example. We should review that before anything else.

After that, I'm not sure if it makes the most sense to:

  • Add full versions of all warnings in a single large PR, all at once. This is best from a user perspective but it is probably too much to write at once, and might get stuck in review.
  • Add a short stub page for each warning initially, then write each page separately in a subsequent PRs.
  • Add only a single warning page initially, then write each page separately in a subsequent PRs.

Personally I'd go with option 2 or 3 here, leaning towards 2, but other maintainers might have different opinions about which is better between those options.

I'd recommend making the initial PR with just the index and a single page, mark it as "ready for review" once it's something that could go in the manual (the version in the OP here is fine to start with, I think), and we can go from there.

@Meorge Meorge linked a pull request Feb 7, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features enhancement topic:gdscript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants