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

[Merged by Bors] - Allow minimising in 2d #4527

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Allow minimising in 2d #4527

wants to merge 2 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Apr 18, 2022 •
edited
Loading

Objective

Solution

@DJMcNab DJMcNab added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in P-Regression Functionality that used to work but no longer does. Add a test for this! labels Apr 18, 2022
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 18, 2022
@alice-i-cecile alice-i-cecile removed the S-Needs-Triage This issue needs to be labelled label Apr 18, 2022
This was referenced Apr 26, 2022
Closed
Closed
superdump
superdump requested changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Hmm, I am wondering what view entity is passed in if there is no such entity matching the query.

crates/bevy_core_pipeline/src/main_pass_2d.rs Outdated Show resolved Hide resolved
@superdump
Copy link
Contributor

I also wonder why this should panic instead of just returning an error. The Node::run are supposed to be fallible.

@mockersf
Copy link
Member

mockersf commented Apr 28, 2022 •
edited
Loading

the panic is thread 'main' panicked at 'view entity should exist: QueryDoesNotMatch(0v0)', crates\bevy_core_pipeline\src\main_pass_2d.rs:45:14, so panicking on the expect there. Should it return a NodeRunError in this case?

This PR makes it returns a Ok(()) which swallows the error

@tim-blackbird
Copy link
Contributor

A similar change was made to main_pass_3d.rs in #3330, which Cart approved.

@mockersf
Copy link
Member

mockersf commented May 8, 2022

can't reproduce the initial crash, but the code change makes sense

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 8, 2022
@superdump
Copy link
Contributor

Why is there no view when the window is minimised?

@superdump
Copy link
Contributor

Also, how come this only affects 2d?

@DJMcNab
Copy link
Member Author

DJMcNab commented May 9, 2022

I don't know why this happens whilst minimised. Presumably we have some code that prevents creating swapchain textures of size 0.

And as @devil-ira mentioned, #3330 added the same thing for 3d, hence that not crashing.

@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone May 9, 2022
@alice-i-cecile alice-i-cecile mentioned this pull request May 15, 2022
Closed
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 15, 2022 •
edited
Loading

@DJMcNab, #4757 does the same thing for UI cameras. Can you roll in that fix and credit the author?

@DJMcNab
Copy link
Member Author

DJMcNab commented May 15, 2022

I can do if someone makes a pr against the branch for it 😆

I'm not in a context where I can do any manual git operations at the moment

@hoshino111
Copy link
Contributor

I just made a pr against your branch. 👌

@DJMcNab
Copy link
Member Author

DJMcNab commented May 15, 2022

I guess we should add a UI camera to the minimising and resizing examples too

@alice-i-cecile alice-i-cecile added the A-Rendering Drawing game state to the screen label May 16, 2022
@mockersf
Copy link
Member

soon ©️ (#4745) the ui camera will be a component on existing cameras

@DJMcNab
Allow minimising in 2d
8907d1a 
Add a comment
@DJMcNab DJMcNab mentioned this pull request May 28, 2022
Closed
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request May 30, 2022
@DJMcNab @hoshino111
Allow minimising in 2d (#4527)
09a3d8a 
# Objective

- We can't minimise if there's a 2d camera because ??? there legally must be a 2d target.
- Fixes #4526
- Fixes #4856

## Solution

- Make it not crash in those cases, just do nothing
- Seems to work ¯\\_(ツ)_/¯
- See also the companion commit in #3597 - 503c247

Co-authored-by: Asteria <asteria131@outlook.com>
@bors
Copy link
Contributor

bors bot commented May 30, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Allow minimising in 2d [Merged by Bors] - Allow minimising in 2d May 30, 2022
@bors bors bot closed this May 30, 2022
@infmagic2047 infmagic2047 mentioned this pull request May 31, 2022
Closed
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
@DJMcNab @hoshino111 @james7132
Allow minimising in 2d (bevyengine#4527)
9cd55df 
# Objective

- We can't minimise if there's a 2d camera because ??? there legally must be a 2d target.
- Fixes bevyengine#4526
- Fixes bevyengine#4856

## Solution

- Make it not crash in those cases, just do nothing
- Seems to work ¯\\_(ツ)_/¯
- See also the companion commit in bevyengine#3597 - 503c247

Co-authored-by: Asteria <asteria131@outlook.com>
@asaaki asaaki mentioned this pull request Jun 26, 2022
Merged
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@DJMcNab @hoshino111 @ItsDoot
Allow minimising in 2d (bevyengine#4527)
5781a0a 
# Objective

- We can't minimise if there's a 2d camera because ??? there legally must be a 2d target.
- Fixes bevyengine#4526
- Fixes bevyengine#4856

## Solution

- Make it not crash in those cases, just do nothing
- Seems to work ¯\\_(ツ)_/¯
- See also the companion commit in bevyengine#3597 - 503c247

Co-authored-by: Asteria <asteria131@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superdump superdump superdump requested changes

@hoshino111 hoshino111 hoshino111 approved these changes

@mockersf mockersf mockersf approved these changes

@IceSentry IceSentry IceSentry approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile 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 P-Regression Functionality that used to work but no longer does. Add a test for this! 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
Bevy 0.8
Development

Successfully merging this pull request may close these issues.

UI crashes if a UI camera is removed Minimizing window with 2d camera causes a crash
7 participants
@DJMcNab @superdump @mockersf @tim-blackbird @alice-i-cecile @hoshino111 @IceSentry