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

Don't resolve icons from script base class. #76993

Closed

Conversation

HolonProduction
Copy link
Member

@HolonProduction HolonProduction commented May 12, 2023

fixes partially #76983

There are three cases:

  1. The class of the node matches the script base class: In this case it does not matter whether the icon is resolved from script or class name
  2. The script class is more general than the node (script extends Node but added to a Node2D): In this case the more specific icon should be shown. The icon has to be resolved based on the class name.
  3. The script is incompatible with the node type. This will not work when running and therefore it makes sense to show the node icon and not that of the script.

There is no case I can think of, in which the icon should be resolved based on the script base type. Therfore this PR removes this code.

@Rindbee
Copy link
Contributor

Rindbee commented May 12, 2023

There is no case I can think of, in which the icon should be resolved based on the script base type. Therfore this PR removes this code.

I think it's the case in the inspector. See the picture below.

2

@HolonProduction
Copy link
Member Author

HolonProduction commented May 12, 2023

I think it's the case in the inspector.

Seems you are right with this. That makes it a little bit complicated. The passed class name in this case is GDScript so the script base should be evaluated first.

Maybe we could check whether the passed classname is a subtype of Node.

@HolonProduction
Copy link
Member Author

HolonProduction commented May 14, 2023

Ok so there is basically the fourth case, that the object is a Script. Only then should the script base type be taken into account. This special case is already present in get_object_icon. I extended it to pass the script base type as class name. The behaviour is therefore the same as if resolving based on script. I make the assumption, that all Script subtypes will yield an icon. The fallback would never be triggered in this case and the actual class name can be put into the fallback. (This might lead to problems with custom script types that are added through GDNative but I cannot think of a better solution without splitting the logic in two functions and violating DRY)

@YuriSizov
Copy link
Contributor

YuriSizov commented May 15, 2023

This is an incorrect way to solve the reported problem because the problem is not about icons. You've proposed the correct approach in the issue itself, we should warn with a configuration warning that the script is attached to a wrong kind of node.

But there is nothing wrong with the icon resolution logic. If there is no script, then the node's type name is used. If there is script, then the most relevant icon for that script is used. The icon ends up misleading in the reported case because the case is invalid. So it's not the icon that we should worry about.

@HolonProduction
Copy link
Member Author

In the beginning the issue was about icons but it shifted away from that. The confusing case which the issue reffered to, might not be important if adding a warning. However there is still a problem with the icon resolution logic. If you use a script that inherits Node and attatch it to a Node2D the icon displayed may confuse the user about the node type. As far as I know this is a valid use case.

Thinking about how the discussion on the issue moved along I should propably open a seperate issue for that.

@YuriSizov
Copy link
Contributor

If you use a script that inherits Node and attatch it to a Node2D the icon displayed may confuse the user about the node type. As far as I know this is a valid use case.

It is valid, but it's questionable to me which icon should be displayed in such a case. Why would the node type be more important than the script type? And if this script does have an icon, how does that change things? This icon would be also associated with a script extending the base class not the node's class, so you could also argue that showing it would be incorrect.

@YuriSizov
Copy link
Contributor

Superseded by #79203. Thanks for your contribution nevertheless!

@YuriSizov YuriSizov closed this Jul 26, 2023
@YuriSizov YuriSizov removed this from the 4.x milestone Jul 26, 2023
@HolonProduction HolonProduction deleted the script-icons branch December 9, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants