Misleading "USER ERROR:" prefix in logs when using Push Error

[imvulpi]

Tested versions

• v4.3.rc3.mono.official.03afb92ef,
• v4.2.2.stable.mono.official.15073afe3,

System information

Godot v4.3.rc3.mono - Windows 10.0.19045 - Vulkan (Mobile) - dedicated NVIDIA GeForce GTX 1050 Ti (NVIDIA; 31.0.15.5123) - Intel(R) Core(TM) i5-8400 CPU @ 2.80GHz (6 Threads)

Issue description

The issue

When using the GD.PushError() in C# or push_error in GDscript the logs created in debug/file_logging/log_path will include a misleading "USER ERROR:" prefix.

Better way

To allow developers to push different types of errors the prefix should be replaced with something more universal. For example just "ERROR:" (This is actually how it appears in the godot console) or no prefix at all. The best way in my opinion would be to allow the usage of PushError without any prefix, possibility to override the prefix or allowing customization of the prefix through settings.

Why it matters?

To players the error might suggest that it's their fault, while that's not always the case. I think that the prefix is supposed to seperate game developer errors from the godot engine errors, but for a player "USER ERROR:" prefix suggests that it's their fault. Same with developers not familiar with godot.

Changing the prefix and allowing the developers to customize the prefix or use the method without it would be great for better error logging experience for developers. It would allow better control over the presentation of developer errors. Maybe godot engine errors could have their own prefix to make them distinct from developer errors.

Why PrintError() is not a replacement of PushError

While it's true that PrintError doesn't print the "USER ERROR" prefix, it's not a replacement for PushError which prints debug information such as returning type, class, method, path, and line number, additionally the PushError appears in the debugger which futher helps when developing. PrintError doesn't do any of those. The debug info is what makes the PushError very useful.

I look forward to hearing feedback from others.
Thanks.

Steps to reproduce

Create a new project.
Create a node with a script using the GD.PushError() or push_error somewhere.
Run the project
Check the debug/file_logging/log_path log file

Minimal reproduction project (MRP)

N/A

[dalexeev]

I think we could rename "USER ERROR" to "SCRIPT ERROR". And optionally "ERROR" to "ENGINE ERROR". However, the later may be incorrect, since there are custom modules, GDExtension, etc.

[imvulpi]

I think we could rename "USER ERROR" to "SCRIPT ERROR". And optionally "ERROR" to "ENGINE ERROR". However, the later may be incorrect, since there are custom modules, GDExtension, etc.

Hmm I would still prefer something more flexible, "SCRIPT ERROR" is definitely better but it still lacks the customization, I see that it would be a lot easier to change that for now.

For me the best way would be to have a way of pushing the error without any prefix, but I get that for others that push errors without formatting it would be a downside.

How about just "ERROR", but I guess that would be confusing with engine errors? or there could be a setting that allows the change of the prefix.

[dalexeev]

I'm not sure customization is really needed here. The prefix serves to identify the message type, allowing engine and game developers to differentiate engine messages from custom errors/warnings. If you have a need to categorize your own messages, you can use a sub-prefix.

[Zireael07]

IMO further customization is the realm of user-made logger scripts

[dalexeev]

I think we could rename "USER ERROR" to "SCRIPT ERROR".

After reading the source, I realized that "SCRIPT ERROR" is already used. But we could replace "USER" with "CUSTOM" or something like that.

godot/core/io/logger.cpp

Lines 56 to 93 in 28a72fa

	void Logger::log_error(const char *p_function, const char *p_file, int p_line, const char *p_code, const char *p_rationale, bool p_editor_notify, ErrorType p_type) {
	if (!should_log(true)) {
	return;
	}
	const char *err_type = "ERROR";
	switch (p_type) {
	case ERR_ERROR:
	err_type = "ERROR";
	break;
	case ERR_WARNING:
	err_type = "WARNING";
	break;
	case ERR_SCRIPT:
	err_type = "SCRIPT ERROR";
	break;
	case ERR_SHADER:
	err_type = "SHADER ERROR";
	break;
	default:
	ERR_PRINT("Unknown error type");
	break;
	}
	const char *err_details;
	if (p_rationale && *p_rationale) {
	err_details = p_rationale;
	} else {
	err_details = p_code;
	}
	if (p_editor_notify) {
	logf_error("%s: %s\n", err_type, err_details);
	} else {
	logf_error("USER %s: %s\n", err_type, err_details);
	}
	logf_error(" at: %s (%s:%i)\n", p_function, p_file, p_line);
	}

[akien-mga]

I never really understood the point and the validity of the "USER" prefix. It seems to be added based on somewhat weird conditions which I'm not sure are what was intended.

As you can see in the code, it adds USER for errors which are not p_editor_notify, i.e. not meant for the toaster I guess? Seems very weird to me and in my experience, not related to whether the error originated in user code or editor code.

CC @groud who implemented this back then IIRC.

[groud]

As you can see in the code, it adds USER for errors which are not p_editor_notify, i.e. not meant for the toaster I guess? Seems very weird to me and in my experience, not related to whether the error originated in user code or editor code.

Yeah, p_editor_notify is mainly used to choose whether or not the error shows up in the editor as a toast. I think the rationale here was that errors with p_editor_notify would be user-caused errors and those without it would be internal. But in that case I think the output are inverse.

Anyway, I think it can be removed, it's probably useless and confusing.

CC @groud who implemented this back then IIRC.

I was about to say it was not me, then checked with git blame. Turns out it was me. Typical:

[akien-mga]

Yeah I agree that we can remove it. IIRC I had a quick look a while ago, and basically removing it means the whole p_editor_notify bool could be removed in all the multiple level of logging features we have, as it was only used for this (needs confirming).

Though depending on what's exposed, some care might be needed to not break compat.

[groud]

Yeah I agree that we can remove it. IIRC I had a quick look a while ago, and basically removing it means the whole p_editor_notify bool could be removed in all the multiple level of logging features we have, as it was only used for this (needs confirming).

Hmm, I meant to just remove theUSER prefix. The bool is needed for the editor toast. I don't know if there would be an other solution for it though, we kind of need a way to notify a logger an error needs to be popped without introducing editor code in the game code. That's why I added the boolean.

[akien-mga]

Yeah I agree that we can remove it. IIRC I had a quick look a while ago, and basically removing it means the whole p_editor_notify bool could be removed in all the multiple level of logging features we have, as it was only used for this (needs confirming).

Hmm, I meant to just remove theUSER prefix. The bool is needed for the editor toast. I don't know if there would be an other solution for it though, we kind of need a way to notify a logger an error needs to be popped without introducing editor code in the game code. That's why I added the boolean.

Right indeed, out of the 35(!) files where this bool is passed along, it's used meaningfully in

editor/gui/editor_toaster.cpp
168:    if (p_editor_notify || (show_all_setting == 0 && in_dev) || show_all_setting == 1) {
172:            if (!p_editor_notify) {

and so it shouldn't be removed.

[CreatedBySeb]

Hello, I'm looking to start contributing to Godot to give back since I've been using it for a few smaller projects and am beginning my first major project with the engine. I saw this issue was marked as 'good first issue' and would like to give it a go to get used to the flow of contributing to Godot. (I have experience making PRs at work and to another open source project, and some experience with C++, but I am unfamiliar with SCons or the Godot internals.)

My understanding from the discussion so far is that either:

1. lines 87-91 in core/io/logger.cpp can be simplified to just the function call on line 88
2. line 90 in core/io/logger.cpp can be updated to use a clearer prefix than 'USER', like 'CUSTOM' (or potentially 'DEVELOPER' or 'MANUAL'?)

Is one approach or the other preferable? An alternative option could potentially be to have the internal errors use an 'ENGINE' or 'INTERNAL' prefix, and leave developer emitted errors as just 'ERROR'/'WARNING'. I think that this approach could be clearer for a new or inexperienced user, rather than wondering where some .cpp file is in their project, but could be undesirable for other reasons.

[akien-mga]

lines 87-91 in core/io/logger.cpp can be simplified to just the function call on line 88

This approach is the consensus so far. There's a few other places that need to be adjusted similarly, which add a "(User)" suffix to some in-editor messages:

platform/windows/os_windows.cpp
175:            err_str += " (User)\n";

editor/editor_log.cpp
56:             err_str += " (User)";

[CreatedBySeb]

I have drafted a PR #96168 and validated it works as expected for the case outlined in the original issue on macos.arm64. I do not have access to Windows so cannot test it on that platform.

In the case of the platform/windows/os_windows.cpp modification I opted to move the end line to the initial assignments rather than have the second assignment given it no longer seemed necessary.

In the three functions modified, the p_editor_notify parameter seems to no longer used, is it preferable to remove it from these functions and then update their usages or leave in the redundant parameter? I am unsure if this would result in compatibility breakage as I don't know if the functions here are exposed.

[akien-mga]

In the three functions modified, the p_editor_notify parameter seems to no longer used, is it preferable to remove it from these functions and then update their usages or leave in the redundant parameter? I am unsure if this would result in compatibility breakage as I don't know if the functions here are exposed.

AFAIK it cannot be removed, these are virtual functions so they all need to have the same prototype, and the arguments is used in one of the more specific implementations (in editor/gui/editor_toaster.cpp).