Fix conversion from u32 to c_int

[Selene-Amanita]

This PR fixes the conversion from u32 to c_int which resulted in negative c_int values for big u32 values.

I'm a new contributor to Winit and I honestly didn't check all the guidelines nor I understand all the code, but I think I found a bug while investigating a bug in Bevy, which uses Winit: bevyengine/bevy#8948 (comment)

In short, when using set_max_inner_size or with_max_inner_size with a big value (for Bevy we use a f32 logical size but I believe it's simply converted to a u32 down the line), the max constraint end up being equal to the min one (making the window not resizeable, to the size of the min constraint). After testing, the threshold value for this to happen seem to be around u32::MAX / 2 (more or less, not sure why not exactly that but can be rounding errors), which seem to indicate a conversion to a signed int that would make the value negative somewhere.

After investigating in Winit's code, I found conversions from u32 to c_int, and after testing in the playground it seems to be what's wrong: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=be9b6b0416fbb79ff0935ae4300036c7

I'm not sure how to test stuff with Winit alone so I didn't test my changes, but they seem minor enough to not be a problem, hopefully?

[kchibisov]

@Selene-Amanita thanks for your patch, however I think that your issue is fixed in that PR #2825 could you ensure that? If not it's better to open against that work, since both of them touch the same things.

[kchibisov]

Hm, it might be not.

cc @notgull

[Selene-Amanita]

I don't understand much of the code of this other PR but it does delete the code that this PR modifies so...

I see a self.xconn.default_screen_index() as c_int in #2825 I'm not sure what type is it but it might have the same problem, all the as c_int should be checked just in case I think.

[kchibisov]

Yeah, I'd at least wait for @notgull to reply. Though, I wonder why you passed that big of the size to window, most windows are limited to u16/i16 under the hood.

I think there're other cases in other backends where we don't clamp to max the values.

[Selene-Amanita]

I wonder why you passed that big of the size to window, most windows are limited to u16/i16 under the hood.

Because in Bevy the lack of a max constraint is represented by a f32::INFINITY, a width max constraint can be set without a height constraint and vice-versa, and also we don't control what the end user might put.

So either f32::INFINITY/an arbitrary high value should work out of the box (it doesn't because of this c_int conversion ultimately I think), or I should have a value I can at least set under the hood as the max value.

I guess I can use u16::MAX since it fixes the problem and you're telling me that it should work for most cases. But even setting that is a bit of a pain because I don't know the scale_factor yet at window creation and our size constraints are defined as logical size.

[Selene-Amanita]

I think there're other cases in other backends where we don't clamp to max the values.

Well we have another bug that I didn't investigate where calling set_inner_size with very high values panics but that might come from Bevy's side.

[kchibisov]

But logical sizes are scale agnostic though? It's weird though that you have such an API using infinity instead of, let's say Option which more rust way of doing things.

Even winit accepts an Option here.

[Selene-Amanita]

It's weird though that you have such an API using infinity instead of, let's say Option which more rust way of doing things.

I wasn't the one that defined this on Bevy's side, but even if it was an Option that doesn't change the fact that the end user can put Some(f32::INFINITY) if they want, or an arbitrary high value.

Even winit accepts an Option here.

Not if you want to set a constraint only for height or width but not both, you have to provide a value for both or none with set_max_inner_size and with_max_inner_size.

Previous to my PR, Bevy checks if both are finite, if one of them are infinite it just ignores the constraint, I want to make it work with one constraint only, but for that I need to provide a maximum value for the one that is not set.

But logical sizes are scale agnostic though?

Well yes but in that case where we want to define the "max value that wouldn't be converted to a negative value in Winit's backend because of c_int conversion", this value is a physical size actually, so we need a conversion.

[kchibisov]

Clamping is generally good to do, but I'm not sure how we should ensure that consistently across all the backends, maybe each backend internally. For now what you're doing is fine, but it just moves the problem.

Anyway, that's up to future changes of winit, and now I'd wait for @notgull since I was about to merge his patch pretty soon and don't want to force rebase on them.

[notgull]

winit/src/platform_impl/linux/x11/window.rs

Line 1273 in 5634305

normal_hints.min_size = dimensions.map(|(w, h)| (w as i32, h as i32))

Apparently it's not fixed, since I still do bitcasting here.

[notgull]

I see a self.xconn.default_screen_index() as c_int in #2825 I'm not sure what type is it but it might have the same problem, all the as c_int should be checked just in case I think.

In this case it's casting a usize representing an array index as an int. In this case the array in question is the array of screens attached to the display. Unless you have several billion screens attached to your display this shouldn't be an issue, and if you do I doubt that winit is the right library for your use case.

[kchibisov]

I don't see any issue with doing clamping, however I think that the way it should be done is that backend specify internally what Size in particular it'll expect and then we simply into() and clamping will be done automagically, though, the internal size will be some other Size type. The same for other types, I think.

[kchibisov]

I'm fine with the current patch if it matters.

[notgull]

I reimplemented this PR in a653b04

[Selene-Amanita]

Thank you for the work on this!
Can I assume this is fixed and close this PR?

[kchibisov]

I think so.