[Merged by Bors] - Avoid windows with a physical size of zero

[HackerFoo]

Objective

Fix #4097

Solution

Return None from RenderTarget::get_physical_size if either dimension is zero.

[aevyrie]

Can confirm this fixes #4097 on Win11. Changes look like a nice improvement. 👍

Marking as ready for review because this is a bugfix with a tiny footprint.

[mockersf]

As this bug continue to come back, I think we should really look into merging #3597 (ideally making sure it would be failing in CI before this fix). last time it was tried, it was blocked on #3596. Would this fix work for that issue too?

[superdump]

The changes that are in the PR look good. I don't know if they address all possible cases across the entire codebase, but I think that is probably difficult to know and I'm too tired to sensibly go through and check right now.

[HackerFoo]

I had to make quite a few changes to rebase this PR. In addition to avoiding the zero size cases, I removed an unnecessary clear of clusters.lights which prevents the reuse of the vectors contained within.

The last commit (d1d4027) is optional - it's a small change to avoid the unconditional allocation of the visible_lights_scratch vector.

I ran the many_lights example, and the changes didn't make a measurable difference.

[aevyrie]

@HackerFoo instead of handling this special case throughout the code, what if we squash it at the source to prevent this from popping up yet again?

Changing the update_actual_size_from_backend function to prevent window dims from being set to zero on the bevy side appears to work:

    pub fn update_actual_size_from_backend(&mut self, physical_width: u32, physical_height: u32) {
        self.physical_width = physical_width.max(1);
        self.physical_height = physical_height.max(1);
    }

It feels a little bit hacky, but maybe it's a more robust way to handle this edge case by effectively ensuring window dims are nonzero?

[HackerFoo]

Using .max(1) can give an incorrect answer, and sweeps the problem under the rug for a while.

A render target with a dimension of 0 is invalid, and so that is why RenderTarget::get_physical_size must return None. Everything else follows from that. There are also other reasons to return None, and they should all be handled the same way.

More generally, I've found it a good practice to use Option<T> and Result<T, E> wherever appropriate, and handle them properly. In fact, I somewhat recently added a bunch of Results and the result (ha) is much more robust and errors are more observable. It can be slightly verbose, but is generally worth doing as soon as it seems useful.

It's useful to change the problem to avoid edge cases (here's a really cool example), but it should never be done in a way that could give an incorrect answer.

[superdump]

For what it's worth, I agree with the returning None approach and that for me is a centralised solution. When I've said "is there a way to solve this in one place and avoid problems everywhere else?" what I mean is that having things have to check for 0s on usizes or so is easy to miss and then have random panics or unexpected visual artifacts far from the root problem. Making it an Option or Result, in my opinion, forces you to have to handle it when using it. It strongly indicates that this can 'fail' or give an 'invalid' result and so you need to handle that. Makes sense to me.

[aevyrie]

You are both entirely correct, using an Option would be much better than pretending the problem doesn't exist.

Along those lines, then couldn't we also squash this issue at the source by changing get_physical_size to return None if either physical dimension is zero? It seems reasonable to say (based on the bugs we've repeatedly seen) that any zero sized dimension is ill-formed and should be handled.

Oof, I'm an idiot. I just realized you are doing this with .filter(|size| size.x > 0 && size.y > 0)

So, uh, long story short, this PR is perfect, please ignore me. 🙂

[cart]

We've got conflicts now that #4345 is merged :)

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• check-benches
• check-doc
• check-markdown-links
• check-missing-examples-in-docs
• check-unused-dependencies
• ci
• markdownlint
• miri
• run-examples