Make CanvasItem's "drawing outside of NOTIFICATION_DRAW" error a macro

[Mickeon]

The premise is simple:

Imagine this but 25 times.

I also took the chance to improve upon the error message itself. At least, attempted to.

Please give suggestions for a better macro name, I do not know the pattern.

Resolves #75376.

[Mickeon]

I do not have the capacity to test any of this so I just stripped the DEBUG_ENABLED

[AThousandShips]

LGTM

Worth doing a future pass to see if it's something to make a debug only check, but generally we don't make that distinction unless it's very development side, if it's possible without weird side effects it'd be worth considering, but since the check is so cheap I don't think it's worth evaluating on it's own, maybe a future sweep for different checks would be something to consider

[kleonc]

I also took the chance to improve upon the error message itself. At least, attempted to.

This should resolve #75376. But maybe the error message should also explicitly state what this and CanvasItem::current_item_drawn are when the error happens? Directly pointing out the problematic nodes should make debugging / understanding what's the problem much easier.

[Mickeon]

I do agree and I tried to do that. Problem is I just couldn't find a nice "spot" to explicitly state both.

[AThousandShips]

CanvasItem::current_item_drawn will always be nullptr when drawing is false though

[Mickeon]

Yeah, that one cannot be done. But the node name can. I wouldn't want the error to be too verbose as it will be prone to get spammed in the debugger. Not pretty, so I gotta be careful.

[AThousandShips]

Risks causing unhelpful editor paths indeed, like parenting errors at times root/@EditorNode@17034/@Panel@13/@VBoxContainer@14/@HSplitContainer@17/@HSplitContainer@25/@VSplitContainer@27/@TabContainer@29/Scene/@P

[Mickeon]

I thought about it and I cannot fit the name anywhere. I wish there was a standard way to mention it.
vformat("Node (%s) cannot draw outside of its drawing routine. Drawing is only allowed inside this node's `_draw()`, functions connected to its `draw` signal, or when it receives NOTIFICATION_DRAW.", )

But this is bad already.

Of course that doesn't mean errors shouldn't be more explicit, and the Errors tab should be MUCH IMPROVED for this, but if this message is any longer it'd be just plain irritating.
This behavior is already documented (by yours truly may I add) but perhaps it needs to be even more explicit, as what the "drawing routine" is isn't exactly clear on the surface.

[Mickeon]

Applied both suggestions. Note that the ERR_THREAD_GUARDs are not formatted the same way.

[AThousandShips]

Re-confirming, let's take a look later at a standardized and low-clutter way to convey where errors occur as per the suggestions above, but looks good just as a cleanup

[kleonc]

CanvasItem::current_item_drawn will always be nullptr when drawing is false though

Not really, you can call draw_x methods on a different CanvasItem than current_item_drawn and it will result in such errors, as such other CanvasItem is not drawing at that point (drawing is non-static contrary to current_item_drawn).
That's exactly when the current error message (pre this PR) would confuse people when they try to do something like:

extends Node2D
func _draw():
	# When this is called `current_item_drawn` is `self`.
	some_other_canvas_item.draw_circle(...) # Any `draw_x` method called on non-self canvas item.

	# Error: "Drawing is only allowed inside NOTIFICATION_DRAW, _draw() function or 'draw' signal."
	# User: What do you mean?! It is in `_draw()`...

The error being changed to:

"Drawing is only allowed inside this node's `_draw()`, functions connected to its `draw` signal, or when it receives NOTIFICATION_DRAW."

already makes the error message correct/clearier. 👍

---

Regarding further improvement - yeah, I agree it's out of scope of this PR. I'm also not sure what would be a good way to refer to the problematic nodes. Path seems too long, just names too vague, Node::to_string which includes ObjectID not "friendly enough" and still it doesn't provide info the path does, etc.

---

BTW it could be also considered to have slightly different error messages depending on the case, like:

ERR_FAIL_COND_MSG(current_item_drawn == nulltpr, "..."); // Not in drawing routine at all.
ERR_FAIL_COND_MSG(current_item_drawn != this, "..."); // In drawing routine of a different CanvasItem.

(drawing bool would be no longer needed if the error conditions would be changed like that.)

Wondering if something like this could make things clearer even if not directly specifying what canvas item(s) this happened for. But also not sure how doubling the checks would affect performance.

Just a thought, not saying it should be done in here. This PR is fine as is.

[Mickeon]

Speaking of the much prior topic, if only checking for drawing in debug builds has a good chance of breaking things in release builds, I have to question how bad things can break when the raw RenderingServer methods try to draw outside of the routine, as well? If that's even possible?

[akien-mga]

Thanks!