Would it be possible to make AudioFrame: Clone + Copy?

[bluenote10]

Currently, the code generation seems to generate AudioFrame as

#[repr(C)]
pub struct AudioFrame {
    pub left: f32, pub right: f32,
}

Would it be possible the derive(Clone, Copy) here, perhaps also PartialEq? This would make a few things easier, and typically for a pair of floats implementing Copy should be fine. Or is there a reason why this must not be done?

[Bromeon]

Yeah, I don't see why not. This affects all native types, not just AudioFrame.

Should be a small change, I can try to integrate it into one of the PRs.

[vortexofdoom]

Was thinking of taking this on as a first dip in contributing.

Are there any types that SHOULDN'T be Clone/Copy? A couple of the types contain pointers, and ScriptLanguageExtensionProfilingInfo contains a StringName, which doesn't derive Copy, but is a newtype wrapper around Opaque, which does.

[Bromeon]

Yes, class types can't be Copy/Clone, because we cannot create values of them (just references).

Native structure types should be at least Clone. We can consider Copy, but some of them are bigger and this could potentially lead to large copies? Pointer shouldn't matter here.

I would start with Clone only. People typically create native structs once and pass them.

[vortexofdoom]

I figured that classes couldn't, was only going to for the native types listed above.

was gonna do Clone for all, maybe PartialEq for all? Copy for AudioFrame and ObjectId. Was also gonna do Eq, PartialOrd, and Ord for ObjectId since it's just a u64, and those comparisons are reasonable. I also split up the fields while going through

These are all in the .gitignore by default as well, should I be changing something in the codegen or in the end files themselves?

[Bromeon]

was gonna do Clone for all, maybe PartialEq for all? Copy for AudioFrame and ObjectId. Was also gonna do Eq, PartialOrd, and Ord for ObjectId since it's just a u64, and those comparisons are reasonable. I also split up the fields while going through

I would like this to be fully automated (no special cases) and simple in its implementation (no extra-smart heuristics to find out if a type can implement a certain trait or not). Reason is to avoid extra maintenance overhead whenever something on Godot's native type definitions changes, or new types are added. I already need to go ridiculous lengths for enums, no need to have this for every single niche API.

So, the traits can be: Clone, PartialEq, PartialOrd.
No Copy, Eq or Ord as they can't be applied to all.

If any of these causes problems in the future, we can revisit that, but otherwise YAGNI.

---

These are all in the .gitignore by default as well, should I be changing something in the codegen or in the end files themselves?

This is by design. Being generated code means you can't manually change their definition, hence my rationale above.

You would need to adjust godot-codegen directly.

---

Was also gonna do Eq, PartialOrd, and Ord for ObjectId since it's just a u64

We could add a conversion to Option<InstanceId> instead.

---

By the way, from your previous message:

[...] a StringName, which doesn't derive Copy, but is a newtype wrapper around Opaque, which does.

Just because it contains Copy fields, doesn't mean StringName is Copy itself. Copies of this type are tracked via Godot engine.

[vortexofdoom]

Awesome, thanks for all the clarification!

The goal of automation makes the path much clearer.

I also knew StringName wasn't Copy, I just saw that all it contained was a Copy type and I was making sure since there was nothing in the prior discussion that indicated any barriers toward both Copy/Clone.

EDIT: Just looked at classes to be consistent with where I put the derive macro, and figured I'd ask if I should include Debug?

StringName prevents PartialOrd as well, so probably won't include that.

Builds fine with Debug, Clone, PartialEq so I'll submit a pull request that just adds that line with/without Debug per your preference later

[Bromeon]

EDIT: Just looked at classes to be consistent with where I put the derive macro, and figured I'd ask if I should include Debug?

Yes, Debug might be nice.

StringName prevents PartialOrd as well, so probably won't include that.

I was looking into that again -- but it turns out it's not a good idea to provide PartialOrd and Ord for this type (see #586).

So yes, the Clone, PartialEq, Debug traits might be a good start. Derive them in this order please 🙂