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

docs(examples): add animation and FPS counter to colors_rgb #583

Merged
merged 6 commits into from Dec 17, 2023

Conversation

joshka
Copy link
Member

@joshka joshka commented Oct 22, 2023

This doesn't render nicely in VHS yet unfortunately (there's some odd screen tearing that happens)

Screen.Recording.2023-10-22.at.3.23.45.AM.mov

examples/colors_rgb.rs Outdated Show resolved Hide resolved
examples/colors_rgb.rs Outdated Show resolved Hide resolved
examples/colors_rgb.rs Outdated Show resolved Hide resolved
examples/colors_rgb.rs Show resolved Hide resolved
This doesn't render nicely in VHS yet unfortunately
- Rebased
- changed hooks to use color_eyre and shuffled some of the init code
- Tweaked the VHS tape (it still renders glitches though)
- ran the for loop for setting up colors to 1 larger than the column
  and row counts to avoid off by one errors
VHS makes glitches in thish gif due to the animation
@joshka joshka force-pushed the colors-rgb-animation branch 3 times, most recently from a154dab to 43617de Compare December 17, 2023 01:10
@joshka
Copy link
Member Author

joshka commented Dec 17, 2023

Screen.Recording.2023-12-16.at.5.12.22.PM.mov

@joshka
Copy link
Member Author

joshka commented Dec 17, 2023

Ok - I think this is in the right place now.

@Valentin271
Copy link
Member

Did you tested this? I still got it to panic quite easily.

I'm afraid this is related to fps and computer performance. If you didn't got it to panic that's probably the case

@Valentin271
Copy link
Member

Valentin271 commented Dec 17, 2023

I really think this is because the frame changes size between the resize event and the next draw. So I included the setup_colors call in the draw loop where the frame.size() doc says "This is guaranteed not to change when rendering". I got it to work with that.

Could you try these changes? fix-panic-colors_rgb.patch (had to rename it to .txt to upload on github, you can git apply <file>). If you include these, feel free to rearrange as you see fit.
Might also require a comment somewhere to explain all this.

@joshka
Copy link
Member Author

joshka commented Dec 17, 2023

The problem is cause by Terminal::draw internally calling autoresize before creating the frame (this is intentiona). If the resize event occurred twice since the previous draw, then the second event may not have been seen by the app (but the size is changed regardless). So the app really can't rely on the size from the resize event to be the size to use for sizing the colors cache. It has to instead use the size from the Frame.

@Valentin271
Copy link
Member

Yep that's what I did in the patch. Ultimately this is going to be a problem in every app that want to use the resize event then ...

@joshka
Copy link
Member Author

joshka commented Dec 17, 2023

Yep that's what I did in the patch. Ultimately this is going to be a problem in every app that want to use the resize event then ...

yep

Copy link
Member

@Valentin271 Valentin271 left a comment

Choose a reason for hiding this comment

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

Looks good, happy to merge that

@joshka joshka merged commit 1b8b626 into ratatui-org:main Dec 17, 2023
33 checks passed
@joshka joshka deleted the colors-rgb-animation branch December 17, 2023 09:35
@joshka
Copy link
Member Author

joshka commented Dec 17, 2023

Yep that's what I did in the patch. Ultimately this is going to be a problem in every app that want to use the resize event then ...

#697

joshka added a commit that referenced this pull request Dec 17, 2023
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.

None yet

3 participants