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.

Sign up for GitHub

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

Jump to bottom

Add First/Pre/Post/Last schedules to the Fixed timestep #10977

Merged
merged 4 commits into from
Dec 14, 2023
Merged

Add First/Pre/Post/Last schedules to the Fixed timestep #10977

merged 4 commits into from
Dec 14, 2023

Conversation

Aceeri
Copy link
Member

@Aceeri Aceeri commented Dec 13, 2023 •
edited
Loading

Fixes #10974

Objective

Duplicate the ordering logic of the Main schedule into the FixedMain schedule.

Changelog

  • FixedUpdate is no longer the main schedule ran in RunFixedUpdateLoop, FixedMain has replaced this and has a similar structure to Main.

Migration Guide

  • Usage of RunFixedUpdateLoop should be renamed to RunFixedMainLoop.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Time Involves time keeping and reporting labels Dec 13, 2023
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Dec 13, 2023
@alice-i-cecile
Copy link
Member

Companion to #10969, which does a similar transformation for startup schedules.

@Aceeri Aceeri mentioned this pull request Dec 13, 2023
Merged
3 tasks
maniwani
maniwani approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@maniwani maniwani left a comment •
edited
Loading

Choose a reason for hiding this comment

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

FixedMain and Main carve up the same timeline in different ways (fixed vs. variable-sized steps), so it makes sense that they would have the same structure.

@rlidwka
Copy link
Contributor

rlidwka commented Dec 13, 2023

Startup has different structure, there are no equivalents for First and Last there

what's the reason for the disparity?

@maniwani
Copy link
Contributor

maniwani commented Dec 13, 2023 •
edited
Loading

What's the reason for the disparity?

It seems like it'd be better to ask this in the other PR. Especially if it's something that's blocking you.

@rlidwka
Copy link
Contributor

rlidwka commented Dec 14, 2023 •
edited
Loading

It seems like it'd be better to ask this in the other PR.

I can ask "why First doesn't exist for Startup", and that question belongs to other PR. I can ask "why First exists for FixedUpdate", and that question belongs here. But that's the same fundamental question. Is Startup very different from Main and Fixed, such that first/last don't make sense there? Or is Main very different from Startup and Fixed, such that first/last only make sense there?

This PR is the one that adds FixedFirst and FixedLast. It seems reasonable to ask why. Especially if I prefer to err on the side of not adding things if in doubt, since it's much easier to add things later than to remove them.

@Aceeri
Copy link
Member Author

Aceeri commented Dec 14, 2023 •
edited
Loading

It seems like it'd be better to ask this in the other PR.

I can ask "why First doesn't exist for Startup", and that question belongs to other PR. I can ask "why First exists for FixedUpdate", and that question belongs here. But that's the same fundamental question. Is Startup very different from Main and Fixed, such that first/last don't make sense there? Or is Main very different from Startup and Fixed, such that first/last only make sense there?

This PR is the one that adds FixedFirst and FixedLast. It seems reasonable to ask why. Especially if I prefer to err on the side of not adding things if in doubt, since it's much easier to add things later than to remove them.

The only difference between fixed and main is that one is variable timing vs a set tick rate. Ideally all gameplay logic is in FixedMain, while UI/rendering/input related things are in Main. I would say First/Last exist here for the same reason it exists in Main, so if we want to remove it here, then we should remove it in Main as well.

Startup is different in that it is just a setup tool for spawning scenes/ui/whatever or inserting resources or something. I don't have much objection to First/Last in Startup but I don't see much motivation for it currently.

We mostly say this is better talked about in the other one because it is effectively the same thing as this PR but for Startup.

@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 Dec 14, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 14, 2023
Merged via the queue into bevyengine:main with commit fe28e0e Dec 14, 2023
@Aceeri Aceeri deleted the fixed-update-inner-schedules branch December 14, 2023 05:23
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Apr 21, 2024
github-merge-queue bot pushed a commit that referenced this pull request Apr 23, 2024
@Aceeri
Contextually clearing gizmos (#10973)
b1ab036 
# 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
- [x] `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: #10977
- [x] use mem::take internally for the swaps?
- [x] Better name for the `Context` generic on gizmos? `Clear`?

---

## Changelog
- Gizmos drawn in `FixedMain` now last until the next `FixedMain`
iteration runs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@maniwani maniwani maniwani approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events A-Time Involves time keeping and reporting C-Usability A targeted quality-of-life change that makes Bevy easier to use 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
Milestone
0.13
Development

Successfully merging this pull request may close these issues.

Add support for PreFixedUpdate/PostFixedUpdate
4 participants
@Aceeri @alice-i-cecile @rlidwka @maniwani