asny / three-d

2D/3D renderer - makes it simple to draw stuff across platforms (including web)
MIT License
1.34k stars 110 forks source link

Add FrameInput::redraw_needed for handling window visibility changes #475

Closed maxime-tournier closed 2 months ago

maxime-tournier commented 2 months ago

This small PR adds a flag redraw_needed to FrameInputGenerator/FrameInput that is set on WindowEvent::Occluded(false) event and cleared during the next FrameInputGenerator::generate call.

This is to allow the render loop to emit render calls after (partial) window visibility changes, like so:

    window.render_loop(
        move |mut frame_input| {
            let mut change = frame_input.first_frame | frame_input.redraw_needed;

Without it the window doesn't get redrawn as it should e.g. on Alt+Tab (Linux/X11).

This is my first Rust PR, so let's hope I did not screw up :)

asny commented 2 months ago

That makes sense, and I think your solution is good. However, first_frame and redraw_needed are very similar, could we just have one of them? Now that first_frame is already there, the easiest solution would be to just have

WindowEvent::Occluded(false) => {
    self.first_frame = true;
}

I know the name might not be super great, but if we update the documentation, I think it's ok. It's the first frame after it's not occluded. Alternatively, we can rename it to redraw_needed or similar.

I think it's a nice first Rust PR, you just forgot to run cargo fmt (as everyone else).

maxime-tournier commented 2 months ago

Thanks for your feedback.

could we just have one of them?

Actually that was my first try, but I thought maybe there would be cases where one needs to disambiguate the two?

Either way I'm fine with having just first_frame. I pushed the changes.

you just forgot to run cargo fmt (as everyone else).

Ah! Thanks, I'm eager to learn good practices.

asny commented 2 months ago

Thanks for your feedback.

No problem, thanks for the contribution 🙏

Actually that was my first try, but I thought maybe there would be cases where one needs to disambiguate the two?

Hmm. Might be, but I can't think of a case 🤔 If it's necessary, it's easy to split it into two variables later.

Either way I'm fine with having just first_frame. I pushed the changes.

Great, looks good 👍

maxime-tournier commented 2 months ago

Thanks :)