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

issues with rotationAmountProperty #274

Closed
pixelzoom opened this issue Dec 18, 2018 · 2 comments
Closed

issues with rotationAmountProperty #274

pixelzoom opened this issue Dec 18, 2018 · 2 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 18, 2018

Related to code review #259.

WaveScreenModel defines rotationRange and rotationAmountProperty like this:

 const rotationRange = new Range( 0, 1 );

// @public - amount the 3d view is rotated. 0 means top view, 1 means side view.
this.rotationAmountProperty = new NumberProperty( 0, {
  range: rotationRange
} );

Besides the fact that something named "rotation amount" should be something like a rotation angle, not some magic constants... This leads to code smell elsewhere. There are numerous places where the magic 0 and 1 are interpreted (sometimes to 1 decimal place, for some reason).

WaveScreenModel:

264 rotationAmount => rotationAmount !== rotationRange.min && rotationAmount !== rotationRange.max );
...
320 this.rotationAmountProperty.value = Util.clamp( this.rotationAmountProperty.value + wallDT * sign * 1.4, 0, 1 );

WaveScreenView:

384     waterSideViewNode.visible = rotationAmount === 1.0 && scene === model.waterScene;
385     waterGrayBackground.visible = rotationAmount !== 0 && scene === model.waterScene;

WaterDropLayer:

76 const fullyRotated = model.rotationAmountProperty.value === 1.0;

WaveMeterNode:

101 if ( model.rotationAmountProperty.value !== 0 && model.sceneProperty.value === model.waterScene ) {

And my personal favorite, Perspective3DNode:

117 const rotationAmount = Easing.CUBIC_IN_OUT.value( this.rotationAmountProperty.get() );

I think you'd be way better off if you:

(1) rename ViewType to ViewpointEnum, with values TOP and SIDE

(2) ditch rotationRange and rotationAmountProperty

(3) define viewpointProperty in WaveScreenModel:

this.viewpointProperty = new Property( ViewpointEnum.TOP, {
  validValues: ViewpointEnum.VALUE
} );

Then made changed your logic to make decisions based on whether viewpointProperty.value is TOP or SIDE. E.g. in WaveScreenView:

WaveScreenView:

384     waterSideViewNode.visible = ( viewpointProperty.value === ViewpointEnum.SIDE && scene === model.waterScene );
385     waterGrayBackground.visible = ( viewpointProperty.value === ViewpointEnum.SIDE && scene === model.waterScene );
@pixelzoom
Copy link
Contributor Author

Ah... so in WaveScreenModel:

this.rotationAmountProperty.value = Util.clamp( this.rotationAmountProperty.value + wallDT * sign * 1.4, 0, 1 );

So 0 and 1 are not the only values, it does range continuously between 0 and 1, presumably to rotate between top and side views.

Then I'm not sure what to do here. It sure is confusing, and it's probably because of the rotatationRange and rotationAmountProperty names.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 20, 2018

This was addressed by @samreid in REVIEW comments. We stuck with rotationAmountProperty with the same semantics. rotationRange was changed to rotationAmountRange.

Closing.

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

No branches or pull requests

2 participants