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

Allow extending CanvasItem in GDExtension #67510

Closed
wants to merge 1 commit into from

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Oct 16, 2022

This PR allows extending CanvasItem and overriding its get_transform method in GDExtension by defining a method called _get_transform. This will not affect Control or Node2D because both of those classes override their own get_transform. The _get_transform method is therefore only used when implementing a custom CanvasItem.

@aaronfranke aaronfranke added this to the 4.0 milestone Oct 16, 2022
@aaronfranke aaronfranke requested review from a team as code owners October 16, 2022 21:44
@aaronfranke aaronfranke marked this pull request as draft October 17, 2022 20:42
@aaronfranke aaronfranke changed the title Allow overriding CanvasItem get_transform and get_anchorable_rect methods in GDExtension Allow extending CanvasItem in GDExtension Nov 7, 2022
@aaronfranke aaronfranke modified the milestones: 4.0, 4.x Nov 7, 2022
@aaronfranke aaronfranke marked this pull request as ready for review November 7, 2022 08:54
@aaronfranke aaronfranke requested a review from a team as a code owner April 16, 2023 03:25
@YuriSizov YuriSizov requested a review from a team April 18, 2023 13:52
@dsnopek
Copy link
Contributor

dsnopek commented May 18, 2023

For the most part, this looks good to me!

The one thing I'm unsure about is if there's any compatibility breaking issues with an abstract class becoming virtual? From the perspective of godot-cpp, I think it's fine. But I'm not sure what impact that would have on the Rust or Python bindings? Or, outside of GDExtension, what impact would this have in C#?

@dsnopek dsnopek requested a review from a team May 19, 2023 16:26
@raulsntos
Copy link
Member

There's some prior discussion here:

From what I can see in those discussions the conclusion was that it doesn't make sense to inherit/instantiate CanvasItem. I'm not sure if this decision has changed since then.


About breaking compatibility in C#:

Removing the abstract keyword from a C# type does not break compat, but we don't generate abstract classes. If a class is meant to be abstract, we generate it with an internal constructor so it can't be instantiated by the user. Changing a class from abstract to virtual in Core would mean the C# API will make the constructor public which does not break compat. Of course, the other way around does break compat, so if we make this change we can't go back.

@aaronfranke
Copy link
Member Author

@raulsntos Reduz's comment here from 2022 March 5 seems to be in favor of this PR:

here this reminds me that we should organize to go through all the most core classes and add GDVIRTUALS to allow inheriting and creating custom stuff, like custom textures, custom meshes, etc.

@raulsntos
Copy link
Member

A little bit after that the conversation continues like this:

akien: Sounds good to me. Is there a lot of stuff that would be virtual abstract?
reduz: most stuff I think
reduz: specially the nodes
reduz: like CanvasItem, Light3D, etc.
reduz: those make no sense to inherit from script
reduz: or extension

Which I interpreted as: CanvasItem would be abstract because it makes no sense to inherit from script or GDExtension.

After that, the conversation continues:

akien: Another related issue is godot#55549 for C#
reduz: yeah, but it is intended, you are not supposed to do it. If you consider it valuable you are most likely doing things wrong.
reduz: for BaseButton it makes sense, but for CanvasItem does not
reduz: which is why the disctinction should exist

So as I understand it, while he seems to be in favor of making some classes virtual, CanvasItem seems to be an exception.

@aaronfranke
Copy link
Member Author

I don't see why "those make no sense to inherit from script". It absolutely does make sense to inherit CanvasItem in GDExtension. This PR is all that's required, the rest of the code already works.

@dsnopek
Copy link
Contributor

dsnopek commented May 25, 2023

Discussed at the GDExtension meeting, and from a GDExtension perspective this seems fine from the technical side. It would be good to know more about both (1) the use-case for inheriting from CanvasItem and (2) why @reduz thought that it would make no sense to inherit from CanvasItem.

@BastiaanOlij brought up that we can't extend abstract classes because on the Godot side we have to make an instance of the class we are extending. In theory, because C++ modules can extend abstract classes, we should be able to in GDExtension as well, but this a technical limitation. Previous discussions had the outcome that we needed to create an *Extension class such as XRInterfaceExtension as to not pollute the abstract class, however, there were also discussions this was overkill (as is probably the case here because we're only adding one virtual method).

@SaracenOne SaracenOne self-assigned this Sep 16, 2023
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

Some use cases such as implementing an HTML engine (or anything that would involve wanting to create their own UI system without relying on the Godot's internal UI rendering logic) could be a potential use-case for exposing this were discussed in a conversation, so I think this looks good to me.

@reduz
Copy link
Member

reduz commented Feb 22, 2024

Sorry, as I mentioned before, I oppose this. Reasoning is:

  • No demand (@SaracenOne if you implement a web engine like WebKit you will not inherit CanvasItem, but instead just use RenderingServer directly).
  • No proposal attached with a reasonable use case for this related to a real-world scenario.
  • Generally, the code is simpler if GUI and 2D nodes don't share a lot, specially the transforms.

@reduz
Copy link
Member

reduz commented Feb 22, 2024

To make it clear, core code needs to be as bloat free as possible, so any change there has to be extremely well justified with a real-world application that is impeded without this change. It is not the case here.

@aaronfranke
Copy link
Member Author

Given that my justifications for this PR are not extreme, I'll close this.

For my 1D module I will just keep it as a module and note that it's not possible to do this in GDExtension.

@aaronfranke
Copy link
Member Author

Generally, the code is simpler if GUI and 2D nodes don't share a lot, specially the transforms.

Just to clarify, that is still the case even with this PR.

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.

6 participants