Success animation for Model R #2339

TychoVrahe · 2022-06-16T14:19:08Z

Imeplements #2293

Implements also popup screen from design, but text is not centered (limitation of Paragraph implementation in rust, seems out of scope for this PR to implement that). Button in the popup also differs, since out current button implementation does not follow design. Fonts used in design are also not available.

Builds on #2276 which should be merged first.

matejcik

partial review so far

Overall, i worry about performance of the new functions, esp. generating the overlay pixel by pixel that incurs glyph lookup per every pixel. This might be good enough for 1bpp Trezor R, but I strongly suspect that it won't be good enough for the T.

Still, probably OK to keep it and worry about performance only when we need to.

matejcik · 2022-07-13T09:37:31Z

core/embed/rust/src/trezorhal/display.rs

@@ -136,3 +143,12 @@ pub fn set_window(x0: u16, y0: u16, x1: u16, y1: u16) {
        ffi::display_set_window(x0, y0, x1, y1);
    }
 }
+
+pub fn get_offset() -> (i32, i32) {


what is the purpose of this (and the original display_offset function)?

does it ever return non-zero?

Purpose of this is to be consistent with the C functions, what is/was the purpose of the original i don't know, as far as i can tell its not being set anywhere. (function setting the offset is also exposed to python, but even that does not seem to be called anywhere).

let's ask @prusnak

IIRC it was for some sort of scrolling / panning animation?

core/embed/rust/src/trezorhal/display.rs

core/embed/rust/src/trezorhal/ffi.rs

core/embed/rust/src/trezorhal/uzlib.rs

core/embed/rust/src/ui/display.rs

TychoVrahe · 2022-07-14T20:43:22Z

pushed some fixes requested in review and also:
fixed a off by one bug in a863432
removed confusing comment: 33c770c
removed unnecessary casting: 60127ff

mmilata

This is looking good but will need a few mostly cosmetic changes. Especially please use the higher-level geometric types as much as possible.

Please note that after rebase the animation doesn't work probably because I broke the timer handling in 3dcdffe and will change it again in #2335. This patch can be used in the meantime. With this workaround the animation is working fine!

core/embed/rust/src/trezorhal/uzlib.rs

core/.changelog.d/2293.added

mmilata · 2022-07-18T20:48:27Z

core/embed/rust/src/ui/display.rs

+    }
+}
+
+fn get_point_on_line(v0: (i32, i32), v1: (i32, i32), position: f32) -> (i32, i32) {


Please use geometry::Point instead of (i32, i32) wherever possible. Then you can take advantage of helper methods like Rect::contains which will simplify several if conditions in this PR.

done here b964e7e , hopefully i didn't miss much.

i also tried adding and iterator to Rect, which returned directly Point, to eliminate repetitive nested loop iterating: but it came with non negligible performance downgrade, so i left it as it was.

core/embed/rust/src/ui/display.rs

core/embed/rust/src/ui/model_tr/component/result_popup.rs

mmilata

Looking good, left some comments but otherwise don't see any issues.

ui/display.rs is growing quite large and it would be nice to split it into multiple files but we can do it in another PR since this one is pretty large already.

core/embed/rust/src/trezorhal/display.rs

core/embed/rust/src/ui/display.rs

mmilata · 2022-08-01T11:25:53Z

core/embed/rust/src/ui/display.rs

+    match angle % 360 {
+        0..=44 => Point::lerp(v[0], v[1], (angle) as f32 / 45_f32),
+        45..=89 => Point::lerp(v[1], v[2], (angle - 45) as f32 / 45_f32),
+        90..=134 => Point::lerp(v[2], v[3], (angle - 90) as f32 / 45_f32),
+        135..=179 => Point::lerp(v[3], v[4], (angle - 135) as f32 / 45_f32),
+        180..=224 => Point::lerp(v[4], v[5], (angle - 180) as f32 / 45_f32),
+        225..=269 => Point::lerp(v[5], v[6], (angle - 225) as f32 / 45_f32),
+        270..=314 => Point::lerp(v[6], v[7], (angle - 270) as f32 / 45_f32),
+        315..=359 => Point::lerp(v[7], v[0], (angle - 315) as f32 / 45_f32),
+        _ => Point::new(1000, 0),
+    }


So many constants scare me, what about something like:

let angle %= 360; let vertices = v.len(); let sector = angle / vertices; let sector_angle = angle % verticse as f32; let sector_length = 360 / vertices as f32; // only works if 360 is divisible by vertices let v1 = v[sector as usize]; let v2 = v[(sector + 1) % vertices as usize]; Point::lerp(v1, v2, inner_angle / sector_length)

Does using sin()/cos() cost us anything btw?

Using your suggestion: e18e0fb

about sin/con: one thing is that we don't satisfy #[lang = "f32_runtime"] so we don't have these available (and i don't know how to enable this and what other implications there might be if any). Then we would need to evaluate flash usage and execution time. But if we are gonna do some more animations and graphics, these might come handy, so it would be worth experimenting with at least.

Tried it with sin/cos: we don't have these function in rust since they are part of std, which we don't use. Tried using rust crate libm, which worked but it produced about 7kB larger binary. Tried also compiling&exposing C sin/cos functions, which is a bit better - about 4kB more. Didn't check the performance as the size is imo blocker until we really need it.

core/embed/rust/src/trezorhal/uzlib.rs

core/embed/rust/src/ui/display.rs

matejcik · 2022-08-09T14:06:20Z

looks good from my side. please rebase, squash as appropriate, and implement #2339 (comment)

core/embed/rust/src/trezorhal/uzlib.rs

…s in rust

TychoVrahe · 2022-08-10T12:12:46Z

Squashed, rebased, fixed #2339 (comment)

Also changed the animation end detection in e32aaf6 and 413ac64 since the issue has been fixed in meantime.

Open points:

offset handling - do we want to remove it? maybe do evaluation and possible removal in another issue?
recall @matejcik not being happy about Loader component name for use it the hold to confirm animation, any suggestions?
not happy about icon_rust name also. do we even want to keep this? (it has an advantage of being capable of drawing on odd x coordinate, but mainly i just used it as stepping stone for implementation of more complex display functions)

matejcik · 2022-08-10T12:23:09Z

offset handling - do we want to remove it? maybe do evaluation and possible removal in another issue?

Let's keep it for now and create an issue.
One thing I would love to try is to scroll off the window contents via the offset functionality.

recall @matejcik not being happy about Loader component name for use it the hold to confirm animation, any suggestions?

That is true. How about progress_button or button_progress ?
But we can do that separately too, probably in the same step where we introduce a proper "loader" for TR.

not happy about icon_rust name also. do we even want to keep this? (it has an advantage of being capable of drawing on odd x coordinate, but mainly i just used it as stepping stone for implementation of more complex display functions)

Let's keep it for now. I would like to do more in this direction, in particular, introduce a struct Icon, we can modify the icon_rust name in that step.
(maybe @grdddj already did something about a separate Icon type?)

grdddj · 2022-08-10T12:28:23Z

I would like to do more in this direction, in particular, introduce a struct Icon, we can modify the icon_rust name in that step.
(maybe @grdddj already did something about a separate Icon type?)

Yes, I am already using Icon struct as a convenient way to allow drawing/helper methods on the data (and also to name the icon for debugging purposes).
Current situation:

Icon is defined at https://github.com/trezor/trezor-firmware/blob/grdddj/trezor_r_passphrase_input/core/embed/rust/src/ui/display.rs#L1088-L1164
individual icons are created as const at https://github.com/trezor/trezor-firmware/blob/grdddj/trezor_r_passphrase_input/core/embed/rust/src/ui/model_tr/theme.rs#L19-L53

grdddj · 2022-08-10T12:34:01Z

It could be also relevant here - I have made a HoldToConfirm::icon and Loader::icon constructors, to enable me to create the HTC component even with icon data. And I assume we will want the possibility to use HTC with icon, the implementation should not be too different than the text one

TychoVrahe requested a review from matejcik June 16, 2022 14:19

TychoVrahe requested a review from prusnak as a code owner June 16, 2022 14:19

TychoVrahe force-pushed the tychovrahe/success_anim branch from 1a14b28 to 275ddcb Compare June 17, 2022 09:16

hynek-jina linked an issue Jun 21, 2022 that may be closed by this pull request

Success animation for Model R #2293

Closed

matejcik requested a review from mmilata June 21, 2022 12:16

matejcik assigned mmilata and matejcik Jun 21, 2022

TychoVrahe mentioned this pull request Jun 24, 2022

Hold to confirm animation for Model R #2334

Closed

prusnak removed their request for review June 26, 2022 13:43

grdddj mentioned this pull request Jul 8, 2022

Input methods for model R #2277

Closed

matejcik reviewed Jul 13, 2022

View reviewed changes

TychoVrahe linked an issue Jul 13, 2022 that may be closed by this pull request

Model R: Hold to confirm animation #2276

Closed

mmilata reviewed Jul 18, 2022

View reviewed changes

TychoVrahe force-pushed the tychovrahe/success_anim branch from d74b5fa to b964e7e Compare July 22, 2022 20:57

matejcik mentioned this pull request Jul 26, 2022

Improve image decompression speed and handling #2414

Closed

mmilata approved these changes Aug 1, 2022

View reviewed changes

TychoVrahe force-pushed the tychovrahe/success_anim branch from f6685c7 to c6e8aaf Compare August 2, 2022 08:51

matejcik reviewed Aug 9, 2022

View reviewed changes

matejcik reviewed Aug 10, 2022

View reviewed changes

core/embed/rust/src/trezorhal/uzlib.rs Outdated Show resolved Hide resolved

TychoVrahe added 5 commits August 10, 2022 13:45

feat(core/rust): make ButtonPos hit function public to enable reuse

aa3ebce

refactor(core/rust): separate lerp and animations

f0f5ef9

feat(core/rust): implement interpolate color and color table function…

f315d65

…s in rust

feat(core/rust): expose display offset handling to rust

726a637

feat(core/rust): glyph and text rendering in rust

74ad24d

TychoVrahe force-pushed the tychovrahe/success_anim branch from ce97b64 to 449cbc2 Compare August 10, 2022 11:50

matejcik approved these changes Aug 10, 2022

View reviewed changes

TychoVrahe added 4 commits August 12, 2022 13:19

feat(core/rust): hold to confirm animation for Model R

c1bda5c

feat(core/rust): expose uzlib functions to rust

a18954c

feat(core/rust): added success animation and rust icon rendering

9208ac9

feat(core/rust): autoclosing popup for Model R with success animation

6646250

TychoVrahe force-pushed the tychovrahe/success_anim branch from 413ac64 to 6646250 Compare August 12, 2022 11:20

TychoVrahe merged commit cc2bfd9 into master Aug 12, 2022

TychoVrahe deleted the tychovrahe/success_anim branch August 12, 2022 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Success animation for Model R #2339

Success animation for Model R #2339

TychoVrahe commented Jun 16, 2022

matejcik left a comment

matejcik Jul 13, 2022

TychoVrahe Jul 13, 2022

matejcik Jul 13, 2022

jpochyla Jul 14, 2022

TychoVrahe commented Jul 14, 2022

mmilata left a comment

mmilata Jul 18, 2022

TychoVrahe Jul 22, 2022

mmilata left a comment

mmilata Aug 1, 2022

TychoVrahe Aug 2, 2022

TychoVrahe Aug 10, 2022

matejcik commented Aug 9, 2022

TychoVrahe commented Aug 10, 2022

matejcik commented Aug 10, 2022

grdddj commented Aug 10, 2022

grdddj commented Aug 10, 2022

Success animation for Model R #2339

Success animation for Model R #2339

Conversation

TychoVrahe commented Jun 16, 2022

matejcik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TychoVrahe commented Jul 14, 2022

mmilata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik commented Aug 9, 2022

TychoVrahe commented Aug 10, 2022

matejcik commented Aug 10, 2022

grdddj commented Aug 10, 2022

grdddj commented Aug 10, 2022