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

Provide context for every error #24199

Closed
KoBeWi opened this issue Dec 6, 2018 · 7 comments
Closed

Provide context for every error #24199

KoBeWi opened this issue Dec 6, 2018 · 7 comments

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Dec 6, 2018

Can't believe no one made such issue yet, but I did search and didn't see one. The issue is basically this:
image
Often when working in Godot, you get a seemingly random error telling you unrelated stuff and any additional details are just the line in source code (which should open GitHub source when clicked IMO). There's no context on the error and often it's not caused by any of the scripts.

The error above tells me that some image is null. Cool, but where? How do I find it? Why there is no other way than removing objects from scene one by one or blindly going through anything that might have a texture? In the above case, the error should provide some information on which node/resource causes this and in what scene.

There are lots of error like this. My "favorite" is Condition '!err' is true. How would I be even supposed to know what is this if it wasn't related to something I just did?

Some other related issues with weird errors, as an example:
#22565
#24174
#24311

IMO there should be more debug information to make pinpointing any error easier.

@MelvinWM
Copy link
Contributor

This might take more effort and work than one might anticipate. For instance, there seems to be about 5200 occurrences of the ERR_FAIL_COND and ERR_FAIL_COND_V preprocessor-macros:

$ grep -RiI "ERR_FAIL_COND(" * | wc -l
2801
$ grep -RiI "ERR_FAIL_COND_V(" * | wc -l
2424

Cherry-picking the most commonly occurring user errors and adding more context and information to them, as well as maybe providing more preprocessor-macros/other to make it easier to provide more context and information for an error (there are already a decent variety, though more might be useful, see core/error_macros.h), might be a good approach.

@bojidar-bg
Copy link
Contributor

Note that many of those macros are located in helper functions, where there is little-to-no context available as to what is currently being done.

For example, RID::get contains this little gem:

ERR_FAIL_COND_V(!id_map.has(p_rid.get_data()), NULL);

Basically, every time someone passes a rid which is unrelated to the function he is calling (for example, passing a texture RID to the physics server), he is going to get that error. And, if some C++ code manages to mess up (as was the case in #23889), you get the cryptic error.

Thus, IMO, no amount of context would help unless the context comes from the calling function somehow.

As an additional example, consider an animation which animates a Sprite's frame property. The setter for that property makes an error in case new frame is out-of-bounds:

godot/scene/2d/sprite.cpp

Lines 255 to 257 in df7d370

void Sprite::set_frame(int p_frame) {
ERR_FAIL_INDEX(p_frame, vframes * hframes);

Now, the context which a person would like to have in this case is:

  • Who is causing the frame to be set? - the AnimationPlayer
  • Which animation/keyframe is having said OOB value?
  • Which sprite node raises the error?

Some additional context which might be useful, but will be likely impossible to include:

  • Who started the animation? - hard to represent in memory in a manner which does not impact non-error cases.

Just my rather long two cents.

@MelvinWM
Copy link
Contributor

@bojidar-bg Excellent points.

Reg. adding more context, the runtime cost for non-error cases is important to avoid, and the overhead of added code may make it overall not worth it adding context for many error checking cases.

One idea I wonder about is whether it might make sense to have (debug-mode-only?) context information functionality. Possibly generic functionality that is decoupled from the calling of the error checking preprocessor-macros, where the context information is somehow set or stored earlier. That way, the context can be available in some form without having to change the specific preprocessor-macro calls. There are a number of drawbacks with that, however, including the risk of a lot of irrelevant context information. In that regard, adding more context when feasible to specific calls might still make sense. There are different ways that this could be set up - logging seems the most obvious to me, with including (debug-mode-only?) logging calls at various places in the engine source code as appropriate. The user could then disable/enable logging and maybe set logging levels as well when trying to figure out errors they see in in the editor debugger error console or other (possibly erroneous) behaviour in their games (if this approach is taken, it might make sense to create some sort of documentation or (in-editor) hint to users that logging is supported and that it might be useful for figuring out at least some errors). It would not necessarily make sense to add lots of logging, at least regarding this, since Godot has a built-in debugger and other debuggers like gdb can be used as well I assume (though I have not used the built-in debugger myself yet). Logging would mostly be added to help support users quickly figuring out expected errors, like above with the image being null. That said, I am uncertain how well this approach would work in practice.

I do not have a good understanding of what kinds of logging are supported in Godot - I know some sort of logging can be enabled in "Project Settings -> Logging -> File Logging" (though I am uncertain what that involves), and there are some issues reg. logging here: #7095 and #19122 . I know about some GDScript logging plug-ins, though that does not seem applicable in this case, since that AFAICT can only be used for user-written game code.

As a side-note, in a greenfield game engine project, I would have wondered whether something like ADTs for error handling would be a meaningful approach, since that would enable bubbling up the error and thus let context be provided by callers. But that would likely have implied some runtime overhead, which is desirable to avoid in a game engine.

@Faless
Copy link
Collaborator

Faless commented Jun 24, 2019

It's worth noticing that clicking on the error in the debugger, should bring you to the GDScript line that called the function that generated the error.
See this screencast

@Zireael07
Copy link
Contributor

@Faless: The issue is old, and the change that makes clicking work is very recent ;)

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 24, 2019

Yeah, we'd probably need to reevaluate this issue, as not sure how many of the error issues are still valid. As I mentioned in #30023, it won't probably work for errors that depend on properties, not scripts. Providing a node path in case of failing node would be helpful too.

@Calinou
Copy link
Member

Calinou commented Oct 11, 2020

Closing in favor of godotengine/godot-proposals#1605, as feature proposals are now tracked in the Godot proposals repository.

Please report cryptic error messages here from now on: #42719

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

No branches or pull requests

7 participants