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.

Sign up for GitHub

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

Jump to bottom

Fix screenshot example #15094

Merged
merged 6 commits into from
Sep 9, 2024
Merged

Fix screenshot example #15094

merged 6 commits into from
Sep 9, 2024

Conversation

akimakinai
Copy link
Contributor

@akimakinai akimakinai commented Sep 8, 2024 •
edited
Loading

Objective

I noticed some issues in screenshot example:

  1. Cursor icon won't return from SystemCursorIcon::Progress to default icon, even though screen shot saving is done.
  2. Panics when exiting window: called `Result::unwrap()` on an `Err` value: NoEntities("bevy_ecs::query::state::QueryState<bevy_ecs::entity::Entity, bevy_ecs::query::filter::With<bevy_window::window::Window>>")

Solution

  1. Caused by cursor updating system not responding to CursorIcon component removal. I believe it should, so change it to react to RemovedComponents<CursorIcon>. (a suggestion)
  2. Use get_single for window.

Testing

  • run screenshot example

@akimakinai akimakinai changed the title Fix screenshot example (Cursor icon removed Fix screenshot example Sep 8, 2024
@IQuick143 IQuick143 added C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Rendering Drawing game state to the screen A-Windowing Platform-agnostic interface layer to run your app in labels Sep 8, 2024
alice-i-cecile
alice-i-cecile approved these changes Sep 8, 2024
View reviewed changes
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 fixes, thank you!

janhohenheim
janhohenheim suggested changes Sep 8, 2024
View reviewed changes
Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Looks nearly ready, thanks! I'd appreciate if you could use an observer here, so we don't have to fix this usage of RemovedComponents when we deprecate it :)

crates/bevy_render/src/view/window/cursor.rs Outdated Show resolved Hide resolved
@janhohenheim janhohenheim 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 Sep 8, 2024
@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Sep 9, 2024
@alice-i-cecile
Copy link
Member

@akimakinai it looks like I didn't resolve merge conflicts fully correctly in the web UI: there's an unused import. When you get a chance can you fix this locally and push the changes? Once that's done I'll merge this in.

auto-merge was automatically disabled September 9, 2024 16:34

Head branch was pushed to by a user without write access

@akimakinai
Copy link
Contributor Author

Done. Removed use and moved app.observe to CursorPlugin added in #15078.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 9, 2024
Merged via the queue into bevyengine:main with commit bafffe1 Sep 9, 2024
26 checks passed
@akimakinai akimakinai deleted the cursor_icon_removed branch September 10, 2024 00:22
@akimakinai akimakinai mentioned this pull request Sep 28, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Sep 28, 2024
@akimakinai
Use try_insert in on_remove_cursor_icon (#15492)
4a013b6 
# Objective

- Fixes #15490 introduced in #15094.

## Solution

- Use non-panicking `try_insert`

## Testing

- Closing window with `CursorIcon` no longer crashes after this change
(confirmed with `window_settings` example)
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
@akimakinai @robtfm
Use try_insert in on_remove_cursor_icon (bevyengine#15492)
27f7c97 
# Objective

- Fixes bevyengine#15490 introduced in bevyengine#15094.

## Solution

- Use non-panicking `try_insert`

## Testing

- Closing window with `CursorIcon` no longer crashes after this change
(confirmed with `window_settings` example)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@eero-lehtinen eero-lehtinen eero-lehtinen approved these changes

@janhohenheim janhohenheim janhohenheim approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@akimakinai @alice-i-cecile @eero-lehtinen @janhohenheim @IQuick143