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

Change Window scale factor to f32 #8942

Closed
wants to merge 8 commits into from
Closed

Conversation

thmsgntz
Copy link
Contributor

@thmsgntz thmsgntz commented Jun 24, 2023

Hi!
This is my first contribution! I am not familiar yet with everything in Bevy so I am opened to criticism to learn :)

Objective

Fixes #8900

Solution

  • Changed scale_factor of Window and UI from f64 to f32

Changelog

Structs changed:

  • bevy_ui: layout_context
  • bevy_ui: UI_Scale
  • bevy_window: WindowResolution
  • carmera.rs: RenderTargetInfo

Tests changed:

  • examples/ui/ui_scaling.rs

Migration Guide

If manipulating scale_factor, some conversion from f64 to f32 may be necessary.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@thmsgntz
Copy link
Contributor Author

thmsgntz commented Jun 24, 2023

Hmm cargo -p run ci passes well on my computer. Checking these errors.

EDIT: oh ok I need to rebase on main, did not see the changes on main. Working on it.

@Selene-Amanita Selene-Amanita added A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jun 24, 2023
@alice-i-cecile
Copy link
Member

Great! We need a second approval before we can merge this :)

In this case I'm going to pull in both @JMS55 (rendering expert) and @ickshonpe (UI expert who requested the change).

P.S. I couldn't request ickshonpe as a reviewer because you're not in the Bevy org; bug @cart for membership and triage powers?

@ickshonpe
Copy link
Contributor

I'll take a look when I get home this evening.

@ickshonpe
Copy link
Contributor

ickshonpe commented Jun 26, 2023

These are the changes I had in mind when I wrote up this issue, and the implementation looks good. I'll run through some examples just to make sure everything is okay.

I should have written up the original issue better though. For Bevy UI there should be no problems, even with something silly like a resolution ten times larger than 8k single-precision floats should be enough. But I don't know if (for example) there might be complications caused by the changes to Camera for people running simulations of very large worlds. There should also be a reason why winit uses double-precision too (HIDPI?), I'd like to know that as well.

I think even if there are complications, they wouldn't block most of the changes as we can just convert the values from Window at access and otherwise use f32s internally.

@nicoburns
Copy link
Contributor

The other case in which there could be issues is very long scrollable views. But fixing this would likely require f64 everywhere (or a fixed point value where 1 = 1/60th like web browsers use), not just for the scale factor.

@thmsgntz thmsgntz closed this Jun 30, 2023
@thmsgntz thmsgntz force-pushed the main branch 2 times, most recently from c87ebc2 to fd32c6f Compare June 30, 2023 16:09
@alice-i-cecile
Copy link
Member

Are you planning to remake this from an alternate branch?

@thmsgntz thmsgntz reopened this Jun 30, 2023
@thmsgntz
Copy link
Contributor Author

back online, my bad.

@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Jun 30, 2023
self.start_scale = self.current_scale();
self.target_scale = scale;
self.target_time.reset();
}

fn current_scale(&self) -> f64 {
fn current_scale(&self) -> f32 {
let completion = self.target_time.percent();
let multiplier = ease_in_expo(completion as f64);
Copy link
Contributor

Choose a reason for hiding this comment

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

should ease_in_expo be changed to take a f32 also?

@alice-i-cecile alice-i-cecile changed the title Refs #8900 -- Changing scale factor to f32 Change Window scale factor to f32 Aug 11, 2023
@alice-i-cecile
Copy link
Member

@thmsgntz can you please resolve the merge conflicts so we can merge this? @ickshonpe, can I get your review?

@alice-i-cecile alice-i-cecile removed this from the 0.12 milestone Sep 29, 2023
@thmsgntz
Copy link
Contributor Author

thmsgntz commented Oct 5, 2023

@thmsgntz can you please resolve the merge conflicts so we can merge this? @ickshonpe, can I get your review?

Sure, I will have some free time this weekend to work on it

# Conflicts:
#	crates/bevy_text/src/pipeline.rs
#	crates/bevy_ui/src/layout/mod.rs
#	crates/bevy_ui/src/lib.rs
#	crates/bevy_ui/src/render/mod.rs
#	crates/bevy_ui/src/ui_node.rs
#	crates/bevy_ui/src/widget/text.rs
#	crates/bevy_window/src/window.rs
#	crates/bevy_winit/src/lib.rs
Modify ease_in_expo to take an f32 instead of f64
@thmsgntz
Copy link
Contributor Author

thmsgntz commented Oct 5, 2023

I think I'm done but I need help to fix these problems with cargo doc :(

cargo run -p ci -- doc returns :

error: redundant explicit link target
   --> crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs:425:112
    |
425 | ...rom the [`members_to_serialization_denylist`](crate::utility::members_to_serialization_denylist) function.
    |             -----------------------------------  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ explicit target is redundant
    |             |
    |             because label contains path that resolves to same destination
    |
    = note: when a link's destination is not specified,
            the label is used to resolve intra-doc links
    = note: `-D rustdoc::redundant-explicit-links` implied by `-D warnings`
help: remove explicit link target
    |
425 |     /// The returned bitset is a collection of indices obtained from the [`members_to_serialization_denylist`] function.
    |                                                                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: could not document `bevy_reflect_derive`

And I do not know how to fix errors from cargo run -p ci -- compile like this one :

test tests/ui/system_state_iter_mut_overlap_safety.rs ... mismatch

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error[E0502]: cannot borrow `query` as immutable because it is also borrowed as mutable
  --> tests/ui/system_state_iter_mut_overlap_safety.rs:18:13
   |
15 |         let mut_vec = query.iter_mut().collect::<Vec<bevy_ecs::prelude::Mut<A>>>();
   |                       ---------------- mutable borrow occurs here
...
18 |             query.iter().collect::<Vec<&A>>(),
   |             ^^^^^^^^^^^^ immutable borrow occurs here
...
23 |             mut_vec.iter().map(|m| **m).collect::<Vec<A>>(),
   |             -------------- mutable borrow later used here
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error[E0502]: cannot borrow `query` as immutable because it is also borrowed as mutable
  --> tests/ui/system_state_iter_mut_overlap_safety.rs:18:13
   |
15 |         let mut_vec = query.iter_mut().collect::<Vec<bevy_ecs::prelude::Mut<A>>>();
   |                       ----- mutable borrow occurs here
...
18 |             query.iter().collect::<Vec<&A>>(),
   |             ^^^^^ immutable borrow occurs here
...
23 |             mut_vec.iter().map(|m| **m).collect::<Vec<A>>(),
   |             ------- mutable borrow later used here
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
note: If the actual output is the correct output you can bless it by rerunning
      your test with the environment variable TRYBUILD=overwrite

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Dec 5, 2023
@alice-i-cecile
Copy link
Member

Adopted in #10897 <3 Thanks for getting this started!

github-merge-queue bot pushed a commit that referenced this pull request Dec 14, 2023
# Objective

- Finish the work done in #8942 .

## Solution

- Rebase the changes made in #8942 and fix the issues stopping it from
being merged earlier

---------

Co-authored-by: Thomas <1234328+thmsgntz@users.noreply.github.com>
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 A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

f32 scale factor
7 participants