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

Contextually clearing gizmos #10973

Merged
merged 13 commits into from
Apr 23, 2024
Merged

Conversation

Aceeri
Copy link
Member

@Aceeri Aceeri commented Dec 13, 2023

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.

@Aceeri Aceeri added C-Bug An unexpected or incorrect behavior X-Controversial There is active debate or serious implications around merging this PR A-Gizmos Visual editor and debug gizmos labels Dec 13, 2023
@Aceeri Aceeri marked this pull request as ready for review December 13, 2023 21:41
@nicopap
Copy link
Contributor

nicopap commented Dec 14, 2023

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.

@nicopap nicopap mentioned this pull request Dec 14, 2023
57 tasks
@Aceeri
Copy link
Member Author

Aceeri commented Dec 14, 2023

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.

Copy link
Contributor

@SpecificProtagonist SpecificProtagonist left a comment

Choose a reason for hiding this comment

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

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.

crates/bevy_gizmos/src/gizmos.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/gizmos.rs Outdated Show resolved Hide resolved
SpecificProtagonist

This comment was marked as resolved.

@jeliag
Copy link
Contributor

jeliag commented Dec 22, 2023

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.

.init_resource::<GizmoStorage>()
.add_systems(Last, update_gizmo_meshes)
.init_resource::<GizmoStorage<Swap>>()
.init_resource::<GizmoStorage<()>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple questions.

crates/bevy_gizmos/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

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.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 25, 2024
@Aceeri
Copy link
Member Author

Aceeri commented Feb 26, 2024

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
Copy link
Contributor

SpecificProtagonist commented Feb 26, 2024

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 Aceeri force-pushed the fixed-update-gizmos branch 2 times, most recently from ad64888 to feba79f Compare March 1, 2024 01:20
@Aceeri
Copy link
Member Author

Aceeri commented Mar 1, 2024

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
Copy link
Member Author

Aceeri commented Mar 10, 2024

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

@Aceeri
Copy link
Member Author

Aceeri commented Mar 11, 2024

Should be ready for merging/final reviews now

Fix bug of strips getting extended by list

Document GizmoStorage

Don't import bevy_internal

Formatting

Move clearing the gizmos to FixedLast

Fix some ordering issues with clearing/collecting

Rename Context to Clear

Formatting

Use mem::swap for changing clear context contents

Revert examples

Improve documentation, explain how to setup a custom context

Hide Swap buffer, improve documentation for update_gizmo_meshes

Reword again

more rewording
@alice-i-cecile alice-i-cecile added M-Needs-Release-Note Work that should be called out in the blog due to impact and removed X-Controversial There is active debate or serious implications around merging this PR labels Apr 21, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 23, 2024
Merged via the queue into bevyengine:main with commit b1ab036 Apr 23, 2024
31 checks passed
@Aceeri Aceeri deleted the fixed-update-gizmos branch May 4, 2024 01:28
@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#1301 if you'd like to help out.

@BD103 BD103 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 12, 2024
Copy link
Contributor

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
Copy link
Member

BD103 commented Jul 12, 2024

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 :)

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-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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