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

feat(js): Added UI to update animation speed parameter (delta) #706

Merged
merged 3 commits into from
Feb 11, 2019
Merged

feat(js): Added UI to update animation speed parameter (delta) #706

merged 3 commits into from
Feb 11, 2019

Conversation

Poseclop
Copy link
Contributor

@Poseclop Poseclop commented Feb 8, 2019

Adds a button on the animation player controls that pops up a a menu and allows to select the animation rendering speed (delta parameter)

  • Added a new sub-component in AnimationPlayer (SimSpeedController)
  • Added delta to animation-parameters context
  • Added a onDeltaChange method on animation-parameters context to manage delta updates.

PS: Second pull request, rebuilt using the latest master from BlueBrayns/Brayns.

@bbpbuildbot
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@rolandjitsu rolandjitsu left a comment

Choose a reason for hiding this comment

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

As a side note on the {delta} state that was added to the animation context, it would actually require a bit more work as right now, if you refresh your browser or some other client connects and plays the animation, you loose the current value or it reflects a wrong value.

For that purpose, we would actually use the delta that is on {animationParams} (as the server keeps its value and it will be synced between clients). But the problem with that is that we also use it to play/stop.

Though, there is nothing you can do at the moment, I hope that you understand the issue.

import Menu from '@material-ui/core/Menu';
import MenuItem from '@material-ui/core/MenuItem';

export default class AnimSpeedController extends React.Component<IAnimSpeedControllerProps, IAnimSpeedControllerState> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export default class AnimSpeedController extends React.Component<IAnimSpeedControllerProps, IAnimSpeedControllerState> {
export default class AnimationSpeedController extends Component<Props, State> {

Copy link
Contributor

Choose a reason for hiding this comment

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

Also rename the file to animation-speed-controller.tsx.

js/apps/viewer/src/common/client/animation-context.tsx Outdated Show resolved Hide resolved
js/apps/viewer/src/common/client/animation-context.tsx Outdated Show resolved Hide resolved
js/apps/viewer/src/common/client/animation-context.tsx Outdated Show resolved Hide resolved
js/apps/viewer/src/common/client/animation-context.tsx Outdated Show resolved Hide resolved
js/apps/viewer/src/common/client/animation-context.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@rolandjitsu rolandjitsu left a comment

Choose a reason for hiding this comment

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

A few more changes left and it should be ok.

Also, align the speed controller button all the way to the right (near the right edge of the window). Use a combination of classes and styles for this (same as for the other buttons).


const animationSpeedId = 'animation-speed-controller';

function speedLabel(value: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move speedLabel fn right after the class.

}

export default class AnimationSpeedController extends Component<Props, State> {
state : State = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
state : State = {
state: State = {

anchorEl: null
}

openMenu = (evt : MouseEvent<HTMLElement>) => this.setState({ anchorEl: evt.currentTarget });
Copy link
Contributor

Choose a reason for hiding this comment

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

Run yarn test at the root of the js folder to catch these code format issues.

Suggested change
openMenu = (evt : MouseEvent<HTMLElement>) => this.setState({ anchorEl: evt.currentTarget });
openMenu = (evt: MouseEvent<HTMLElement>) => this.setState({anchorEl: evt.currentTarget});


openMenu = (evt : MouseEvent<HTMLElement>) => this.setState({ anchorEl: evt.currentTarget });

closeMenu = () => this.setState({ anchorEl: null });
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
closeMenu = () => this.setState({ anchorEl: null });
closeMenu = () => this.setState({anchorEl: null});


closeMenu = () => this.setState({ anchorEl: null });

changeAnimationSpeed = (evt : MouseEvent<HTMLSelectElement>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
changeAnimationSpeed = (evt : MouseEvent<HTMLSelectElement>) => {
changeAnimationSpeed = (evt: MouseEvent<HTMLSelectElement>) => {

{speedLabel(delta ? delta : 1)}
</Button>
<Menu
id='simulation-speed-controller'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id='simulation-speed-controller'
id={animationSpeedId}

interface Props {
delta?: number;
disabled?: boolean;
onDeltaChange(delta : number): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onDeltaChange(delta : number): void;
onDeltaChange(delta: number): void;

@@ -86,6 +89,9 @@ export class Controls extends PureComponent<Props> {
</IconButton>
</div>
</Tooltip>
<Tooltip title={"Animation speed"} placement="right" {...TOOLTIP_DELAY}>
<AnimationSpeedController delta={delta} disabled={disabled} onDeltaChange={onDeltaChange} />
Copy link
Contributor

Choose a reason for hiding this comment

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

For readability:

Suggested change
<AnimationSpeedController delta={delta} disabled={disabled} onDeltaChange={onDeltaChange} />
<AnimationSpeedController
delta={delta}
onDeltaChange={onDeltaChange}
disabled={disabled}
/>

disablePrev?: boolean;
disableNext?: boolean;
disabled?: boolean;
onPrev(): void;
onPlayToggle(): void;
onNext(): void;
onDeltaChange(delta : number): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onDeltaChange(delta : number): void;
onDeltaChange(delta: number): void;

});
}

onDeltaChange = async (delta : number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onDeltaChange = async (delta : number) => {
onDeltaChange = async (delta: number) => {

Copy link
Contributor

@rolandjitsu rolandjitsu left a comment

Choose a reason for hiding this comment

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

Just a few more format changes left to do and it should be good to go.

@@ -25,6 +25,7 @@ import {onKeyboardLockChange, onPageVisibilityChange} from '../../common/events'
import Controls from './controls';
import ProgressBar from './progress-bar';
import {frameToTimeStr} from './utils';
import AnimationSpeedController from './animation-speed-controller';
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this above the Controls import (run yarn test at the root of the js dir to catch these format issues, I must have mentioned it before).

@@ -208,6 +211,12 @@ export class AnimationPlayer extends PureComponent<Props> {
<Typography variant="caption">
{currentMs} / {totalMs} {animationParams.unit}
</Typography>
<span className={classes.spacer}></span>
<AnimationSpeedController
delta={delta}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use 4 spaces for indentation for all these comp. props 😉

Suggested change
delta={delta}
delta={delta}

@@ -15,7 +15,6 @@ import SkipPreviousIcon from '@material-ui/icons/SkipPrevious';

import {TOOLTIP_DELAY} from '../../common/constants';


Copy link
Contributor

Choose a reason for hiding this comment

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

Undo changes to this file, you have 3 blank lines that were removed.

@tribal-tec tribal-tec merged commit 09ea657 into BlueBrain:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants