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

Rewrite part of Commands rustdoc #11141

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Conversation

stepancheg
Copy link
Contributor

  • Explain it is flushed in the same schedule run (was not obvious to me)
  • Point to apply_deferred example
  • Remove mentions of System::apply_deferred and Schedule::apply_deferred which are probably too low level for the most users

@hymm
Copy link
Contributor

hymm commented Dec 30, 2023

With #9822 merged the story here is a little more complex now.

@stepancheg
Copy link
Contributor Author

Suggestions how to rephrase it?

I submit PRs to rustdoc comments, when I'm trying to understand something, and apparently I don't always understand it correctly.

Also that PR says:

This PR auto inserts sync points if there are deferred buffers on a system and there are dependents on that system (systems with after relationships).

Doesn't it mean that apply_deferred example is outdated — apply_deferred call is no longer needed here?

(
despawn_old_and_spawn_new_fruits,
// We encourage adding apply_deferred to a custom set
// to improve diagnostics. This is optional, but useful when debugging!
apply_deferred.in_set(CustomFlush),
count_apple,
)
.chain(),

@hymm
Copy link
Contributor

hymm commented Dec 30, 2023

Doesn't it mean that apply_deferred example is outdated — apply_deferred call is no longer needed here?

Yeah probably. You can still add apply_deferred manually and the algorithm will respect it, but the algo doesn't merge manually added sync points together only the auto added ones. For that reason I would not recommend not adding sync points in manually. For the example it might make sense to make a new schedule that disables auto inserted sync points. That or just delete it. That should be done in a separate PR of course.

@hymm
Copy link
Contributor

hymm commented Dec 30, 2023

Suggestions how to rephrase it?

I think the main point of emphasis should be that it's enough to add a before/after relationship between the systems for them to see the effect of commands.

@stepancheg
Copy link
Contributor Author

stepancheg commented Dec 30, 2023

Changed the docs — added dependencies between systems, and moved it to apply_deferred.

@stepancheg stepancheg force-pushed the commands branch 3 times, most recently from 768a2d8 to b4a9baa Compare December 30, 2023 05:55
@matiqo15 matiqo15 added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Dec 30, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jan 8, 2024
# Objective

Re this comment:
#11141 (comment)

Since #9822, Bevy automatically
inserts `apply_deferred` between systems with dependencies where needed,
so manually inserted `apply_deferred` doesn't to anything useful, and in
current state this example does more harm than good.

## Solution

The example can be modified with removal of automatic `apply_deferred`
insertion, but that would immediately upgrade this example from beginner
level, to upper intermediate. Most users don't need to disable automatic
sync point insertion, and remaining few who do probably already know how
it works.

CC @hymm
@TrialDragon TrialDragon 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 Mar 5, 2024
@james7132 james7132 enabled auto-merge March 5, 2024 09:45
@james7132 james7132 added this pull request to the merge queue Mar 5, 2024
Merged via the queue into bevyengine:main with commit 70b70cd Mar 5, 2024
25 checks passed
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
- Explain it is flushed in the same schedule run (was not obvious to me)
- Point to `apply_deferred` example
- Remove mentions of `System::apply_deferred` and
`Schedule::apply_deferred` which are probably too low level for the most
users

Co-authored-by: James Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation 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.

6 participants