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

Changing Control node Z Index pushes unnecessary Node configuration warning #69895

Closed
valedict0 opened this issue Dec 11, 2022 · 38 comments
Closed

Comments

@valedict0
Copy link

valedict0 commented Dec 11, 2022

Godot version

4.0.beta8

System information

Windows 10

Issue description

Changing the z_index of a Control node pushes a Node configuration warning to the editor:
image
This warning is unnecessary and also redundant, as the property description already outlines this:
image

Steps to reproduce

  1. Create a Control node.
  2. Change the Control node's z_index.
  3. Observe Node configuration warning.

Minimal reproduction project

N/A

@MewPurPur
Copy link
Contributor

#68070 The warning wasn't its own PR but part of the one that implemented this in the first place.

I agree this warning is unnecessary

@KoBeWi
Copy link
Member

KoBeWi commented Dec 11, 2022

This warning is necessary. Not everyone will read the description and I can already imagine some angry user coming to Reddit, or worse, GitHub, saying they wasted 5 hours debugging why their Control does not work as "expected". With warning in place, chance of this happening is 90% lower.

@YuriSizov
Copy link
Contributor

Closing as the warning is necessary.

@YuriSizov YuriSizov closed this as not planned Won't fix, can't repro, duplicate, stale Feb 10, 2023
@axilirate
Copy link

is there a way to hide this warning?

@YuriSizov
Copy link
Contributor

No, there is no way to hide it.

@eh-jogos
Copy link

eh-jogos commented Mar 31, 2023

I agree the warning is necessary, but can a way to hide this warning be added?

I think a way to disable this warning is also necessary, just as much as the warning itself.

I mean, I use a _get_configuration_warnings on my own scripts, sometimes even add @tool to a script just to add warnings that help me not make mistakes in my own scenes, and having this warning appear on places where using z_index in control node is exactly what I want is confusing/annoying. Also, after some time I don't know anymore if that node with a warning is one that I should pay attention to and fix something or if it's just the "z_index" warning, I have to click on them to read it.

In fact, it creates a situation where I actually want to ignore a warning, and I don't think getting used to ignoring warnings is something desired, if that's the case I should be able to disable the warning so that it doesn't "weaken" the effect of other warnings.

Or at least move it to something similar to the "layout" messages, if it can't be disabled:
image

@YuriSizov
Copy link
Contributor

You'd want to suppress it per node, and we don't have a system for that yet. Moving it out of a configuration warning and into a small info banner would defeat its purpose of preventing issues for people misusing the property (or changing it by accident/without a clear idea of the effect). If there is a good reason to introduce a system to suppress configuration warnings, per node, then your case will also be covered.

@eh-jogos
Copy link

Wouldn't it be possible to add a it in a similar way to the GDScript warnings we can set on ProjectSettings?

This is a warning the engine is forcing on me, and different than other warnings the engine has, like when you create an Area/PhysicsBody without a collision, this gives a warning to a perfectly plausible and valid use case.

It's like Godot is saying "hey we added this cool feature so that you can animate UI in a more friendly way, but every time you use it I'll keep nagging you that you're using it Wrong! (Even though you aren't I just don't have any way of knowing it)"

I don't want to suppress the "z_index" warning on some nodes and not on others, I want to suppress it always, because it's a warning that is not relevant to me or my project. I want to be able to tell Godot "Hey, chill out, I heard you but I'm telling you, everything is okay."

I mean, if there was some project settings for this like for the GDScript warnings, I suppose it would be possible to turn off this particular warning with something like this but in C++:

func _get_configuration_warnings() -> PackedStringArray:
	var warnings := PackedStringArray()
	if ProjectSettings.get_setting("debug/node_configuration/z_index_warning") == true:
		warnings.append("Changing the Z index of a control only affects the drawing order, not the input ecent handling order.")
	return warnings

Don't know if this would actually be similar in C++ but seems valid to me to add this for this case as this warning is already more of an exception since its purpose is to "prevent users from coming to complain on github" then pointing out an actual error in the way of using the node, so why not also be an exception in the sense that you can disable it?

And no new "system to suppress configuration warnings" would be needed, just like PhysicsBody has some way to check if they have a CollisionShape and stop showing their warnings, this would be the way for Control nodes to know everything is okay.

@Calinou
Copy link
Member

Calinou commented Mar 31, 2023

It's technically feasible to read a project setting value to decide whether to emit a node configuration warning in C++, but I don't think we should make band-aid changes like this. Eventually, people will ask to do this for every warning individually, which is a mess to maintain 🙂

@KoBeWi
Copy link
Member

KoBeWi commented Mar 31, 2023

This is a warning the engine is forcing on me, and different than other warnings the engine has, like when you create an Area/PhysicsBody without a collision, this gives a warning to a perfectly plausible and valid use case.

Is it? For a long time I had a scene setup where root was StaticBody2D and I had all colliders under a Node2D child, because I wanted the scene to be organized (I later moved them using script). So every of my 200+ scenes had a warning in root node and 2-15 warnings for each of the colliders.

Every API has its intended usage and warnings are a way to point to that and avoid common mistakes. If you are an advanced user and you know what you are doing, you can ignore them for the most part.

@eh-jogos
Copy link

eh-jogos commented Mar 31, 2023

It's technically feasible to read a project setting value to decide whether to emit a node configuration warning in C++, but I don't think we should make band-aid changes like this. Eventually, people will ask to do this for every warning individually, which is a mess to maintain slightly_smiling_face

Still, adding warnings for valid use cases just to prevent people that don't read documentation to come complain on github is also a band-aid change isn't it?

I agree in this case it's worth it, but if warnings like these also start to get added a lot then isn't it also a problem? I don't know, I think this particular band aid (the z_index warning) justifies another particular band aid (the option to remove this particular warning), and then if people actually start to request more things like this, then maybe it will be useful to think up a proper system for this with clear use cases and thinking on maintainability.

For now this warning seems like a band-aid that solves the "pain" of the people who would use this wrong, and the "pain" of the godot developers that don't want to keep closing bugs about controls with higher z_index not capturing input, but creates another "pain" for everyone that uses the property as it was intended.

That's why I agree this warning should continue existing, but just has as much reason to have the option to disable it: "to prevent people that use the property correctly from coming to github to complain about the warning"

I mean, like we say in brazil "if you're already in hell, might as well hug the devil"

@eh-jogos
Copy link

This is a warning the engine is forcing on me, and different than other warnings the engine has, like when you create an Area/PhysicsBody without a collision, this gives a warning to a perfectly plausible and valid use case.

Is it? For a long time I had a scene setup where root was StaticBody2D and I had all colliders under a Node2D child, because I wanted the scene to be organized (I later moved them using script). So every of my 200+ scenes had a warning in root node and 2-15 warnings for each of the colliders.

Every API has its intended usage and warnings are a way to point to that and avoid common mistakes. If you are an advanced user and you know what you are doing, you can ignore them for the most part.

Your example seems a lot more "advanced" and a choice in the developer side on how they want to architecture their game in a different way than what the engine expects/recommends. You even have to add a script to "fix it" in runtime.

While the z_index is just the "default" way the engine recommends to solve draw order issues with CanvasItem nodes, and I'm getting a warning by using the API Godot provides in the way Godot expects me to use it, not by doing something "advanced". So I don't really think the two examples compare that well.

But, okay, maybe having a whole system to suppress warnings by node types like @YuriSizov was mentioning would be more desirable, as you'd be able to use that in your project as well. Is there already some proposal for that?

@YuriSizov
Copy link
Contributor

Well, you are getting a warning precisely because the engine is not expecting you to use it. It allows you to use it, but it warns you because it's not really a safe feature to use in this case.

Also, I was talking about suppressing per node, not per node type. IMO this should never be something that you can just globally disable. And I say it as a person who uses those global GDScript warning settings to disable some default ones.

@eh-jogos
Copy link

Well, you are getting a warning precisely because the engine is not expecting you to use it. It allows you to use it, but it warns you because it's not really a safe feature to use in this case.

I think this is a bit contradictory, Godot 3.x went its whole existence without this option and having z_index on Controls is a very welcomed addition that simplifies a lot of UI problems/animations, I don't see how this was added but still expected not to be used, and there are far more unsafe options that give no warnings at all, like top_level.

But, thinking about the "safe" part, maybe another suggestion then:

  • if mouse_filter is set to either MOUSE_FILTER_STOP or MOUSE_FILTER_PASS and z_index is different then 0, show the warning, if it's set to MOUSE_FILTER_IGNORE then hide the warning.

@YuriSizov
Copy link
Contributor

This option exists in Godot 3, but it's not exposed because it was not designed to be used by controls. It's just possible to use in controls, because the underlying server works with all types of canvas items.

But because it is situationally useful, we agreed to expose it but with a warning. Which is what we did. It is no more safe to use it in 4 than it is in 3, so the warning is a compromise for making it more accessible but still strictly "not really supported". This is why we closed this issue with a note that the warning is necessary.

@eh-jogos
Copy link

eh-jogos commented Apr 1, 2023

I didn't know it was possible to set z_index in Controls in 3.x, would have saved me a lot of trouble, still, I find it weird this position that it is "still strictly 'not really supported'." when you recognize it was situationally useful and the documentation even give examples of when it is useful.

But what about the suggestion to make the warning go away when the control mouse_filter is set to MOUSE_FILTER_IGNORE? If the "problem" is the confusion that can come from input handling, wouldn't setting mouse_filter to ignore "fix it"?

I was looking at the PR linked here and while I don't know C++ very well yet, it seems like something simple enough that even I could do, I'd just need to change the condition for the warning instead of removing the warning like the PR is doing. It would also be a cool way (for me) to start looking more into godot source code and contributing.

But I don't want to take the time to do that if it will just be summarily ignored because the official position on this is that no matter what the warning must appear.

@YuriSizov
Copy link
Contributor

I find it weird this position

Well, yes, this situation is a good demonstration why exposing something that is not really designed to work in a certain way but people still do it — is a bad idea. We are sending mixed signals here, and IMO having just the old way of interacting with the server directly would've been enough for something that may or may not work as expected in all cases.

extends Control

func _ready():
    VisualServer.canvas_item_set_z_index(get_canvas_item(), your_value)

But we have reached a different consensus, so here we are. People who knew the trick wanted a better way to use it. But the limitations for controls themselves cannot be so easily fixed. For example, reordering of the input handling order is an expensive operation, so doing it as z-index changes would likely result in a way worse performance across the board. But since most requested it to be exposed for animations, it was deemed to be okay not to address that issue and keep the input order as is.

And there might be other limitations. The only way we can truly get rid of the warning is to fix the underlying issues without causing more problems. As for options to hide it conditionally, I still think that we probably need a more generic way to suppress arbitrary configuration warnings.

But what about the suggestion to make the warning go away when the control mouse_filter is set to MOUSE_FILTER_IGNORE? If the "problem" is the confusion that can come from input handling, wouldn't setting mouse_filter to ignore "fix it"?

And if you don't want to ignore it? I feel like you're trying to come up with an acceptable hack that you yourself can utilize. But this is not going to be applicable to everyone, so it's not going to be a good solution IMO.

@eh-jogos
Copy link

eh-jogos commented Apr 1, 2023

But what about the suggestion to make the warning go away when the control mouse_filter is set to MOUSE_FILTER_IGNORE? If the "problem" is the confusion that can come from input handling, wouldn't setting mouse_filter to ignore "fix it"?

And if you don't want to ignore it? I feel like you're trying to come up with an acceptable hack that you yourself can utilize. But this is not going to be applicable to everyone, so it's not going to be a good solution IMO.

No, I was really trying to think if there was any situation where a control with MOUSE_FILTER_IGNORE would cause any trouble with a changed z_index and could think of none.

So I thought it would be a valid improvement to anyone that decides to use z_index and for the engine itself not just for me or my project, as in fact it would not hide all warnings in my case.

I was asking more to see if it was a good idea or if there were other problems related to Control and z_index that are not about input handling and I'm not aware. Could have rephrased the question to be more specific.

I find it weird this position

Well, yes, this situation is a good demonstration why exposing something that is not really designed to work in a certain way but people still do it — is a bad idea. We are sending mixed signals here, and IMO having just the old way of interacting with the server directly would've been enough for something that may or may not work as expected in all cases.

I totally agree with you about the mixed signals, and even though I disagree that z_index on Controls is that harmful when it's clear it's just something related to drawing and not input handling, I'd also prefer that it wasn't exposed at all if it was going to be exposed with these mixed signals.

extends Control

func _ready():
    VisualServer.canvas_item_set_z_index(get_canvas_item(), your_value)

But we have reached a different consensus, so here we are. People who knew the trick wanted a better way to use it.

I had seen this function in the documentation before when I had to use VisualSever for a project in 3.x, but never realized I could use this for Controls, even though they are also CanvasItem, and you could just export a variable with a setter to make it more friendly. Damn, really wish I had realized this a couple years ago.

@wilfredjonathanjames
Copy link

I think there's an argument here for an "Info" notification on a node alongside the "Warning" notification for things that aren't game-breaking but the user should be aware of.

Alerts are generally things that the user needs to fix, and are fixable, whereas in this case we're talking about an FYI.

The behaviour in this issue isn't wrong, it's just being communicated to the user in a slightly UX-orthogonal way.

@MewPurPur
Copy link
Contributor

MewPurPur commented Jul 6, 2023

TBH I'm just confused about this whole premise. We exposed the property, even though using it always leads to warnings? 🤔

@wilfredjonathanjames
Copy link

TBH I'm just confused about this whole premise. We exposed the property, even though using it always leads to warnings? 🤔

Yeah, it's to avoid user confusion, which makes sense. I would probably have expected z-index to affect input event handling order if it wasn't spelt out so clearly.

I believe the reason it's communicated as a warning is a side-effect of the warning system being available when this was built, and considered a quick fix.

If we can find a nice way to keep the FYI while avoiding the use of the Warning system I'm sure it would solve the issue.

@WarrenMarshall
Copy link

Right, but it feels like one of those things that gets added in the interest of helping new people but does nothing but annoy intermediate to advanced folks. And there's nothing to be done about it - can't even be suppressed.

@stilestr
Copy link

Chiming in to say that I encountered this warning today, and was quite sad to find there was no way to suppress it. I think with this large influx of new users of an intermediate to advanced level, it would be worth re-considering having an "ignore this warning" button.

Very OCD triggering to have a warning that is irrelevant but can't be removed!

@mikehoyle
Copy link

I strongly agree. I think in all cases, permanent warnings should only be surfaced if they can/should be acted upon, thereby removing the warning.

The persistence of this warning is not just obnoxious, it also essentially forces me to learn to ignore Node-level warnings, because I will always be looking at yellow triangles in my Node hierarchy.

My UI proposal would be to include an "info" level icon in the Node warning system that exists for cases like these. If there are situations where information should be surfaced but not necessarily acted upon, then an "info" widget is more appropriate than a "warning" widget.

@Calinou
Copy link
Member

Calinou commented Sep 26, 2023

Please continue the discussion in the proposals linked here: #69895 (comment)

@TriceratopsAteVelociraptor

Yep, this is pretty atrocious UX. A warning that you can't fix, other than by basically not using the option. Then why does the option even exist?

@Calinou
Copy link
Member

Calinou commented Oct 25, 2023

Yep, this is pretty atrocious UX. A warning that you can't fix, other than by basically not using the option. Then why does the option even exist?

The Z Index property exists because it was requested in Control for a long time: godotengine/godot-proposals#839

We will need to have dismissable node configuration warnings eventually, as there are a lot of situations where additional node configuration warnings would be useful (but only until you acknowledge them). However, this is nontrivial to implement on a per-warning basis.

Node configuration warnings don't have a unique ID (only a string that contains the entire message). Storing the string works at a basic level, but it will break if the node configuration warning string changes in any way (even a typo fix) and will bloat the scene files somewhat. Changing the API to require warnings to have a unique ID would be a compatibility-breaking change. On the other hand, dismissing all warnings at once on a node could hide important configuration issues in the future, so I don't think it's a good idea.

Until then, #69903 has been rejected in the past so I don't think we should remove the warning.

@SkyanUltra
Copy link

SkyanUltra commented Feb 19, 2024

If we can find a nice way to keep the FYI while avoiding the use of the Warning system I'm sure it would solve the issue.

just coming here to say that i absolutely agree with the premise of finding some way to let the developer easily know that z-index only affects draw order. it is endlessly aggravating to have to look at my node structure and see a warning that i know isn't dangerous in my use case, but decides to make itself known regardless.

a couple of ways to fix it:

  • rather than showing up as a warning, it will show up as a small "i" icon, as an info bubble. these can be hidden after viewing them and selecting an option to not show this info again.
  • some form of hiding warnings like a simple "don't show this warning again" would work perfectly fine while still making the issue clear to those who actually need the warning.

really just any way to signify the issue without making it obnoxious to look at like a zit on a node structure would work. it's not like there NEEDS to be a warning there in the first place, considering how when hovering over z-index it parrots the same warning at you from there. it just feels horribly redundant.

@Calinou
Copy link
Member

Calinou commented Feb 20, 2024

Unfortunately, we can't implement different node configuration warning levels or discarding individual warnings without breaking compatibility with the existing _get_configuration_warnings() API. That method is expected to return an array of Strings, not objects that can specify a warning code (which is required to accurately target individual warnings) or severity.

The only thing we could do for now is having a way to discard all present and future warnings on a given node, which is likely a footgun in the long run as you can miss important warnings that appear in the future.

It's technically possible to add a new method and merge the return values of the old and new methods together (with the old method being deprecated), but this is a lot of work.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 20, 2024

It's possible to add a project setting to disable specific warnings. The _get_configuration_warnings() method would just check for that setting.

@Calinou
Copy link
Member

Calinou commented Feb 20, 2024

It's possible to add a project setting to disable specific warnings. The _get_configuration_warnings() method would just check for that setting.

Would it be targeted based on a node type or warning string? I feel storing the warning string in version control is problematic, as any upstream changes would invalidate it.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 20, 2024

No I mean the setting could be editor/controls/show_z_index_warning and Control node would check for this property specifically. We don't have a generic system to disable warnings, but we can do that on case-by-case basis.

@Proggle
Copy link
Contributor

Proggle commented Mar 16, 2024

Yep, this is pretty atrocious UX. A warning that you can't fix, other than by basically not using the option. Then why does the option even exist?

Yeah, having warnings that aren't relevant creates alarm fatigue, and means that all other warnings in Godot get degraded as a feature.

@BlueSpark12
Copy link

Right, but it feels like one of those things that gets added in the interest of helping new people but does nothing but annoy intermediate to advanced folks. And there's nothing to be done about it - can't even be suppressed.

Even as a very new user myself, the warning scares me more than anything, and makes me look into the reasoning behind it, only to find out that I'm using it correctly. The only reason I'm in this thread is due to the confusion and uncertainty that the warning causes.

I agree that it's certainly useful, but it also feels like there are countless other areas where using something incorrectly can cause breaks, yet there aren't constant warnings for every opportunity where things can fail

@RedMser
Copy link
Contributor

RedMser commented Mar 28, 2024

Unfortunately, we can't implement different node configuration warning levels or discarding individual warnings without breaking compatibility with the existing _get_configuration_warnings() API.

Since my PR for configuration warnings for certain properties also requires a breaking API change of some kind, I could see value in combining both approaches. So a _get_configuration_info() that returns an array of dicts with message, severity, property, possibly a unique ID code for discarding

@Kingpiplup
Copy link

Id be satisfied with a simple "Ignore Warning for this node" button if the certain warning pops up

@KoBeWi
Copy link
Member

KoBeWi commented May 16, 2024

The warning is removed in newest 4.3 release.

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

Successfully merging a pull request may close this issue.