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

Fix viewport_to_world and world_to_viewport #6526

Conversation

irate-devil
Copy link
Contributor

@irate-devil irate-devil commented Nov 9, 2022

Objective

The origin of the viewport lies at the top-left, as documented here.

Test program.
use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup)
        .add_system(system)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());

    commands.spawn(NodeBundle {
        background_color: Color::BLACK.into(),
        style: Style {
            size: Size::new(Val::Px(20.), Val::Px(20.)),
            position_type: PositionType::Absolute,
            ..default()
        },
        ..default()
    });
}

fn system(query: Query<(&Camera, &GlobalTransform)>, mut ui_query: Query<&mut Style>) {
    let (camera, global_transform) = query.single();

    let world_pos = Vec3::new(0., 150., 0.);

    let Some(pos) = camera.world_to_viewport(global_transform, world_pos) else {
        println!("B");
        return;
    };

    // UI element should be 150 pixels up from the center, instead it was down 150 pixels.
    ui_query.single_mut().position = UiRect {
        left: Val::Px(pos.x),
        top: Val::Px(pos.y),
        ..default()
    };
}

Migration Guide

Camera::world_to_viewport now correctly returns the viewport position with Y pointing downwards, instead of up.
You may need to adjust your code if you were using this method.

@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Nov 9, 2022
@irate-devil irate-devil marked this pull request as draft November 9, 2022 09:17
@irate-devil
Copy link
Contributor Author

irate-devil commented Nov 9, 2022

Needs more testing D:
edit: Sanity-check completed. See the test program in the PR description :)

@irate-devil irate-devil marked this pull request as ready for review November 9, 2022 09:29
@mockersf
Copy link
Member

mockersf commented Nov 9, 2022

this may be reverted for the 0.9, see #6522

@irate-devil
Copy link
Contributor Author

irate-devil commented Nov 9, 2022

@mockersf While this change is somewhat related, that issue is only about reverting the origin of the cursor back to the bottom-left.
#6000 did not touch viewports; world_to_viewport was already wrong in Bevy 0.8 (Hence the migration guide).

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior A-Input Player input via keyboard, mouse, gamepad, and more labels Nov 9, 2022
@alice-i-cecile alice-i-cecile 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-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Nov 12, 2022
@alice-i-cecile
Copy link
Member

I'm less convinced by this. it feels like the right fix is that the viewport's y should match the world's and be at the bottom.

@alice-i-cecile alice-i-cecile removed this from the Bevy 0.9 milestone Nov 12, 2022
@bzm3r
Copy link
Contributor

bzm3r commented Nov 12, 2022

@devil-ira sorry, actually, I don't quite understand this. Could you provide some more commentary on why your solution does what is intended?

From a mathematical perspective, I get that if I want to mirror a coordinate system along a particular axis, we multiply the values of that axis by -1.

But suppose we have a grid an N x M grid, and we want to mirror in the y-direction.

Then, the mapping from (x, y) to mirrored coordinates is: (x, M - y), right? How come we don't have to do something like that there?

@irate-devil
Copy link
Contributor Author

I'm less convinced by this. it feels like the right fix is that the viewport's y should match the world's and be at the bottom.

Pls no. I really want screen space stuff to have consistent coordinates, unless it really makes sense for them to be different. (UI and viewports are aligned currently).

@bzm3r Note that this is a breaking change as I believed(I'm rethinking atm) the usage of the existing method was technically incorrect, even if it did work. It's just the sign/direction of the Y axis that is off. The comments in the code should be changed to reflect that better.

I'm closing this as I realize the way it is now is not necessarily incorrect, and merging this would make using these methods with the cursor position more cumbersome.

I do want to make an argument in favor of making the coordinate systems of screen space stuff in Bevy(UI, cursor position, and viewports) be consistent/least surprising, but I'll do that in a dedicated issue once I've written something up :)

@lewiszlw
Copy link
Member

lewiszlw commented Feb 8, 2023

I do want to make an argument in favor of making the coordinate systems of screen space stuff in Bevy(UI, cursor position, and viewports) be consistent/least surprising, but I'll do that in a dedicated issue once I've written something up :)

Yes, I agree that making the coordinate systems of screen space stuff be consistent. It's weird that when I first using world_to_viewport method. Do you create issue or something to track this? @devil-ira

@irate-devil irate-devil deleted the fix-viewport-to-world-and-back branch March 13, 2023 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more 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.

None yet

5 participants