Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use instant for Time::now() #2090

Merged
merged 1 commit into from
May 11, 2023
Merged

Use instant for Time::now() #2090

merged 1 commit into from
May 11, 2023

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented May 11, 2023

What

The rerun viewer wants to use Time::now() when creating a blueprint on-demand.

Checklist

PR Build Summary: https://build.rerun.io/pr/2090

@jleibs jleibs marked this pull request as ready for review May 11, 2023 17:13
@jleibs jleibs force-pushed the jleibs/now_on_web branch from 394025e to 949b6d0 Compare May 11, 2023 17:15
@jleibs jleibs added the 🕸️ web regarding running the viewer in a browser label May 11, 2023
Comment on lines +158 to +160
// On non-wasm32 builds, `instant::SystemTime` is a re-export of `std::time::SystemTime`,
// so it's covered by the above `TryFrom`.
#[cfg(target_arch = "wasm32")]
Copy link
Member

@emilk emilk May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So remove impl TryFrom<std::time::SystemTime> for Time { and have just impl TryFrom<instant::SystemTime> for Time { ?

That should help ensure we stop using std::time::SystemTime everywhere, and start switching to instant::SystemTime.

Same for Duration imho

Copy link
Member Author

@jleibs jleibs May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then on wasm, that wouldn't be compatible with a actual instance of std::time::SystemTime since on wasm it's a distinct type. My undestanding is it could still be validly created, just not using now().

Copy link
Member

@emilk emilk May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I was thinking that would be a good thing (force us to switch to instant), but we have external users that I didn't consider

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, still likely kind of a stretch -- would have to be an external user using SystemTime and compiling to wasm and wanting to interact with our rust APIs...

@jleibs jleibs merged commit 50e0026 into main May 11, 2023
@jleibs jleibs deleted the jleibs/now_on_web branch May 11, 2023 17:37
@jleibs jleibs mentioned this pull request May 11, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕸️ web regarding running the viewer in a browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants