Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

HARP-13852 Increase precision, otherwise tilt is too inaccurate #2062

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

nzjony
Copy link
Member

@nzjony nzjony commented Jan 19, 2021

  • Improve the epsilon used when comparing the dot product.
  • When the tilt animation has finished, a final check is done to see if the targeted tilt is reached, and if not, we apply the delta needed to reach the desired tilt.

@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #2062 (b96f0e6) into master (d2c4078) will not change coverage.
The diff coverage is 66.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2062   +/-   ##
=======================================
  Coverage   66.63%   66.63%           
=======================================
  Files         297      297           
  Lines       26338    26341    +3     
  Branches     5951     5952    +1     
=======================================
+ Hits        17550    17552    +2     
- Misses       8788     8789    +1     
Impacted Files Coverage Δ
@here/harp-map-controls/lib/MapControls.ts 69.86% <40.00%> (-0.02%) ⬇️
@here/harp-mapview/lib/Utils.ts 81.38% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2c4078...b96f0e6. Read the comment docs.

@nzjony nzjony force-pushed the HARP-13852 branch 2 times, most recently from d25868c to dcf7298 Compare January 19, 2021 15:19
@atomicsulfate atomicsulfate self-requested a review January 19, 2021 16:05
Comment on lines 631 to 646
// There may be some numerical inaccuracies in the application of the
// `deltaAngle` during the animation, hence we check one last time to see if we
// need to fix the tilt
const mapViewTiltRad = THREE.MathUtils.degToRad(this.mapView.tilt);
const deltaAngle = (this.m_targetedTilt ?? mapViewTiltRad) - mapViewTiltRad;
if (deltaAngle !== 0) {
MapViewUtils.orbitAroundScreenPoint(
this.mapView,
0,
0,
0,
deltaAngle,
this.m_maxTiltAngle
);
this.updateMapView();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the call to this.stopTilt() is done to soon. It sets:
this.m_targetedTilt = this.m_currentTilt = undefined;

Then later we pass to easeOutCubic() as target value this.targetedTilt, which returns the current mapView.tilt if this.m_targetedTilt is undefined.

If the stopTilt() call is done after easOutCubic, together with your epsilon changes, it works fine for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @atomicsulfate. I have pushed a fix.

atomicsulfate
atomicsulfate previously approved these changes Jan 20, 2021
Copy link
Collaborator

@atomicsulfate atomicsulfate 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. It'd be even better if we add a unit test to check we actually reach 0 tilt.

@nzjony
Copy link
Member Author

nzjony commented Jan 20, 2021

@atomicsulfate , I agree, I will put the test in a separate PR, so we can at least get this into the release.

@nzjony
Copy link
Member Author

nzjony commented Jan 20, 2021

retest this please

- Improve the epsilon used when comparing the dot product.
- When the tilt animation has finished, a final check is done to see if the targeted
tilt is reached, and if not, we apply the delta needed to reach the desired tilt.

Signed-off-by: Jonathan Stichbury <[email protected]>
@nzjony nzjony disabled auto-merge January 20, 2021 15:30
@nzjony nzjony merged commit 67e1fcb into master Jan 20, 2021
@nzjony nzjony deleted the HARP-13852 branch January 20, 2021 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants