PistonDevelopers / conrod

An easy-to-use, 2D GUI library written entirely in Rust.
Other
3.35k stars 297 forks source link

Bug in gfx backend when using crop_kids #1166

Open Epsylon42 opened 6 years ago

Epsylon42 commented 6 years ago

It seems like the gfx backend incorrectly calculates the area's y coordinate.

For example Canvas in the middle: default

Canvas moved down: default Whoops. I can't see half of it.

Canvas moved up: default And here's a text area I probably should not see.

I managed to kinda fix it by changing rect_to_gfx_rect here like this:

let rect_to_gfx_rect = |rect: Rect| {
    let (w, h) = rect.w_h();
    let left = (rect.left() * dpi_factor + half_win_w) as u16;
    let bottom = (-rect.bottom() * dpi_factor - (h * dpi_factor) + half_win_h) as u16;
    let width = (w * dpi_factor) as u16;
    let height = (h * dpi_factor) as u16;
    gfx::Rect {
        x: std::cmp::max(left, 0),
        w: std::cmp::min(width, screen_w as u16),
        y: std::cmp::max(bottom, 0),
        h:  std::cmp::min(height, screen_h as u16),
    }
};

This fixes the problem, but now the label above the canvas is invisible. Also this solution doesn't seem right since the glium backend has the same conversion function and it works correctly.

tdaffin commented 5 years ago

This looks a lot like a problem I ran into. I've posted a PR to fix it: https://github.com/PistonDevelopers/conrod/pull/1274

Perhaps you could try using the branch my fix is on with your code to see if it fixes your problem?

You'd use the following two lines in your Cargo.toml:

[dependencies]
conrod_core = { git = "https://github.com/tdaffin/conrod", branch = "fix_crop" }
conrod_piston = { git = "https://github.com/tdaffin/conrod", branch = "fix_crop" }
Epsylon42 commented 5 years ago

I'm using gfx backend, so the fix doesn't work for me. As far as I know, the bug is present only there.

tdaffin commented 5 years ago

Got it. My fix is specific to the piston backend.

However, I think that I can reproduce your problem too. I made a branch with some changes to the shared examples to demonstrate the problem I saw here: https://github.com/tdaffin/conrod/tree/list_crop_bug

You can then run the example for multiple backends. Originally I saw the same problem for both the gfx and piston backends, but not for glium or vulkano. I didn't think too deeply about where I made the fix, so I didn't realize that my fix didn't also fix the gfx backend.

Run it for gfx using: cargo run --example all_winit_gfx

I've also made a branch that merges the fix into the shared example branch: https://github.com/tdaffin/conrod/tree/fix_list_crop_bug

The piston example can be seen to be broken before fix and fixed after here: cargo run --example all_piston_window

Perhaps I can find where the problem is in the gfx backend too...

tdaffin commented 5 years ago

Actually, the fix you give above fixes the 'all_winit_gfx' example from my branch already... My example does not have a label above the canvas though. Perhaps you could post the example you were testing with?

Epsylon42 commented 5 years ago

Sure. Here it is: https://github.com/Epsylon42/conrod_crop_bug

I also had to change a macro in conrod_winit (the changed version is included in the repo so it should work fine). Probably some kind of shenanigans with winit version, but this seems like an error someone else may stumble upon too. Perhaps it's worth opening another issue for it? Here's the diff just in case.

@@ -154,7 +154,7 @@ macro_rules! convert_window_event {
     ($event:expr, $window:expr) => {{
         // The window size in points.
         let (win_w, win_h) = match $window.get_inner_size() {
-            Some((w, h)) => (w as conrod_core::Scalar, h as conrod_core::Scalar),
+            Some(size) => (size.width as conrod_core::Scalar, size.height as conrod_core::Scalar),
             None => return None,
         };
tdaffin commented 5 years ago

Thanks! I can reproduce your bug from the sample -- including the disappearing label. In my examples I had not used the label on the Canvas, so I will try adding that to mine and see if I get the same problem.

tdaffin commented 5 years ago

It looks like the problem with the label disappearing is a separate issue. The crop is setup to crop to the kid_area of the canvas but the title is placed outside of the kid area so is cropped. The label issue also occurs with the same ui widgets in all four backends that I tested -- vulkano, piston, glium and gfx. Your fix just makes the gfx backend give the same as the others. Earlier you mention that your solution doesn't seem right since the glium backend has the same conversion function and it works correctly. I think this is ok as some backends define the crop/scissor rectangle as starting in the bottom left, and others in the top left. Another way to write your fix that gives the same result is:

let rect_to_gfx_rect = |rect: Rect| {
            let (w, h) = rect.w_h();
            let left = half_win_w + rect.left() * dpi_factor;
            let top = half_win_h - rect.top() * dpi_factor;
            let width = (w * dpi_factor) as u16;
            let height = (h * dpi_factor) as u16;
            gfx::Rect {
                x: left.max(0.0) as u16,
                y: top.max(0.0) as u16,
                w: width.min(screen_w as u16),
                h: height.min(screen_h as u16),
            }
        };

Which is perhaps more obviously stating that the rect must be defined from top left