-
Notifications
You must be signed in to change notification settings - Fork 197
MAPSJS-2660: Support off-center projection in MapControls. #2281
Conversation
8b7dbcc
to
75fb6bb
Compare
b068983
to
293c9d6
Compare
…eenPoint. Deprecate old orbitAroundScreenPoint signature. Create new signature where orbit center is optional, defaulting to camera principal point. Signed-off-by: Andres Mandado <[email protected]>
293c9d6
to
e91b2c7
Compare
Codecov Report
@@ Coverage Diff @@
## master #2281 +/- ##
==========================================
+ Coverage 67.95% 67.96% +0.01%
==========================================
Files 315 315
Lines 27766 27775 +9
Branches 6215 6219 +4
==========================================
+ Hits 18869 18878 +9
Misses 8897 8897
Continue to review full report at Codecov.
|
/** | ||
* Parameters for {@link orbitAroundScreenPoint}. | ||
*/ | ||
export interface OrbitParams { |
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.
Great idea, this makes the code much more readable IMO that just passing in parameters.
@here/harp-mapview/lib/Utils.ts
Outdated
/** | ||
* Delta tilt in radians. | ||
*/ | ||
deltaTilt: number; |
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.
You could consider to make this optional, at least I see you pass in tilt : 0 a few times.
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.
done for both deltaTilt and deltaAzimuth.
@here/harp-mapview/lib/Utils.ts
Outdated
deltaTilt?: number, | ||
maxTiltAngle?: number | ||
): void { | ||
const ppalPoint = CameraUtils.getPrincipalPoint(mapView.camera, cache.vector2[1]); |
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.
Why do you use the cache [1] vector. Do you expect some valid value to be in [0].
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.
changed this one to [0] and the one for the orbit center (below) to [1].
projection === sphereProjection | ||
? 1e-7 // FIXME: Is this huge error expected? | ||
: Number.EPSILON | ||
); | ||
}); | ||
it("limits tilt when orbiting around screen point", function () { | ||
for (const startTilt of [0, 20, 45]) { |
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.
why do you remove 20, 45?
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.
good catch, I forgot to change that back.
Signed-off-by: Andres Mandado <[email protected]>
* MAPSJS-2660: Support oblique perspective projection in orbitAroundScreenPoint. Deprecate old orbitAroundScreenPoint signature. Create new signature where orbit center is optional, defaulting to camera principal point. Signed-off-by: Andres Mandado <[email protected]> * MAPSJS-2660: Address review comments. Signed-off-by: Andres Mandado <[email protected]> Signed-off-by: Fischer, Thomas <[email protected]>
No description provided.