Expose the get_rid method of Joint2D and Joint3D

[Ughuuu]

Adds the get_rid property to Joint2D and Joint3D nodes.

• Bugsquad edit, closes: Expose the get_rid property from joints godot-proposals#7519

[AThousandShips]

I'd say to call it by the original method name, the documentation makes it clear, and having it named thus makes it clearer what you are getting

Using a different name from the c++ method for the bind feels error prone, and breaks with how Camera3D, VisualInstance3D, NavigationRegion3D, Viewport, etc. does things

[rburing]

It makes sense to expose this, e.g. to allow physics server extensions to add features to joints and to make those features usable from joint nodes.

[mihe]

With stuff like the physics bodies (RigidBody3D, etc.) you can currently (in GDScript at least) pass references of them into parameters that accept a RID and it will implicitly convert it for you, without you having to call self.get_rid() or anything like that.

So you can do stuff like:

extends RigidBody3D

func _physics_process(_delta: float):
    print("My collision layer: ", PhysicsServer3D.body_get_collision_layer(self))

Does anyone know through what mechanism that happens?

That might be worth adding to this PR as well, if it's something that needs to be explicitly bound somehow.

[mihe]

It looks like the implicit cast is done through Variant::operator RID(), where it just assumes that the object will have a bound method on it called get_rid:

godot/core/variant/variant.cpp

Line 2128 in 0511f9d

Variant ret = _get_obj().obj->callp(CoreStringNames::get_singleton()->get_rid, nullptr, 0, ce);

I guess that should work just fine then. Nevermind. :)

[Ughuuu]

I'd say to call it by the original method name, the documentation makes it clear, and having it named thus makes it clearer what you are getting

Using a different name from the c++ method for the bind feels error prone, and breaks with how Camera3D, VisualInstance3D, NavigationRegion3D, Viewport, etc. does things

Not sure what the original name is or what to change. Can you explain or leave code feedback where the change should be?

[lyuma]

I'd say to call it by the original method name, the documentation makes it clear, and having it named thus makes it clearer what you are getting

@AThousandShips I agree with the principle but disagree with naming it get_joint(). As suggested in Variant::operator RID(), Godot expects this method to be called get_rid() so we should use that in the public facing API.

Since the current C++ code uses a private API named get_joint() (not exposed), there would be no API breakage by renaming existing C++ code. Therefore I would rather suggest the C++ code be changed to use naming consistent with other Node classes that use RID internally, such that the public API can be named get_rid().

[lyuma]

Sorry I don't always write clearly. Keep get_rid() in the public API. Consider changing existing C++ code and private APIs to match other classes, for example:

 _FORCE_INLINE_ RID get_rid() const { return rid; }

It will change many more lines of code, but it would be the right thing to do, IMHO.

The reason why @AThousandShips' comment matters, is because GDExtension C++ code will use an API generated from the method bindings. GDExtension is designed to be mostly compatible with Godot modules. However, when there is a mismatch in method naming, it would mean that GDExtension code would use a different function name (get_rid) than the module code which accessess the C++ headers (get_joint). Therefore, we should change the existing private C++ code to match.

[AThousandShips]

Keep get_rid() in the public API

They aren't named get_rid in the API though, all these examples use that in the API, in fact most non-Resource types do not use get_rid but something else, exceptions are CollisionObject and a few others, though I don't remember which else

[lyuma]

exceptions are CollisionObject and a few others

Irrespective of what other decisions were made in the past for other Node types, it makes sense for Joint to be consistent with CollisionObject, since both are part of physics. I still think it should be get_rid() because CollisionObject* also uses get_rid().

[Ughuuu]

Changed it. It wasn't used at all it seems, so it was simple. Now the method is also called get_rid instead of get_joint

[AThousandShips]

As long as the c++ and API names line up I'm for either option, though I think it needs some more reviewers to confirm

[YuriSizov]

exceptions are CollisionObject and a few others

Irrespective of what other decisions were made in the past for other Node types, it makes sense for Joint to be consistent with CollisionObject, since both are part of physics. I still think it should be get_rid() because CollisionObject* also uses get_rid().

There is an argument to be made that this makes things more inconsistent in the engine overall. If we only have a handful of exception it's not that bad, but if we start using exceptions as a second rule, then we end up with a mess.

Resources and RefCounted objects seem to use get_rid consistently, while nodes seems to use pretty much whatever makes sense to them (get_canvas_item, get_skeleton, get_camera_rid, get_instance). There isn't many of such methods on nodes, but they are all unique. Whether we can consider that the preferred style or not I don't know.

Well, as long as there is consensus on this PR, we can go with either option.

---

@Ughuuu Could you please amend the commit message? The title of this PR is a good option for it.

[AThousandShips]

Please squash your commits into one, see here

[Ughuuu]

Oh, sorry, I am trying, hope I got it right this time.

[akien-mga]

Seems good.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[Ughuuu]

Thank you! Can't wait to have my name permanently immortalised in the list of godot contributors(jk)