Skip to content

Bubble sync points if ignore_deferred, do not ignore if target system is exclusive #17880

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

@urben1680 urben1680 commented Feb 16, 2025

Objective

Fixes #17828

This fixes two bugs:

  1. Exclusive systems should see the effect of all commands queued to that point. That does not happen when the system is configured with *_ignore_deferred which may lead to surprising situations. These configurations should not behave like that.
  2. If *_ignore_deferred is used, no sync point may be added at all after the config. Currently this can happen if the last nodes in that config have no deferred parameters themselves. Instead, sync points should always be added after such a config, so long systems have deferred parameters.

Solution

  1. When adding sync points on edges, do not consider AutoInsertApplyDeferredPass::no_sync_edges if the target is an exclusive system.
  2. when going through the nodes in a directed way, store the information that AutoInsertApplyDeferredPass::no_sync_edges suppressed adding a sync point at the target node. Then, when the target node is evaluated later by the iteration and that prior suppression was the case, the target node will behave like it has deferred parameters even if the system itself does not.

Testing

I added a test for each bug, please let me know if more are wanted and if yes, which cases you would want to see.
These tests also can be read as examples how the current code would fail.

@urben1680
Copy link
Contributor Author

Tagging @hymm as the original author of the modified file. 👋

@alice-i-cecile alice-i-cecile 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 Feb 16, 2025
@alice-i-cecile alice-i-cecile requested a review from hymm February 16, 2025 19:31
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'd like the tests to be more explicit about what the final inferred schedule looks like, and why that schedule is correct.

@urben1680
Copy link
Contributor Author

urben1680 commented Feb 16, 2025

I'd like the tests to be more explicit about what the final inferred schedule looks like, and why that schedule is correct.

As in doing more assertions on the schedule's internal values or adding more comments?

Btw I think there is some other problem to address. As it is now, bubbling a sync point further up means more than one sync point could be generated if there is more than one outgoing edge with each different distances. But that makes no sense, the original system that wants it's commands applied needs only one sync point.

I try to think what to do here. Until then I put this one into draft again.

@urben1680 urben1680 marked this pull request as draft February 16, 2025 22:02
@alice-i-cecile
Copy link
Member

More comments mostly :) I had a hard time following what the internal logic of the tests were, and why they were correct.

@urben1680
Copy link
Contributor Author

I was unable to trigger any issues I assumed to be there so I decided to not change my approach.

@urben1680 urben1680 marked this pull request as ready for review February 17, 2025 20:32
@andriyDev andriyDev self-requested a review February 20, 2025 01:57
@urben1680 urben1680 requested a review from andriyDev February 20, 2025 20:20
Copy link
Contributor

@andriyDev andriyDev left a comment

Choose a reason for hiding this comment

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

Great change!

@urben1680
Copy link
Contributor Author

urben1680 commented Feb 25, 2025

Merged @andriyDev's changes into this PR. I must confess both our changes together make this hard to understand. I probably will try to make everything clearer, maybe with more comments, other variable names or such stuff.

If someone has good ideas to make this more clear, please write them in a comment. 😃

EDIT: Done with my ideas, can be reviewed now

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.

Good test comments now! Thanks for tackling some cleanup work while you were here; I agree that this was getting quite tricky to follow 😅

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 26, 2025
@alice-i-cecile alice-i-cecile 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 Feb 26, 2025
Merged via the queue into bevyengine:main with commit 2f38464 Feb 26, 2025
31 checks passed
@urben1680 urben1680 deleted the bubble-sync-points-when-ignored-and-do-not-ignore-prior-exclusive-systems branch February 26, 2025 21:32
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.

Unflushed commands before exclusive system when configured with *_ignore_deferred
4 participants