Skip to content

Comments

Remove frame lag when creating loop region#11862

Merged
emilk merged 1 commit intomainfrom
emilk/remove-frame-selection-frame-lag
Nov 12, 2025
Merged

Remove frame lag when creating loop region#11862
emilk merged 1 commit intomainfrom
emilk/remove-frame-selection-frame-lag

Conversation

@emilk
Copy link
Member

@emilk emilk commented Nov 11, 2025

This was already very noticeable at 60 Hz. My bad for only using 120Hz+ in my day-to-day

@emilk emilk added 📺 re_viewer affects re_viewer itself 📉 performance Optimization, memory use, etc include in changelog labels Nov 11, 2025
@github-actions
Copy link

github-actions bot commented Nov 11, 2025

Web viewer built successfully.

Result Commit Link Manifest
cc54b60 https://rerun.io/viewer/pr/11862 +nightly +main

View image diff on kitdiff.

Note: This comment is updated whenever you push a commit.

@emilk emilk added the consider-patch PRs & issues that should be considered to be cherry-picked to a patch release branch. label Nov 12, 2025
Copy link
Member

@IsseW IsseW 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, also nice to factor out into a function

let selected_range = time_commands
.iter()
.rev()
.find_map(|c| {
Copy link
Member

Choose a reason for hiding this comment

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

That's a neat way to do this!

.iter()
.rev()
.find_map(|c| {
if let TimeControlCommand::SetLoopSelection(range) = c {
Copy link
Member

Choose a reason for hiding this comment

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

Could possibly check for a RemoveLoopSelection here to also have that not be frame delayed. But not as important as setting it

Copy link
Member

Choose a reason for hiding this comment

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

More of a thought, not something I think needs any actions

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but removal is much less sensitive to frame delays than dragging is, so I rather not add the complexity now

Comment on lines +261 to +272
// Use latest range to avoid frame delay
let selected_range = time_commands
.iter()
.rev()
.find_map(|c| {
if let TimeControlCommand::SetLoopSelection(range) = c {
Some(AbsoluteTimeRangeF::from(*range))
} else {
None
}
})
.or(time_ctrl.loop_selection())?;
Copy link
Member

Choose a reason for hiding this comment

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

I wish there was a nicer way to query this out, feels hacky :/

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's that hacky when you consider that time_commands will almost always be empty. Might be nice to have a function for this if similar pattern pops up more times though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the nice way to re-arrange this at some point is to put the time commands into ctx and then have helpers on ctx to get the latest values, including pending commands. Maybe.

Comment on lines +290 to +296
let corner_radius = tokens.normal_corner_radius();
let corner_radius = egui::CornerRadius {
nw: corner_radius,
ne: corner_radius,
sw: 0,
se: 0,
};
Copy link
Member

Choose a reason for hiding this comment

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

pre-existing, but personally I found the corner radius here a bit strange

Image Image

🤷 no strong opinion on it, but can't unsee now

@emilk emilk merged commit fe5a3e5 into main Nov 12, 2025
51 checks passed
@emilk emilk deleted the emilk/remove-frame-selection-frame-lag branch November 12, 2025 14:57
IsseW pushed a commit that referenced this pull request Nov 12, 2025
This was already very noticeable at 60 Hz. My bad for only using 120Hz+
in my day-to-day
@IsseW IsseW removed the consider-patch PRs & issues that should be considered to be cherry-picked to a patch release branch. label Nov 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

include in changelog 📉 performance Optimization, memory use, etc 📺 re_viewer affects re_viewer itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants