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

Remove CanvasParentResizePlugin #11057

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Remove CanvasParentResizePlugin #11057

merged 1 commit into from
Dec 21, 2023

Conversation

Vrixyz
Copy link
Member

@Vrixyz Vrixyz commented Dec 21, 2023 •
edited
Loading

Improves #11052

Changelog

  • Remove Window::fit_canvas_to_parent, as its resizing on wasm now respects its CSS configuration.

Migration Guide

  • Remove uses of Window::fit_canvas_to_parent in favor of CSS properties, for example: 
    canvas {
  width: 100%;
  height: 100%;
}

@Vrixyz Vrixyz added the A-Windowing Platform-agnostic interface layer to run your app in label Dec 21, 2023
@alice-i-cecile
Copy link
Member

You appear to be missing an issue link :)

@alice-i-cecile alice-i-cecile added C-Examples An addition or correction to our examples C-Code-Quality A section of code that is hard to understand or change labels Dec 21, 2023
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Dec 21, 2023
@Vrixyz Vrixyz mentioned this pull request Dec 21, 2023
Open
23 tasks
@rparrett rparrett added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Dec 21, 2023
@github-actions GitHub Actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

tim-blackbird
tim-blackbird approved these changes Dec 21, 2023
View reviewed changes
Copy link
Contributor

@tim-blackbird tim-blackbird left a comment

Choose a reason for hiding this comment

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

When the website gets updated for the 0.13 release we can revert bevyengine/bevy-website#640.

@MeoMix
Copy link

MeoMix commented Dec 21, 2023

Really excited for this! :) I think this will make it simpler to avoid a flicker due to incorrect sizing on first load of my WASM app!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 21, 2023
@alice-i-cecile alice-i-cecile 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 Dec 21, 2023
Merged via the queue into bevyengine:main with commit 80f15e0 Dec 21, 2023
28 checks passed
@cshenton-work
Copy link

A migration guide would be appreciated.

fit_canvas_to_parent provided the exact behaviour I was depending on, simply giving me a full screen canvas that scaled with the window, without the need for any external css/html other than the default generated by most wasm runners.

@daxpedda
Copy link
Contributor

daxpedda commented Jan 5, 2024 •
edited
Loading

@cshenton-work I've noted what needs to be done in #11052 (comment):

Window::fit_canvas_to_parent was still useful to apply the CSS width: 100%; height: 100%, something similar should be introduced again (without all the resizing stuff, it should literally only apply these CSS values).

So until this is hopefully addressed in Bevy, just get the canvas and apply width: 100%; height: 100% to it's style attribute.

EDIT: I can actually see that the commit has added a migration guide: 80f15e0. I assume this will be later compiled into the changelog.

Vrixyz added a commit to Vrixyz/bevy that referenced this pull request Jan 9, 2024
@Vrixyz
Follow up to bevyengine#11057 ; fix fit_canvas_to_parent
c37cb7e 
Implemented suggestions from reviewers: a simpler fit_canvas_to_parent
leads to an explicit CSS setting to the canvas.

It has do be set after wgpu creation due to wgpu overriding the canvas width/height:
https:/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74
@Vrixyz Vrixyz mentioned this pull request Jan 9, 2024
Merged
Vrixyz added a commit to Vrixyz/bevy that referenced this pull request Jan 9, 2024
@Vrixyz
Follow up to bevyengine#11057 ; fix fit_canvas_to_parent
f0dd8b1 
Implemented suggestions from reviewers: a simpler fit_canvas_to_parent
leads to an explicit CSS setting to the canvas.

It has do be set after wgpu creation due to wgpu overriding the canvas width/height:
https:/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74
@richchurcher richchurcher mentioned this pull request Feb 10, 2024
Closed
@frewsxcv
Copy link
Contributor

The CSS replacement of fit_canvas_to_parent does not work for me. Doesn't seem to resize like it used to https://rgis.app

@MeoMix
Copy link

MeoMix commented Feb 23, 2024

The CSS replacement of fit_canvas_to_parent does not work for me. Doesn't seem to resize like it used to https://rgis.app

Your width/height 100% isn't taking precedent over other stylings set programmatically.

image

@frewsxcv
Copy link
Contributor

@MeoMix To my knowledge, I am not setting them programmatically in my project

@frewsxcv
Copy link
Contributor

1280 x 720 is coming from Bevy:

bevy/crates/bevy_window/src/window.rs

Lines 631 to 632 in caa7ec6

physical_width: 1280,
physical_height: 720,

@frewsxcv
Copy link
Contributor

It seems like it's being set by winit:

https:/rust-windowing/winit/blob/a127bd6f6684909852000cdf32469bac3b611887/src/platform_impl/web/web_sys/canvas.rs#L71-L77

@MeoMix
Copy link

MeoMix commented Feb 23, 2024

Understood.

#11278 It sort of looks like this PR wanted to get merged in but didn't make it?

@Vrixyz
Copy link
Member Author

Vrixyz commented Feb 23, 2024

In the meantime I believe a workaround has been to set the width/height of the container div/html

@frewsxcv
Copy link
Contributor

In the meantime I believe a workaround has been to set the width/height of the container div/html

Set it to what?

@Vrixyz
Copy link
Member Author

Vrixyz commented Feb 23, 2024

To 100%. (Or the size you want your inner canvas to be)

Vrixyz added a commit to Vrixyz/bevy that referenced this pull request Mar 3, 2024
@Vrixyz
Follow up to bevyengine#11057 ; fix fit_canvas_to_parent
1c7f49c 
Implemented suggestions from reviewers: a simpler fit_canvas_to_parent
leads to an explicit CSS setting to the canvas.

It has do be set after wgpu creation due to wgpu overriding the canvas width/height:
https:/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74
github-merge-queue bot pushed a commit that referenced this pull request Mar 3, 2024
@Vrixyz
Fix fit_canvas_to_parent (#11278)
8cf5fbb 
Follow up to #11057

Implemented suggestions from reviewers from: a simpler
fit_canvas_to_parent leads to an explicit CSS setting to the canvas.

From my understanding, it has do be set after wgpu creation due to wgpu
overriding the canvas width/height:
https:/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74


# Changelog

- Re-enable a `fit_canvas_to_parent`, it's removal from
#11057 was problematic. Still,
its inner working is more simple than before: bevy doesn't handle its
resizing, winit does.

## Migration Guide

- Cancels the migration from
#11057
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
@Vrixyz @spectria-limina
Fix fit_canvas_to_parent (bevyengine#11278)
904c348 
Follow up to bevyengine#11057

Implemented suggestions from reviewers from: a simpler
fit_canvas_to_parent leads to an explicit CSS setting to the canvas.

From my understanding, it has do be set after wgpu creation due to wgpu
overriding the canvas width/height:
https:/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74


# Changelog

- Re-enable a `fit_canvas_to_parent`, it's removal from
bevyengine#11057 was problematic. Still,
its inner working is more simple than before: bevy doesn't handle its
resizing, winit does.

## Migration Guide

- Cancels the migration from
bevyengine#11057
v-kat pushed a commit to v-kat/bevy that referenced this pull request Mar 17, 2024
@Vrixyz
Fix fit_canvas_to_parent (bevyengine#11278)
40e5d46 
Follow up to bevyengine#11057

Implemented suggestions from reviewers from: a simpler
fit_canvas_to_parent leads to an explicit CSS setting to the canvas.

From my understanding, it has do be set after wgpu creation due to wgpu
overriding the canvas width/height:
https:/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74


# Changelog

- Re-enable a `fit_canvas_to_parent`, it's removal from
bevyengine#11057 was problematic. Still,
its inner working is more simple than before: bevy doesn't handle its
resizing, winit does.

## Migration Guide

- Cancels the migration from
bevyengine#11057
v-kat pushed a commit to v-kat/bevy that referenced this pull request Mar 21, 2024
@Vrixyz
Fix fit_canvas_to_parent (bevyengine#11278)
13fbf52 
Follow up to bevyengine#11057

Implemented suggestions from reviewers from: a simpler
fit_canvas_to_parent leads to an explicit CSS setting to the canvas.

From my understanding, it has do be set after wgpu creation due to wgpu
overriding the canvas width/height:
https:/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74


# Changelog

- Re-enable a `fit_canvas_to_parent`, it's removal from
bevyengine#11057 was problematic. Still,
its inner working is more simple than before: bevy doesn't handle its
resizing, winit does.

## Migration Guide

- Cancels the migration from
bevyengine#11057
v-kat pushed a commit to v-kat/bevy that referenced this pull request Apr 26, 2024
@Vrixyz
Fix fit_canvas_to_parent (bevyengine#11278)
5d1665a 
Follow up to bevyengine#11057

Implemented suggestions from reviewers from: a simpler
fit_canvas_to_parent leads to an explicit CSS setting to the canvas.

From my understanding, it has do be set after wgpu creation due to wgpu
overriding the canvas width/height:
https:/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74


# Changelog

- Re-enable a `fit_canvas_to_parent`, it's removal from
bevyengine#11057 was problematic. Still,
its inner working is more simple than before: bevy doesn't handle its
resizing, winit does.

## Migration Guide

- Cancels the migration from
bevyengine#11057
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

@tim-blackbird tim-blackbird tim-blackbird approved these changes

Assignees
No one assigned
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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
0.13
Development

Successfully merging this pull request may close these issues.
8 participants
@Vrixyz @alice-i-cecile @MeoMix @cshenton-work @daxpedda @frewsxcv @tim-blackbird @rparrett