Skip to content

Update changed cameras in camera_system.#5694

Closed
HackerFoo wants to merge 1 commit intobevyengine:mainfrom
HackerFoo:camera_changes
Closed

Update changed cameras in camera_system.#5694
HackerFoo wants to merge 1 commit intobevyengine:mainfrom
HackerFoo:camera_changes

Conversation

@HackerFoo
Copy link
Contributor

Objective

A Camera should be updated by camera_system whenever it changes, not just when added.

Solution

Replace the Added query filter with Changed.

@HackerFoo HackerFoo added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior labels Aug 14, 2022
@tim-blackbird
Copy link
Contributor

This system already updates every camera where the target or projection was changed.

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 change makes sense to me, and based on my understanding of the logic seems correct. Other fields may change on cameras in surprising ways, and we should be sure to catch those.

.target
.is_changed(&changed_window_ids, &changed_image_handles)
|| added_cameras.contains(&entity)
|| changed_cameras.contains(&entity)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
|| changed_cameras.contains(&entity)
|| camera.is_changed()

Using is_changed on camera makes the second query unnecessary.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

@HackerFoo do you mind addressing @devil-ira's comment? This would be an approval if it were not for that one potential change.

@tim-blackbird
Copy link
Contributor

This was superceded by #5945

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