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

[Merged by Bors] - Ensure 2D phase items are sorted before batching #5942

Closed
wants to merge 1 commit into from

Conversation

asherkin
Copy link
Contributor

Objective

Without this we can inappropriately merge batches together without properly accounting for non-batch items between them, and the merged batch will then be sorted incorrectly later.

This change seems to reliably fix the issue I was seeing in #5919.

Solution

Ensure the batch_phase_system runs after the sort_phase_system, so that batching can only look at actually adjacent phase items.

Without this we can inappropriately merge batches together without
properly accounting for non-batch items between them, and the merged
batch will then be sorted incorrectly later.
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Sep 10, 2022
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.

This fix makes sense to me, and is minimally intrusive.

@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Sep 10, 2022
@mockersf
Copy link
Member

Does it make sense to batch a not sorted phase? If not, could the batch system sort by default before batching, and we could remove the independent sort system when batching?

@asherkin
Copy link
Contributor Author

I was digging through the code that added this (#3460) to try and answer that, and it looks like these systems were originally ordered as I've done here, which was lost in the camera driven rendering rework (#4745). I can't see any discussion about why the systems are separate, and the later removal of the ordering appears to have been accidental when the code was moved (no mention of it in any discussion I could see).

@alice-i-cecile
Copy link
Member

Thanks for the digging. Merging as trivial then; seems like a merge conflict error or something similiarly trivial.

bors r+

bors bot pushed a commit that referenced this pull request Sep 11, 2022
# Objective

Without this we can inappropriately merge batches together without properly accounting for non-batch items between them, and the merged batch will then be sorted incorrectly later.

This change seems to reliably fix the issue I was seeing in #5919.

## Solution

Ensure the `batch_phase_system` runs after the `sort_phase_system`, so that batching can only look at actually adjacent phase items.
@bors bors bot changed the title Ensure 2D phase items are sorted before batching [Merged by Bors] - Ensure 2D phase items are sorted before batching Sep 11, 2022
@bors bors bot closed this Sep 11, 2022
@superdump
Copy link
Contributor

For historical context, I’d have approved this too.

@mockersf
Copy link
Member

@superdump does it make sense to merge those two systems in one when batching? that would remove the code of having two systems to call, both iterating the same list

nicopap pushed a commit to nicopap/bevy that referenced this pull request Sep 12, 2022
# Objective

Without this we can inappropriately merge batches together without properly accounting for non-batch items between them, and the merged batch will then be sorted incorrectly later.

This change seems to reliably fix the issue I was seeing in bevyengine#5919.

## Solution

Ensure the `batch_phase_system` runs after the `sort_phase_system`, so that batching can only look at actually adjacent phase items.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Without this we can inappropriately merge batches together without properly accounting for non-batch items between them, and the merged batch will then be sorted incorrectly later.

This change seems to reliably fix the issue I was seeing in bevyengine#5919.

## Solution

Ensure the `batch_phase_system` runs after the `sort_phase_system`, so that batching can only look at actually adjacent phase items.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Without this we can inappropriately merge batches together without properly accounting for non-batch items between them, and the merged batch will then be sorted incorrectly later.

This change seems to reliably fix the issue I was seeing in bevyengine#5919.

## Solution

Ensure the `batch_phase_system` runs after the `sort_phase_system`, so that batching can only look at actually adjacent phase items.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants