Contextually clearing gizmos

[Aceeri]

Objective

Allow Gizmos to work in FixedUpdate without any changes needed. This changes Gizmos from being a purely immediate mode api, but allows the user to use it as if it were an immediate mode API regardless of schedule context.

Also allows for extending by other custom schedules by adding their own GizmoStorage<Clear> and the requisite systems:

• propagate_gizmos::<Clear> before update_gizmo_meshes
• stash_default_gizmos when starting a clear context
• pop_default_gizmos when ending a clear context
• collect_default_gizmos when grabbing the requested gizmos
• clear_gizmos for clearing the context's gizmos

Solution

Adds a generic to Gizmos that defaults to Update (the current way gizmos works). When entering a new clear context the default Gizmos gets swapped out for that context's duration so the context can collect the gizmos requested.

Prior work: #9153

To do

• FixedUpdate should probably get its own First, Pre, Update, Post, Last system sets for this. Otherwise users will need to make sure to order their systems before clear_gizmos. This could alternatively be fixed by moving the setup of this to bevy_time::fixed?
  PR to fix this issue: Add First/Pre/Post/Last schedules to the Fixed timestep #10977
• use mem::take internally for the swaps?
• Better name for the Context generic on gizmos? Clear?

---

Changelog

• Gizmos drawn in FixedMain now last until the next FixedMain iteration runs.

[nicopap]

Could this PR work as an extension on #10342 ? Since it does create special configurations to handle different types of gizmos… I'll ping @jeliag to get their input.

[Aceeri]

Could this PR work as an extension on #10342 ? Since it does create special configurations to handle different types of gizmos… I'll ping @jeliag to get their input.

I believe it's two different axis of control here, we could use the same generic since this uses it purely as a marker but I don't personally think it's worthwhile. In most cases this should default to Fixed or Update depending on system position and users shouldn't even know its there.

[SpecificProtagonist]

Looks like a clean solution to me. Technically less flexible for extension than the approach of exposing GizmoBuffer as users can only store gizmo buffers based on their type instead of as values, but I think that's sufficient. 👍

Was gonna point out that dropping the swap allocation isn't necessary but it got fixed under my nose :3

Could you also mention in the doc comment of update_gizmo_meshes that this clears GizmoStorage<Update>? Just for making the data flow clear as other gizmo storages persist.

[jeliag]

Could this PR work as an extension on #10342 ? Since it does create special configurations to handle different types of gizmos… I'll ping @jeliag to get their input.

I see the similarities, and it could be integrated into my config groups, but I do not think it should be. Ideally the gizmos should behave the same regardless of where they are used. Linking gizmo appearance settings and where they can be used doesn't seem right to me.

I think this PR is a step in the right direction. A future retained gizmo API could solve this problem as well, but that would probably require a major refactoring of the entire gizmo code.

[nicopap]

Looks like initially, the default buffer had the Update type, now it uses (). What's the reasoning behind changing it? I thought it was clearer.

[Aceeri]

Well, I don't really care much either way. This one gets swapped out for other buffers, so someone looking at that might get confused now that gizmos requested in a GizmoStorage<Update> are cleared in FixedMain.

Ideally I guess it'd be that we have both () and Update but I don't think the complexity of adding that is worthwhile. Maybe if we wanted to add the ability to query Gizmos<Update> inside other contexts I suppose?

[nicopap]

LGTM, just a couple questions.

[MiniaczQ]

I like the user interface, seems pretty robust for custom contexts, persistent gizmos #9178 should cover other cases.
The internals are a bit weird, but I don't have a better idea than introducing GizmoStorage<Update> like you mention.

One question.
The current context uses () (which doubles as Update) and we store the Update in Swap when using FixedUpdate.
Wouldn't another level of context, say FixedUpdateTurn (in some imaginary turn-based game) break this?
Since the previous context of FixedUpdate would need to move from () to Swap, which currently holds Update?

If so, it's probably worth noting, it'd be pretty hard to debug.

[Aceeri]

I like the user interface, seems pretty robust for custom contexts, persistent gizmos #9178 should cover other cases. The internals are a bit weird, but I don't have a better idea than introducing GizmoStorage<Update> like you mention.

One question. The current context uses () (which doubles as Update) and we store the Update in Swap when using FixedUpdate. Wouldn't another level of context, say FixedUpdateTurn (in some imaginary turn-based game) break this? Since the previous context of FixedUpdate would need to move from () to Swap, which currently holds Update?

If so, it's probably worth noting, it'd be pretty hard to debug.

Definitely is a bit hardcoded to just 1 layer of context at the moment. We could solve that by making this stack-based, but I was a bit hesitant about going down that path because I was worried about over-engineering it.

I'll add a comment regardless, I could also add a hard panic by making the swap buffer never overwrite.

[SpecificProtagonist]

I agree that adding stack machinery insn't necessary – if a user nests a context, they can store it in another buffer the same way this PR does it.

I'd say the better solution is to clarify that Swap is meant to be used just for saving the default context during FixedUpdate, which is why Aceeri proposed (in another comment) making it pub(crate) and updating it's documentation (maybe it could also be renamed to FixedUpdateSwap?).

Edit: stash_default_gizmos/pop_default_gizmos would need the same treatment as they're specifically for Swap.

[Aceeri]

I agree that adding stack machinery insn't necessary – if a user nests a context, they can store it in another buffer the same way this PR does it.

I'd say the better solution is to clarify that Swap is meant to be used just for saving the default context during FixedUpdate, which is why Aceeri proposed (in another comment) making it pub(crate) and updating it's documentation (maybe it could also be renamed to FixedUpdateSwap?).

Edit: stash_default_gizmos/pop_default_gizmos would need the same treatment as they're specifically for Swap.

Yeah this seems like a better idea, I'll make the stash/pop take a generic for a swap buffer. I can't make Swap pub(crate) unfortunately because of some rust rules around parameters (even though no one should be calling a system manually).

[Aceeri]

Shouldn't be merged quite yet given some changes with the config stuff.

[Aceeri]

Should be ready for merging/final reviews now

[alice-i-cecile]

I'm more convinced now: the added complexity isn't too bad, and it's worth it for the consistency in developer experience.

[alice-i-cecile]

@Aceeri once this is passing CI I'll merge. Looks like it just needs formatting currently.

[alice-i-cecile]

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#1301 if you'd like to help out.

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

[BD103]

Just marking this as a breaking change so the migration guide tool picks it up. (insert_gizmo_group() was renamed to insert_gizmo_config().) Don't worry about filling in the migration guide yourself, I'll handle it :)