Change how ImageTexture's image is defined

[KoBeWi]

Inspired by godotengine/godot-proposals#9385

This PR changes how image of ImageTexture is defined. It's now a real property (still internal). The only thing that changed is that you can modify it using _validate_property().

[AThousandShips]

This might break C#, setters/getters in C# are not exposed but instead use internal which means that exposing this as a property will hide those accessors, at least SetImage, which is currently public, please confirm that it is still public in the generated files

[KoBeWi]

Seems like it might not apply to internal properties. If the setter was hidden, the checks would fail due to doc differences. So breaking C# would mean a bug in language implementation.

[AThousandShips]

It does apply to internal properties, it does for CanvasItem.anchors_preset, it doesn't apply to the documentation though, but it does to C#

Internal properties are accessible to scripting, but not documented, but all properties are exposed in C# and treated the same, please confirm

Edit: Building and checking now

Before:

    /// <summary>
    /// <para>Replaces the texture's data with a new <see cref="Godot.Image"/>. This will re-allocate new memory for the texture.</para>
    /// <para>If you want to update the image, but don't need to change its parameters (format, size), use <see cref="Godot.ImageTexture.Update(Image)"/> instead for better performance.</para>
    /// </summary>
    public void SetImage(Image image)
    {
        NativeCalls.godot_icall_1_50(MethodBind2, GodotObject.GetPtr(this), GodotObject.GetPtr(image));
    }

After:

    public Image Image
    {
        get
        {
            return GetImage();
        }
        set
        {
            SetImage(value);
        }
    }
...
    /// <summary>
    /// <para>Replaces the texture's data with a new <see cref="Godot.Image"/>. This will re-allocate new memory for the texture.</para>
    /// <para>If you want to update the image, but don't need to change its parameters (format, size), use <see cref="Godot.ImageTexture.Update(Image)"/> instead for better performance.</para>
    /// </summary>
    internal void SetImage(Image image)
    {
        NativeCalls.godot_icall_1_50(MethodBind2, GodotObject.GetPtr(this), GodotObject.GetPtr(image));
    }

So this unfortunately breaks compatibility, but it isn't a broken implementation, it's the design of C# in regards to properties

CC: @raulsntos @paulloz

[KoBeWi]

IMO that design should match the engine design. I don't see why the method can't be public other than arbitrary decision.
But if it's a problem, I could just bind another method that wraps set_image().

[AThousandShips]

It matches the engine design, internal properties are exposed to scripting? C# doesn't expose setters/getters by design because of how properties are supposed to be handled in C#, it isn't arbitrary, it is used as a setter and therefore not exposed, just like every single other setter

[KoBeWi]

Yes, but their setters are also exposed as methods accessible normally.

[AThousandShips]

No, they aren't, C# setters are not exposed, regardless of internal?

Example (Animation):

    /// <summary>
    /// <para>The total length of the animation (in seconds).</para>
    /// <para><b>Note:</b> Length is not delimited by the last key, as this one may be before or after the end to ensure correct interpolation and looping.</para>
    /// </summary>
    public float Length
    {
        get
        {
            return GetLength();
        }
        set
        {
            SetLength(value);
        }
    }
...
    internal void SetLength(float timeSec)
    {
        NativeCalls.godot_icall_1_57(MethodBind64, GodotObject.GetPtr(this), timeSec);
    }

AFAIK the setters/getters being internal is by the C# standard

[KoBeWi]

I was referring to your first sentence. In engine, setters of internal properties are normally available, unlike setters of "public" properties. They appear in documentation and autocompletion like normal methods.

[AThousandShips]

Yes, if they aren't prefixed with _, they still appear in autocompletion

It wouldn't make sense to have just internal properties have exposed setters/getters in C#, but not the rest

[KoBeWi]

Yes, if they aren't prefixed with _, they still appear in autocompletion

Setters of regular properties aren't prefixed with _ and aren't autocompleted.

In any case I pushed a separate method. IMO it's something that should've been caught by CI; thanks for checking.

[AThousandShips]

Worth discussing the long term aspects of C# method accessibility etc., but I think it can have unforeseen consequences (I think you can shadow an internal method for example but not a public one)

But good point about CI, wonder if we can catch that somehow

[AThousandShips]

LGTM, and makes sense, verified that the set_image method is now once again public (get_image was never affected by this, this doesn't happen to inherited methods)

Might be worth checking how we can make _verify_property work with dynamic properties in the future, but don't think there's any issue with exposing this now, I'd say that it's probably a bit more efficient to do this as well as we don't go via set/get

[KoBeWi]

Might be worth checking how we can make _validate_property work with dynamic properties in the future

The thing is that it doesn't make much sense. _get_property_list() already allows all kinds of dynamic behavior, so doing another pass in _validate_property() would be redundant. It's unfortunate that you can't customize properties if you don't control them, but _get_property_list() is not even supposed to be used for single properties like image, so this is a rare case.

[AThousandShips]

But are parent class dynamic properties available in _validate_property of a derived class? There it doesn't matter if it's dynamic or not because you can't prevent it being exposed by the parent class?

That is a pretty common use case (take clip_children for example, though it's handled differently for efficiency's sake)

[KoBeWi]

But are parent class dynamic properties available in _validate_property of a derived class?

It's not. Maybe if there is an easy way to parse parent's dynamic properties it could be added.

[akien-mga]

CC @godotengine/dotnet

IMO it's a shame to add a dummy _set_image method just to avoid using the already exposed setter to prevent from having C# bindings hide it.

[raulsntos]

This is an issue that has come up before and I've been meaning to do something about it, so here it finally is:

• C#: Make internal properties and property accessors public (but hidden) #90002.

What this does is:

• Properties without the PROPERTY_USAGE_INTERNAL are public (not hidden).
• Properties with the PROPERTY_USAGE_INTERNAL are also public (but hidden).
• Methods that are property accessors are public (but hidden).
• Methods that are property accessors of hidden properties are public (not hidden).

This should prevent breaking binary compat since now everything is public, but we are still hiding as much as possible from IntelliSense to still encourage users to use the properties when available and avoid polluting their IntelliSense.

---

Also, thanks @AThousandShips for catching the compat break. This is something that should be catched by #77183 but it's still a work-in-progress.

[KoBeWi]

So should I revert to the original state of this PR?

[AThousandShips]

Good to know! Thank you @raulsntos! Wasn't sure if they were internal by convention or not, I might have mixed things up with the general use of property access in C# as opposed to private variables etc., went and searched while we were discussing this and didn't find anything explicit in the general C# materials by Microsoft, but those didn't really show examples of more complex methods being used but generally just in the get/set themselves, but our use case is a bit unusual as we have bindings

And I try to make it my task to dig through and problematice and analyse the possible consequences of things especially when talking about compatibility and interplay with different scopes, because it's damn hard to keep track of how things affect two different languages, plus extension, plus the engine itself

Awaiting new decisions based on #90002