Skip to content

No schedule build pass overwrite if build settings do not change auto_insert_apply_deferred from true #19217

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

Conversation

urben1680
Copy link
Contributor

Objective

Fixes #18790.
Simpler alternative to #19195.

Solution

As suggested by @PixelDust22, simply avoid overwriting the pass if the schedule already has auto sync points enabled.
Leave pass logic untouched.

It still is probably a bad idea to add systems/set configs before changing the build settings, but that is not important as long there are no more complex build passes.

Testing

Added a test.

@urben1680 urben1680 changed the title no pass overwrite if auto_insert_apply_deferred = true as present No schedule build pass overwrite if build settings do not change auto_insert_apply_deferred from true May 15, 2025
@urben1680
Copy link
Contributor Author

I don't know what is up with that unrelated, failed CI check.

@urben1680 urben1680 marked this pull request as draft May 16, 2025 06:46
@urben1680 urben1680 marked this pull request as ready for review May 16, 2025 17:02
Copy link
Member

@Vrixyz Vrixyz left a comment

Choose a reason for hiding this comment

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

minor comment on doc links, this PR has sensible explaination, tests and documentation, I'm approving :)

@Vrixyz Vrixyz added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 17, 2025
improve docs

Co-authored-by: Thierry Berger <contact@thierryberger.com>
@andriyDev andriyDev 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 May 19, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16.1 milestone May 19, 2025
Copy link
Contributor

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

@urben1680
Copy link
Contributor Author

urben1680 commented May 21, 2025

@PixelDust22 I decided taking the extra api away entirely. You are right that the contains_pass is quite useless as a public method. But adding get(mut) methods to get pass references requires ScheduleBuildPassObj to have an extra Any bound and to increase Minimum Supported Rust Version.

I think that is too much for a bugfix that does not need these methods that much. I also don't want to add new methods to ScheduleBuildPassObj until #18984 removes them again for 0.17.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 26, 2025
Merged via the queue into bevyengine:main with commit b92c0eb May 26, 2025
32 checks passed
mockersf pushed a commit that referenced this pull request May 30, 2025
…o_insert_apply_deferred` from `true` (#19217)

# Objective

Fixes #18790.
Simpler alternative to #19195.

## Solution

As suggested by @PixelDust22, simply avoid overwriting the pass if the
schedule already has auto sync points enabled.
Leave pass logic untouched.

It still is probably a bad idea to add systems/set configs before
changing the build settings, but that is not important as long there are
no more complex build passes.

## Testing

Added a test.

---------

Co-authored-by: Thierry Berger <contact@thierryberger.com>
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-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

Reinserting a schedule's unchanged ScheduleBuildSettings forces sync points insertions despite *_ignore_deferred configs
5 participants