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

Consistent by-value and by-ref parameters for geometric types #749

Closed
Ughuuu opened this issue Jun 9, 2024 · 3 comments · Fixed by #751
Closed

Consistent by-value and by-ref parameters for geometric types #749

Ughuuu opened this issue Jun 9, 2024 · 3 comments · Fixed by #751
Labels
c: core Core components quality-of-life No new functionality, but improves ergonomics/internals

Comments

@Ughuuu
Copy link
Contributor

Ughuuu commented Jun 9, 2024

I am trying to make a physics server 2d and physics server 3d, and I want to use:

Rect2
Aabb

If I look at the docs, it says Aabb is the 3d counterpart of Rect2.

The method for merge of Aabb is:

pub fn merge(&self, b: &Aabb) -> Self {

The method for merge for Rect2 is:

pub fn merge(&self, b: Rect2) -> Self {

Could the 2 change so either both have reference or neither have reference for the parameter b?

@lilizoey lilizoey added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Jun 9, 2024
@Bromeon
Copy link
Member

Bromeon commented Jun 10, 2024

This is a wider issue than merge, we need to establish a clear policy when to use by-value and by-ref for geometric types.

So far, the idea was that Rect2 is by-value, because it has the same size as Vector4, and vectors are always by-value for convenience. However, this is not reflected in Rect2's current &self receivers.

In general, things could be more consistent. I compiled an overview of the parameter passing in our builtin types:

Type Words (à 4 bytes) Parameter
Vector2, Vector2i 2 by-value
Vector3, Vector3i 3 by-value
Vector4, Vector4i 4 by-value
Quaternion 4 by-value
Color 4 by-value
Rect2 4 by-ref (mostly)
Rect2i 4 by-ref (mostly)
Aabb 6 by-ref
Plane 4 by-ref
Transform2D 6 mixed
Basis 9 mixed
Transform3D 12 mixed
Projection 16 by-ref

"mixed" means by-value is used if the return type is Self, by-ref otherwise.

@Bromeon Bromeon changed the title merge method in Aabb and Rect2 have different API. Consistent by-value and by-ref parameters for geometric types Jun 10, 2024
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jun 10, 2024

Impressive investigation, thanks.

I believe we should do by value those that are up to 4 words, and also include Aabb (6 words) for symmetry reasons. All the others I propose to be by reference, for performance reasons(eg. with worst case being double builds, where a word is 8 bytes).

@Bromeon
Copy link
Member

Bromeon commented Jun 12, 2024

Some more input on this, in terms of performance?

I checked glam, and it seems:

  • All vectors use by-val, even DVec4 -- 8 words
  • All matrices use by-ref, even Mat2 -- 4 words
  • Quat uses by-val -- 4 words

So after consistency changes, we do the same for the above types, and additionally treat Aabb, Plane, Rect2, Rect2i as by-val.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants