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

Gizmos primitives by ref #13427

Closed
lynn-lumen opened this issue May 19, 2024 · 2 comments · Fixed by #13534
Closed

Gizmos primitives by ref #13427

lynn-lumen opened this issue May 19, 2024 · 2 comments · Fixed by #13534
Labels
A-Gizmos Visual editor and debug gizmos C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A simple quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon

Comments

@lynn-lumen
Copy link
Contributor

What problem does this solve or what need does it fill?

Currently gizmos.primitive_2d() and gizmos.primitive_3d() require a parameter of type P: PrimitiveNd. Passing &my_primitive is not allowed.
This is fine and barely noticeable for primitives that implement Copy but those that don't, like Polygon<N> or BoxedPolyline, need to be cloned every time they are drawn using gizmos (The examples/math/render_primitives avoids this by having all primitives be const).

Some solutions

Since the primitive_Nd functions do not do anything that could not be done with &my_primitive as of now, they could just accept this as a parameter instead.
This would obviously cause existing code to be rewritten (gizmos.primitive_2d(PRIMITIVE) -> gizmos.primitive_2d(&PRIMITIVE)) which is rather annoying.

We could introduce a new trait akin to GizmoRefPrimitiveNd<P: PrimitiveNd> with a function primitive_ref_Nd(primitive: &P, ...) that accepts primitives by ref. GizmoRefPrimitiveNd could probably even be implemented automatically for primitives implementing GizmoPrimitiveNd.

If we allow bevy_math to be changed to fix this, we could implement PrimitiveNd for &P where P is some Primitive that does not implement Copy. This would allow implementing GizmoPrimitiveNd<&P>

@lynn-lumen lynn-lumen added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels May 19, 2024
@mweatherley
Copy link
Contributor

I'm personally of the mind that the primitive_Nd functions should probably just take the primitive argument by reference; I wouldn't be surprised if they were initially conceived under the assumption that primitives would be Copy. That unfortunately breaks existing code, but I think it's the semantically correct approach.

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Gizmos Visual editor and debug gizmos D-Trivial Nice and easy! A great choice to get started with Bevy C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon and removed S-Needs-Triage This issue needs to be labelled C-Enhancement A new feature labels May 21, 2024
@Olle-Lukowski
Copy link
Contributor

I'll get started on implementing this, probably tomorrow.

github-merge-queue bot pushed a commit that referenced this issue May 27, 2024
# Objective

Fixes #13427.

## Solution

I changed the traits, and updated all usages.

## Testing

The `render_primitives` example still works perfectly.

---

## Changelog

- Made `gizmos.primitive_2d()` and `gizmos.primitive_3d()` take the
primitives by ref.

## Migration Guide

- Any usages of `gizmos.primitive_2d()` and/or `gizmos.primitive_3d()`
need to be updated to pass the primitive in by reference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Gizmos Visual editor and debug gizmos C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A simple quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants