emoon / rust_minifb

Cross platfrom window and framebuffer crate for Rust
MIT License
980 stars 92 forks source link

Deprecate "limit_update_rate" in favor of "set_target_fps" #339

Closed StefanoIncardone closed 5 months ago

StefanoIncardone commented 5 months ago

possibily replace limit_update_rate with this implementation of "set_target_fps", since FPS is a more common and natural way to represent update intervals:

    /// Limits the FPS of polling for new events in order to reduce CPU usage.
    /// The problem of having a tight loop that does something like this
    ///
    /// ```no_run
    /// # use minifb::*;
    /// # let mut window = Window::new("Test", 640, 400, WindowOptions::default()).unwrap();
    /// loop {
    ///    window.update();
    /// }
    /// ```
    /// Is that lots of CPU time will be spent calling system functions to check for new events in a tight loop making the CPU time go up.
    /// Using `set_target_fps` minifb will check how many frames are left to reach the target FPS and there are any it will sleep for that amount of frames.
    /// This means that if more frames than the target happened (external code taking longer) minifb will not do any waiting at all so there is no loss in CPU performance with this feature.
    /// By default it's set to 250 FPS. Setting this value to None and no waiting will be done
    ///
    /// # Examples
    ///
    /// ```no_run
    /// # use minifb::*;
    /// # let mut window = Window::new("Test", 640, 400, WindowOptions::default()).unwrap();
    /// // Set the target rate to 60 fps, meaning events will be polled every ~16.6 ms
    /// window.set_target_fps(Some(60));
    /// ```
    #[inline]
    pub fn set_target_fps(&mut self, fps: Option<usize>) {
        match fps {
            Some(fps) => {
                let time = Some(Duration::from_secs_f32(1. / fps as f32));
                self.0.set_rate(time);
            }
            None => self.0.set_rate(None),
        }
    }
emoon commented 5 months ago

I guess that would be fine, but I would prefer keeping the old one around for a while with a deprecation tag then.

StefanoIncardone commented 5 months ago

I guess that would be fine, but I would prefer keeping the old one around for a while with a deprecation tag then.

That was exactly what i was thinking, so i'm up for any suggestions on how you'd like to handle deprecations

emoon commented 5 months ago

I guess having something like

#[deprecated(since = "0.26", note = "use set_fps_target instead, this function will be removed in the future."]

on the old function. Also given no time when it will be removed gives some flexibility

StefanoIncardone commented 5 months ago

I guess having something like

#[deprecated(since = "0.26", note = "use set_fps_target instead, this function will be removed in the future."]

on the old function. Also given no time when it will be removed gives some flexibility

I would also like to ask how to handle the possibility of setting the FPS to 0 (would crash when calculating 1 / fps), as i came up with different solutions:

  1. this would be the safest option, but more tedious for the user option, where we offload to the user the responsability of making sure that fps is not 0:
    pub fn set_target_fps(&mut self, fps: Option<NonZeroUsize>) {
        match fps {
            Some(fps) => {
                let fps: usize = fps.into();
                self.0
                    .set_rate(Some(Duration::from_secs_f32(1. / fps as f32)));
            }
            None => self.0.set_rate(None),
        }
    }
  2. this would be the "less sound", but esier for the user option, where we are responsible of making sure that fps is not 0:
    pub fn set_target_fps(&mut self, fps: Option<usize>) -> std::result::Result<(), ZeroFPS> {
        match fps {
            Some(fps) => match fps {
                0 => return Err(ZeroFPS),
                non_zero => self.0.set_rate(Some(Duration::from_secs_f32(1. / non_zero as f32))),
            },
            None => self.0.set_rate(None),
        }
        Ok(())
    }
  3. at this point option 2 makes use of a reduntant Option, so it would be reduced to this, treating 0 as if it was the None case:
    pub fn set_target_fps(&mut self, fps: usize) {
        match fps {
            0 => self.0.set_rate(None),
            non_zero => self.0.set_rate(Some(Duration::from_secs_f32(1. / non_zero as f32))),
        }
    }
emoon commented 5 months ago

I think 3.

StefanoIncardone commented 5 months ago

I think 3.

Thanks, I'll make a PR with that then