Use std::time::Duration for time intervals #7616

higumachan · 2022-08-09T05:11:10Z

I replaced the interval declared in f64 that I could find with ruffle_core::RuffleDuration.

Please let me know if you find any omissions in the f64-declared intervals, as they are difficult to find.

relrelb

Welome! I think instant::Instant can be used in place of the proposed RuffleDuration. instant is already a dependency of ruffle-core, and currently being used in various places across the codebase. So I'll be glad if you please proceed with instant::Instant. Also, looks like the PR has conflicts against current master branch. Can you please resolve them while at it?

n0samu · 2022-08-10T20:19:27Z

@relrelb Doesn't that conflict with what Mike was suggesting here? #6075 (comment)
Or am I misunderstanding something?

relrelb · 2022-08-12T08:50:57Z

Welome! I think instant::Instant can be used in place of the proposed RuffleDuration. instant is already a dependency of ruffle-core, and currently being used in various places across the codebase. So I'll be glad if you please proceed with instant::Instant. Also, looks like the PR has conflicts against current master branch. Can you please resolve them while at it?

Right, I take my words back. I was confusing between instant and duration, which are not the same thing.

higumachan · 2022-08-13T09:35:40Z

@relrelb I fixed conflict.

Welome! I think instant::Instant can be used in place of the proposed RuffleDuration. instant is already a dependency of ruffle-core, and currently being used in various places across the codebase. So I'll be glad if you please proceed with instant::Instant. Also, looks like the PR has conflicts against current master branch. Can you please resolve them while at it?

Right, I take my words back. I was confusing between instant and duration, which are not the same thing.

As for this one, the regression test failed due to lack of precision in std::Duration (same definition in instant::Duration), as I wrote in my comment here(#6075 (comment)).

Herschel

Thank you! This looks exactly what I had in mind. There are only a few small naming issues.

Herschel · 2022-08-26T19:46:31Z

scanner/Cargo.toml

-sha2 = "0.10.2"
+sha2 = "0.10.2"


Unrelated formatting change in the PR, could you remove this change?

Herschel · 2022-08-26T19:56:10Z

core/src/duration.rs

+    pub fn zero() -> Self {
+        Self(OrderedFloat::zero())
+    }


Let's make this pub const ZERO: Self. This matches what we do for Twips.

Herschel · 2022-08-26T19:56:36Z

core/src/duration.rs

+    pub const fn one_sec() -> Self {
+        Self(OrderedFloat(1.0))
+    }


Let's make this pub const ONE_SECOND: Self

Herschel · 2022-08-26T20:31:33Z

core/src/duration.rs

+#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Default, Serialize, Deserialize)]
+pub struct RuffleDuration(OrderedFloat<f64>);


For ease of typing and to follow Rust naming conventions, let's name this Duration instead of RuffleDuration. Rust prefers to avoid stuttering in names (ruffle_core::RuffleDuration vs. ruffle_core::Duration). We can explicitly specify for methods that want the std version (std::instant::Duration), or use a renaming import (use std::instant::Duration as StdDuration).

Instead of adding ordered_float dependency so that we can derive Ord and Eq, let's instead implement any necessary methods ourselves. For example, Ord isn't necessary if we add a direct min/max method:

pub fn min(self, other: RuffleDuration) -> RuffleDuration { if self.0 < other.0 { self } else { other } }

then the duration can become:
pub struct Duration(f64);
without the need for ordered_float.

I know we already have lots of dependencies, but I like to avoid adding new ones when possible.

higumachan · 2022-09-02T10:46:19Z

@Herschel Thanks for the review.
I fixed the your review points.

core/src/player.rs

core/src/timer.rs

Herschel · 2022-09-04T19:47:28Z

tests/tests/regression_tests.rs

@@ -1372,7 +1372,7 @@ fn run_swf(
        // with timer execution (timers will see an elapsed time of *at least*
        // the requested timer interval).
        if frame_time_sleep {
-            std::thread::sleep(frame_time_duration);
+            std::thread::sleep(StdDuration::from_nanos(frame_time.as_nanos() as u64));


Could this be frame_time.into()?

Herschel · 2022-09-04T19:48:03Z

tests/tests/regression_tests.rs

@@ -1310,8 +1311,7 @@ fn run_swf(
    let base_path = Path::new(swf_path).parent().unwrap();
    let mut executor = NullExecutor::new();
    let movie = SwfMovie::from_path(swf_path, None)?;
-    let frame_time = 1000.0 / movie.frame_rate().to_f64();
-    let frame_time_duration = Duration::from_millis(frame_time as u64);
+    let frame_time = Duration::from_millis(1000.0 / movie.frame_rate().to_f64());


This can be Duration::from_secs(1.0 / ...); now

higumachan · 2022-09-26T13:35:55Z

@Herschel

Sorry, this is late, but I have fixed the review points.

kmeisthax · 2022-11-24T01:09:40Z

Hmm... looks like this PR needs some TLC. I'll come back to it later.

…tes ago

…tion. At that time, the regression test with a duration of sub nano second was dropped.

… the mismerge

…ally drafted

kmeisthax · 2022-11-30T03:47:16Z

I've pushed an update to this PR that rebases it on a relatively-current master and attempts to fix a few incompatibilities that cropped up from new types adopting Instant and Duration.

I still need to come back to this. There's some stuff in web and desktop that relies on Instant arithmetic that isn't supported with the alternative Duration type.

iwannabethedev · 2023-04-29T11:53:39Z

Is this PR difficult to update and review due to its size? If it is, I wonder whether it could be split into multiple smaller PRs - like, the first PR would be core/src/duration.rs . And after that is merged, a PR would be opened that takes a part of the changes in this PR, gets it merged, and then rinse and repeat with new, small PRs until all changes in this PR has been merged.

The advantage of having a large PR is, I would guess, that you can then try out the new type in all the places where it might be relevant, and thus uncover problems early on, which is very useful and help and can avoid backtracking late in the process. But, now that there is a large PR, I imagine that you already have that potential advantage of a large PR, and can then proceed with small PRs taken from this large PR.

iwannabethedev · 2023-04-29T12:35:03Z

@higumachan In case that you do not currently seek to work on this further, would it be alright with you for others to take a stab at getting these changes merged, for instance through multiple smaller PRs that reference this PR?

relrelb requested changes Aug 9, 2022

View reviewed changes

Herschel reviewed Aug 26, 2022

View reviewed changes

relrelb reviewed Sep 2, 2022

View reviewed changes

core/src/player.rs Show resolved Hide resolved

relrelb reviewed Sep 2, 2022

View reviewed changes

core/src/timer.rs Outdated Show resolved Hide resolved

relrelb approved these changes Sep 4, 2022

View reviewed changes

Herschel reviewed Sep 4, 2022

View reviewed changes

higumachan and others added 15 commits November 27, 2022 15:47

Merge branch 'master' into refactor/f64_to_duration higumachan 2 minu…

ec886e6

…tes ago

fix comments

24fec7e

To confirm, I changed the Duration of SOUND from MILLI SECOND to Dura…

18955ef

…tion. At that time, the regression test with a duration of sub nano second was dropped.

use round

fed9f74

replace other place f64 durations

9d9a863

implement RuffleDuration base f64

36a7084

Avoid using std::time::Duration as much as possible

5c3074b

fix clippy

baf4a79

cargo fmt

a2e3bbf

rename RuffleDuration -> Duration

1ca43fa

remove ordered_float dependencies

db26389

use +=

7e59e60

fix review point

5b28725

chore: Fix the lockfile and a few source files that got screwed up in…

a98f3e2

… the mismerge

core: Fix a handful of types that were added after this PR was origin…

042ee49

…ally drafted

kmeisthax force-pushed the refactor/f64_to_duration branch from 2673859 to 042ee49 Compare November 30, 2022 03:45

danielhjacobs added the T-refactor Type: Refactor / Cleanup label Sep 17, 2024

danielhjacobs added the A-other Area: Not covered by other area labels label Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use std::time::Duration for time intervals #7616

Use std::time::Duration for time intervals #7616

higumachan commented Aug 9, 2022

relrelb left a comment

n0samu commented Aug 10, 2022

relrelb commented Aug 12, 2022

higumachan commented Aug 13, 2022

Herschel left a comment

Herschel Aug 26, 2022

Herschel Aug 26, 2022

Herschel Aug 26, 2022

Herschel Aug 26, 2022

higumachan commented Sep 2, 2022

Herschel Sep 4, 2022

Herschel Sep 4, 2022

higumachan commented Sep 26, 2022

kmeisthax commented Nov 24, 2022

kmeisthax commented Nov 30, 2022

iwannabethedev commented Apr 29, 2023

iwannabethedev commented Apr 29, 2023

		#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Default, Serialize, Deserialize)]
		pub struct RuffleDuration(OrderedFloat<f64>);

		sha2 = "0.10.2"
		sha2 = "0.10.2"

Use std::time::Duration for time intervals #7616

Are you sure you want to change the base?

Use std::time::Duration for time intervals #7616

Conversation

higumachan commented Aug 9, 2022

relrelb left a comment

Choose a reason for hiding this comment

n0samu commented Aug 10, 2022

relrelb commented Aug 12, 2022

higumachan commented Aug 13, 2022

Herschel left a comment

Choose a reason for hiding this comment

Herschel Aug 26, 2022

Choose a reason for hiding this comment

Herschel Aug 26, 2022

Choose a reason for hiding this comment

Herschel Aug 26, 2022

Choose a reason for hiding this comment

Herschel Aug 26, 2022

Choose a reason for hiding this comment

higumachan commented Sep 2, 2022

Herschel Sep 4, 2022

Choose a reason for hiding this comment

Herschel Sep 4, 2022

Choose a reason for hiding this comment

higumachan commented Sep 26, 2022

kmeisthax commented Nov 24, 2022

kmeisthax commented Nov 30, 2022

iwannabethedev commented Apr 29, 2023

iwannabethedev commented Apr 29, 2023