-
-
Notifications
You must be signed in to change notification settings - Fork 764
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
Fix unwanted roll #5083 #5092
Fix unwanted roll #5083 #5092
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5092 +/- ##
==========================================
+ Coverage 91.81% 91.82% +0.01%
==========================================
Files 278 278
Lines 38318 38346 +28
Branches 6703 6704 +1
==========================================
+ Hits 35181 35212 +31
+ Misses 3004 3001 -3
Partials 133 133 ☔ View full report in Codecov by Sentry. |
src/geo/projection/camera_helper.ts
Outdated
tr.setRoll(eulerAngles.roll); | ||
tr.setPitch(eulerAngles.pitch); | ||
tr.setBearing(eulerAngles.bearing); | ||
export function updateRotation(startRotation: quat, endRotation: quat, startEulerAngles: RollPitchBearing, endEulerAngles: RollPitchBearing, tr: ITransform, k: number, useSlerp: boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to define a type for this method to pass the parameters as there are many parameters now, too many for my taste...
We've just migrated the code to vitest, I'm trying to figure out if the coverage reports is accurate. |
It appear to be reporting at least some code as Covered on main And uncovered in this PR Despite being identical |
The report comment is saying that there is a missing uploaded file (8 files instead of 9). |
@NathanMOlson can you address the last code review comment so I can merge this and release a new version? |
We have recently updated the coverage reporter, I hope it's accurate, if so, the tests need to cover more of the new code. |
I don't think the coverage reporter is accurate. I believe the new code has 100% coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!!
Coverage seems to work correctly (hopefully). |
Launch Checklist
Fix #5083.
Use spherical linear interpolation only when
startRoll != endRoll
. Otherwise, use linear interpolation of Euler angles.CHANGELOG.md
under the## main
section.