Skip to content

Retained Gizmos #15473

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

Merged
merged 36 commits into from
Dec 4, 2024
Merged

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Sep 27, 2024

Objective

Add a way to use the gizmo API in a retained manner, for increased performance.

Solution

  • Move gizmo API from Gizmos to GizmoBuffer, abusing Deref to keep usage the same as before.
  • Merge non-strip and strip variant of LineGizmo into one, storing the data in a GizmoBuffer to have the same API for retained LineGizmos.

Review guide

  • The meat of the changes are in lib.rs, retained.rs, gizmos.rs, pipeline_3d.rs and pipeline_2d.rs
  • The other files contain almost exclusively the churn from moving the gizmo API from Gizmos to GizmoBuffer

Testing

Performance

Performance compared to the immediate mode API is from 65 to 80 times better for static lines.

7900 XTX, 3700X
1707.9k lines/ms: gizmos_retained (21.3ms)
3488.5k lines/ms: gizmos_retained_continuous_polyline (31.3ms)
   0.5k lines/ms: gizmos_retained_separate (97.7ms)

3054.9k lines/ms: bevy_polyline_retained_nan (16.8ms)
3596.3k lines/ms: bevy_polyline_retained_continuous_polyline (14.2ms)
   0.6k lines/ms: bevy_polyline_retained_separate (78.9ms)

  26.9k lines/ms: gizmos_immediate (14.9ms)
  43.8k lines/ms: gizmos_immediate_continuous_polyline (18.3ms)

Looks like performance is good enough, being close to par with bevy_polyline.

Benchmarks can be found here:
This branch: https://github.com/tim-blackbird/line_racing/tree/retained-gizmos
Bevy 0.14: https://github.com/DGriffin91/line_racing

Showcase

fn setup(
    mut commands: Commands,
    mut gizmo_assets: ResMut<Assets<GizmoAsset>>
) {
    let mut gizmo = GizmoAsset::default();

    // A sphere made out of one million lines!
    gizmo
        .sphere(default(), 1., CRIMSON)
        .resolution(1_000_000 / 3);

    commands.spawn(Gizmo {
        handle: gizmo_assets.add(gizmo),
        ..default()
    });
}

Follow-up work

  • Port over to the retained rendering world proper
  • Calculate visibility and cull Gizmos

@tim-blackbird tim-blackbird added C-Feature A new feature, making something new possible A-Gizmos Visual editor and debug gizmos labels Sep 27, 2024
@IceSentry IceSentry self-requested a review September 27, 2024 16:31
@alice-i-cecile alice-i-cecile added M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 27, 2024
@tim-blackbird

This comment was marked as outdated.

@tim-blackbird tim-blackbird marked this pull request as ready for review September 30, 2024 12:56
.linestrip_2d(strip.map(|v| self.relative(v)), color);
]
.map(|v| self.relative(v));
self.draw.linestrip_2d(strip, color);
Copy link
Contributor Author

@tim-blackbird tim-blackbird Sep 30, 2024

Choose a reason for hiding this comment

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

This shows an instance where the use of DerefMut can cause compilation errors.
This issue of partial borrows is already a problem for the Mut returned by mutable query items for example. (Polonius pls 🙏)

@tim-blackbird tim-blackbird added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 30, 2024
@tim-blackbird tim-blackbird changed the title Retained LineGizmos WIP Retained LineGizmos Sep 30, 2024
@tychedelia tychedelia self-requested a review October 6, 2024 16:21
@IceSentry
Copy link
Contributor

I kinda like GizmoAsset because it uses the asset system and it doesn't have the retained naming, but I'm honestly not sure either.

@tychedelia
Copy link
Member

I kinda like GizmoAsset because it uses the asset system and it doesn't have the retained naming, but I'm honestly not sure either.

We just don't use FooAsset anywhere else, do we?

@tim-blackbird

This comment was marked as outdated.

@pablo-lua
Copy link
Contributor

Any thoughts on alternative names for LineGizmo?

None, but I think Gizmo is a good idea to follow.

@tim-blackbird tim-blackbird added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 10, 2024
@tim-blackbird
Copy link
Contributor Author

tim-blackbird commented Oct 12, 2024

I've thought about it a bit more and decided to go through with removing the 'line' naming from this PR, see the Rename commit(dbdb9ad) for the changes to naming and docs

@tim-blackbird tim-blackbird changed the title Retained LineGizmos Retained Gizmos Oct 12, 2024
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Oct 15, 2024
@alice-i-cecile
Copy link
Member

Bumping to 0.16, sorry. Rendering's stability is too poor to merge this right now.

@aevyrie
Copy link
Member

aevyrie commented Oct 16, 2024

This is really nice.

I realize this is a bikeshed, but I'd like to point out that the name Gizmo truly makes no sense any more. This is just polyline rendering with an immediate and retained API, and helpers to convert primitive shapes into polylines.

@alice-i-cecile
Copy link
Member

@tim-blackbird I like this and it's well-reviewed. Can you resolve merge conflicts and ping me to merge please?

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 3, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 4, 2024
Merged via the queue into bevyengine:main with commit d2a07f9 Dec 4, 2024
28 checks passed
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
# Objective
Add a way to use the gizmo API in a retained manner, for increased
performance.

## Solution
- Move gizmo API from `Gizmos` to `GizmoBuffer`, ~ab~using `Deref` to
keep usage the same as before.
- Merge non-strip and strip variant of `LineGizmo` into one, storing the
data in a `GizmoBuffer` to have the same API for retained `LineGizmo`s.

### Review guide
- The meat of the changes are in `lib.rs`, `retained.rs`, `gizmos.rs`,
`pipeline_3d.rs` and `pipeline_2d.rs`
- The other files contain almost exclusively the churn from moving the
gizmo API from `Gizmos` to `GizmoBuffer`

## Testing
### Performance

Performance compared to the immediate mode API is from 65 to 80 times
better for static lines.

```
7900 XTX, 3700X
1707.9k lines/ms: gizmos_retained (21.3ms)
3488.5k lines/ms: gizmos_retained_continuous_polyline (31.3ms)
   0.5k lines/ms: gizmos_retained_separate (97.7ms)

3054.9k lines/ms: bevy_polyline_retained_nan (16.8ms)
3596.3k lines/ms: bevy_polyline_retained_continuous_polyline (14.2ms)
   0.6k lines/ms: bevy_polyline_retained_separate (78.9ms)

  26.9k lines/ms: gizmos_immediate (14.9ms)
  43.8k lines/ms: gizmos_immediate_continuous_polyline (18.3ms)
```
Looks like performance is good enough, being close to par with
`bevy_polyline`.

Benchmarks can be found here: 
This branch:
https://github.com/tim-blackbird/line_racing/tree/retained-gizmos
Bevy 0.14: https://github.com/DGriffin91/line_racing

## Showcase
```rust 
fn setup(
    mut commands: Commands,
    mut gizmo_assets: ResMut<Assets<GizmoAsset>>
) {
    let mut gizmo = GizmoAsset::default();

    // A sphere made out of one million lines!
    gizmo
        .sphere(default(), 1., CRIMSON)
        .resolution(1_000_000 / 3);

    commands.spawn(Gizmo {
        handle: gizmo_assets.add(gizmo),
        ..default()
    });
}
```

## Follow-up work
- Port over to the retained rendering world proper
- Calculate visibility and cull `Gizmo`s
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1963 if you'd like to help out.

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-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants