Skip to content

Conversation

@aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jun 5, 2025

I was working on some C++ code and trying to make it compatible with both in-engine module code and GDExtension C++ godot-cpp, and I found that the code uses .is_null() on an RID, which works in-engine (used 390 times), but not in GDExtension because the API is not exposed. This PR exposes it. Regardless of this PR, a workaround that is compatible with the currently released godot-cpp API is to use ! with .is_valid() on an RID instead of .is_null().

@aaronfranke aaronfranke added this to the 4.5 milestone Jun 5, 2025
@aaronfranke aaronfranke requested review from a team as code owners June 5, 2025 09:51
@Ivorforce
Copy link
Member

Ivorforce commented Jun 5, 2025

I'm not sure we need both.
I've already been apprehensive of there being two methods doing the same (give or take a ! anyway) inside the engine, but exposing the second function sets their difference in stone.

I suppose it comes down to if we ever expect there to emerge an actual implementation difference in the future. If not, we shouldn't expose both, and in my opinion, even consider removing one of the two functions altogether.

@aaronfranke
Copy link
Member Author

@Ivorforce I'm fine with not having both if that's the consensus, but only if the internal one is removed too. The internal and external APIs should match for trivial things like this.

@AThousandShips
Copy link
Member

I'm in favor of binding this, it's far clearer and the exposed API should match IMO

Copy link
Member

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

If C++ normally supported not there would be no discussion, but alas. Doc is fine

@akien-mga akien-mga changed the title Expose RID.is_null() to scripting Expose RID.is_null() to scripting Jun 5, 2025
@akien-mga
Copy link
Member

Makes sense for the sake of GDExtension and C++ modules compatibility.

Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

As I said above, I'm not at all convinced we should expose the same functionality twice.
If it's alright with everyone, I'd like to discuss this at the core meeting next Wednesday.

@clayjohn
Copy link
Member

clayjohn commented Jun 5, 2025

As I said above, I'm not at all convinced we should expose the same functionality twice. If it's alright with everyone, I'd like to discuss this at the core meeting next Wednesday.

Yes, let's discuss. We definitely shouldn't be exposing an API "for consistency" with an internal API when we don't even have consensus on the internal API

@beicause
Copy link
Contributor

beicause commented Jun 6, 2025

In C# Rid is a separate wrapper and IsValid is a property.

public bool IsValid => _id != 0;

I agree is_null is just a internal code style of Godot and it is not necessary, and Rid.IsNull might confuse me because there's also nullable Rid (Rid?) in C#. For example: if( rid != null ) if( rid.IsNull ) {} isn't clearer than !rid.IsValid.

@aaronfranke
Copy link
Member Author

If one is kept, I agree that is_valid is the better one to keep, but then we need to remove is_null internally.

@aaronfranke
Copy link
Member Author

Note that !is_valid was recently replaced with is_null in #100776 and #96076 in late 2024.

@AThousandShips
Copy link
Member

For Ref yes, this is RID which was not changed

@aaronfranke aaronfranke modified the milestones: 4.6, 4.7 Dec 7, 2025
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.

8 participants