Skip to content

Fix/debug time slider #529

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.

Sign up for GitHub

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

Draft
ascibisz wants to merge 2 commits into
base: main
Choose a base branch
from
+1 −33
Draft

Fix/debug time slider #529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef9b672
remove extra on change / after change code that doesn't seem necessar…
ascibisz Jun 10, 2024
2a1ba9f
don't trigger buffering when we're jumping around
ascibisz Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 1 addition & 30 deletions src/components/PlaybackControls/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,36 +49,8 @@ const PlayBackControls = ({
timeUnits,
isEmpty,
}: PlayBackProps): JSX.Element => {
// Where to resume playing if simulation was playing before scrubbing
const [timeToResumeAfterScrubbing, setTimeToResumeAfterScrubbing] =
useState(-1);
const [timeInput, setTimeInput] = useState(firstFrameTime);

// - Gets called once when the user clicks on the slider to skip to a specific time
// - Gets called multiple times when user is scrubbing (every time the play head
// passes through a time value associated with a frame)
const handleTimeChange = (sliderValue: number): void => {
onTimeChange(sliderValue);
if (isPlaying) {
// Need to save the sliderValue as timeToResumeAfterScrubbing to use in handleSliderMouseUp,
// because the sliderValue argument available in handleSliderMouseUp is not accurate
// when the time between mouse down and mouse up is short.
setTimeToResumeAfterScrubbing(sliderValue);
pauseHandler();
} else if (timeToResumeAfterScrubbing >= 0) {
// Update value if user is still dragging
setTimeToResumeAfterScrubbing(sliderValue);
}
};

const handleSliderMouseUp = (): void => {
// Resume playing if simulation was playing before
if (timeToResumeAfterScrubbing >= 0) {
playHandler(timeToResumeAfterScrubbing);
setTimeToResumeAfterScrubbing(-1);
}
};

// Called after every keystroke
const handleTimeInputChange = (userInput: number | null): void => {
if (userInput !== null) {
Expand Down Expand Up @@ -148,8 +120,7 @@ const PlayBackControls = ({
</div>
<Slider
value={time}
onChange={handleTimeChange}
onAfterChange={handleSliderMouseUp}
onChange={onTimeChange}
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm... while I agree that this change does fix the buffering issue, as a user, I would expect playback to pause while I'm interacting with the slider. Could you confirm with UX whether this new behavior is something they're okay with ?

Ant now has an onChangeComplete instead of onAfterChange... could that fix some of the issues? https://ant.design/components/slider

tooltip={{ open: false }}
className={classNames(styles.slider, styles.item)}
step={timeStep}
Expand Down
3 changes: 0 additions & 3 deletions src/containers/ViewerPanel/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,6 @@ class ViewerPanel extends React.Component<ViewerPanelProps, ViewerPanelState> {
lastFrameTime,
timeStep,
isBuffering,
setBuffering,
} = this.props;
if (isBuffering) {
return;
Expand All @@ -361,8 +360,6 @@ class ViewerPanel extends React.Component<ViewerPanelProps, ViewerPanelState> {
if (isTimeGreaterThanLastFrameTime || isTimeLessThanFirstFrameTime) {
return;
}

setBuffering(true);
simulariumController.gotoTime(time);
}

Expand Down