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

Fix C# Hint NodeType and ResourceType HintString #91645

Merged
merged 1 commit into from
May 10, 2024

Conversation

ZerxZ
Copy link
Contributor

@ZerxZ ZerxZ commented May 7, 2024

Fix #91644

@ZerxZ ZerxZ marked this pull request as ready for review May 7, 2024 04:13
@ZerxZ ZerxZ requested a review from a team as a code owner May 7, 2024 04:13
@ZerxZ ZerxZ force-pushed the dotnet/hint-string-fix branch from d4da121 to 231eb47 Compare May 7, 2024 04:35
@AThousandShips AThousandShips added this to the 4.3 milestone May 7, 2024
@ZerxZ ZerxZ force-pushed the dotnet/hint-string-fix branch 3 times, most recently from 1bf2d8c to 0d6465c Compare May 7, 2024 12:40
@raulsntos
Copy link
Member

Thanks for contributing to the .NET module! This looks really nice although I feel like it goes against the current behavior.

The way [Export] currently works we don't usually override the hint and hint string computed from the property type, the type takes precedence over the attribute. This means if you have an exported property with an enum type, you can't change how the enum members will be displayed even if you specify it in the attribute.

I'm a bit worried that this change will make things a bit confusing to users, because now some times the type will take precedence and others it will be the attribute.

My personal preference would be for the attribute to always override the type, since I believe when the user is being explicit we should respect that. However, this is a behavior change that may be a bit too disruptive for existing users.

And now that I laid out my concerns and personal preferences, I'd love to hear what other .NET contributors think.

@paulloz
Copy link
Member

paulloz commented May 8, 2024

My personal preference would be for the attribute to always override the type, since I believe when the user is being explicit we should respect that.

Same thing, I feel like always respecting the explicit metadata given by the user (if valid) is what we want / what anyone would expect.

However, this is a behavior change that may be a bit too disruptive for existing users.

Is it disruptive? I'd imagine existing users either

  • Don't have hint string specified: no disruption for them.
  • Have some hint string specified, but it does nothing1: the behaviour will change, but how likely is it that users leave not working hints in their code?
  • Have some hint string specified, and it works2: the behaviour should not change here, no disruption for them.

Or is there something I'm missing/misunderstanding?

Footnotes

  1. [Export(PropertyHint.ResourceType, nameof(Texture2D))] public Resource Resource

  2. [Export(PropertyHint.ResourceType, nameof(Texture2D))] public GodotObject Resource

@ZerxZ
Copy link
Contributor Author

ZerxZ commented May 8, 2024

GDScript: Add @export_custom annotation #72912
Should [Export] be used consistently with @export_custom?
@raulsntos @paulloz


The new @export_custom allows this.

extends Node
enum Alignment { 
 Good,
 Neutral,
 Evil
}
@export_custom(PROPERTY_HINT_NODE_TYPE,"Node2D,Object") var node:Object
@export_custom(PROPERTY_HINT_NODE_TYPE,"Node2D,Resource") var node2:Node
@export_custom(PROPERTY_HINT_RESOURCE_TYPE ,"Node2D,Texture2D") var nodeResouce:Resource
@export_custom(PROPERTY_HINT_RESOURCE_TYPE ,"Node2D,Texture2D") var enumResouce:Alignment

I don't think [Export ] in C# should prevent users from using .
As long as both Hint and HintString are available, the user should be allowed to use them.

public partial class CustomNode: Node
{
    public enum  Alignment { 
        Good,
        Neutral,
        Evil
    }
    [Export(PropertyHint.NodeType, "Node2D,Object")] public GodotObject Node;
    [Export(PropertyHint.NodeType, "Node2D,Resource")] public Node Node2;
    [Export(PropertyHint.ResourceType, "Node2D,Texture2D")] public Resource NodeResouce;
    [Export(PropertyHint.ResourceType,"Node2D,Texture2D")] public Alignment MyResource;
}

Even if something goes wrong, it should be thrown a warning by the editor instead of being directly overwritten by the source code generator.

@ZerxZ ZerxZ force-pushed the dotnet/hint-string-fix branch 2 times, most recently from 710d0d2 to 06b716d Compare May 8, 2024 14:23
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is there something I'm missing/misunderstanding?

@paulloz No, I think you got it perfectly. Whenever something changes behavior I get a little nervous, but I think you are completely correct that it won't be disruptive, thank you.

Should [Export] be used consistently with @export_custom?

@ZerxZ Yeah, I think the GDScript annotation fulfills the same use cases. Thank you for showing how @export_custom works, this was helpful. I think it makes sense to follow the same behavior, and it probably matches user expectations when using the attribute.


So it sounds like there's consensus on allowing explicit hints override computed hints, since explicitness indicates intention. We should probably make sure other hints also work like this, but that would be work for a follow-up PR.

This PR goes in that direction and works as expected, so I'm approving.

@ZerxZ
Copy link
Contributor Author

ZerxZ commented May 9, 2024

Or is there something I'm missing/misunderstanding?

@paulloz No, I think you got it perfectly. Whenever something changes behavior I get a little nervous, but I think you are completely correct that it won't be disruptive, thank you.

Should [Export] be used consistently with @export_custom?

@ZerxZ Yeah, I think the GDScript annotation fulfills the same use cases. Thank you for showing how @export_custom works, this was helpful. I think it makes sense to follow the same behavior, and it probably matches user expectations when using the attribute.


So it sounds like there's consensus on allowing explicit hints override computed hints, since explicitness indicates intention. We should probably make sure other hints also work like this, but that would be work for a follow-up PR.

This PR goes in that direction and works as expected, so I'm approving.

Thanks! 🎉

@akien-mga akien-mga merged commit 7704457 into godotengine:master May 10, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@ZerxZ
Copy link
Contributor Author

ZerxZ commented May 10, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

Thanks! 🎉

@ZerxZ ZerxZ deleted the dotnet/hint-string-fix branch May 15, 2024 09:48
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.

C# Export Hint NodeType and ResourceType HintString Cannot be displayed correctly
6 participants