Skip to content

Control Screensaver Animation Speed Acroos Different Hardware & Emulator #733

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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

1abdelhalim
Copy link

@1abdelhalim 1abdelhalim commented Apr 15, 2025

Description

This PR ensures the screensaver animation runs at a consistent speed across all Raspberry Pi models, including the SeedSigner Emulator.
Previously, the logo moved too fast on more powerful devices due to differences in hardware rendering speeds.


Changes:

  • Reworked animation to use time-based velocity (pixels/second) instead of frame-based movement
  • Calculates movement based on minimum screen dimension to ensure proper scaling on different display sizes
  • Added sub-pixel movement accumulation to prevent animation from getting stuck at low speeds
  • Added a minimal sleep (0.001s) to slightly reduce CPU usage without affecting smoothness

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

  • I’ve run pytest and made sure all unit tests pass before submitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

@kdmukai
Copy link
Contributor

kdmukai commented Apr 16, 2025

If I'm reading this right, your approach sets an effective max frame rate (faster screen refresh cycles are ignored until the threshold time has elapsed). But the velocity / pixels-per-update is not altered based on any frame rendering speed metric.

So just making up numbers, for a given initial rand velocity, we might see:

  • Pi Zero: 10fps, moves 3 pixels per frame: 30 total pixels moved per second
  • Pi Zero 2W: 15fps, moves 3 pixels per frame: 45 total pixels moved per second
  • More powerful hardware: 25fps, moves 3 pixels per frame: 75 total pixels moved per second
  • Mega-powerful hardware: 25fps, moves 3 pixels per frame (max frame rate already reached): 75 total pixels moved per second

The more powerful setups might be capable of, say, 60fps but this approach will restrict it to the max effective frame rate.

INSTEAD: I was thinking that the rand velocity would be constant on a per-second basis (e.g. 30 total pixels moved per second no matter what hardware it runs on) but it would be chopped up into n individual frames depending on the hardware rendering times.

This way we gain:

  • Guarantee the same effective pixel velocity per second across all platforms.
  • Maximize smooth motion (esp on faster hardware) by using each available frame update.

@1abdelhalim
Copy link
Author

You're right, your approach is better. I was mainly focusing on controlling the speed, but I now see how your idea will provide smoother motion across all devices.
I'll give it a try

@1abdelhalim
Copy link
Author

It should now behave consistently across all hardware. Appreciate any feedback

# pixels to move per second
self.pixels_per_second = 150.0

# Direction of movement in x and y
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this right, these vars aren't just the direction but rather a kind of magnitude scalar for each axis.

In which case I don't think "direction" is the right naming convention to use. At the moment "scalar" is the best term I can think of. Or "direction_multiplier". Or "delta_rate_x"? "cur_vector" with x/y component?

@@ -96,8 +96,6 @@ def _render(self):
# Display version num below SeedSigner logo
font = Fonts.get_font(GUIConstants.get_body_font_name(), GUIConstants.get_top_nav_title_font_size())
version = f"v{controller.VERSION}"

# The logo png is 240x240, but the actual logo is 70px tall, vertically centered

This comment was marked as resolved.

@@ -84,7 +84,7 @@ def _render(self):

background = Image.new("RGBA", size=self.logo.size, color="black")
if not self.is_screenshot_renderer:
# Fade in alpha
Copy link
Contributor

Choose a reason for hiding this comment

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

We always try to over-explain things via comments. Do not remove unless a comment is wrong, no longer relevant, etc.

- Simplified `rand_speed()` to return values between `min_speed` and `max_speed`.
- Made speed scale with screen’s smallest dimension.
- Renamed direction vars to `vector_x` / `vector_y`.
- Casted `cur_x` / `cur_y` to integers to fix rendering issues.
min_screen_dimension = min(self.renderer.canvas_width, self.renderer.canvas_height)
# Min speed: cross the screen in 3 seconds, max: cross in 1 second
self.min_speed = min_screen_dimension / 3.0 # pixels per second
self.max_speed = min_screen_dimension / 1.0 # pixels per second
Copy link
Contributor

@kdmukai kdmukai Apr 21, 2025

Choose a reason for hiding this comment

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

In my testing, min / max speeds worked best if it took 1.5s - 10s to cross the screen in any one dimension.

But note that because of your rand_direction implementation, the speeds specified here do NOT correspond to the actual min/max speed you're achieving.

min_speed is X but you're scaling it by a fractional rand_direction so the actual minimum speed can be much slower, approaching zero.

@kdmukai
Copy link
Contributor

kdmukai commented Apr 21, 2025

The current iteration of this strikes me as a confused in-between version that does not fully incorporate updated ideas while still being burdened by some old ones.

As noted in my comment above, min/max speed are not the actual min/max achieved.

The vector_* vars store direction and magnitude, but then are further scaled by current_speed. But that creates a new problem where the effective magnitude can approach zero, thus requiring the need for the accumulator concept.

I hacked around in your code and simplified it to:

  • eliminated current_speed completely.
  • eliminated rand_direction completely.
  • vector updates become just: self.vector_x = -1 * self.rand_speed() (if we're hitting the right edge)
  • No need for accumulator.
  • add a check if there are any net pixel movement changes; if not, continue to let more time elapse. Time elapsing IS the accumulator.

self.increment_y *= -1.0
# Handle boundary collisions and ensure we don't get stuck
if self.cur_x <= self.min_coords[0]:
self.cur_x = self.min_coords[0] + 1 # Move slightly away from edge
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to worry about 1 pixel differences. The logo bounce is calculated from the center of the logo; there aren't any extreme edge scenarios to worry about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants