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

Accurate near distance for spherical projection. #2245

Merged
merged 4 commits into from
Aug 2, 2021

Conversation

atomicsulfate
Copy link
Collaborator

No description provided.

@atomicsulfate atomicsulfate changed the base branch from master to MAPSJS-2660_ClipPlanesCleanup July 23, 2021 10:43
@atomicsulfate atomicsulfate force-pushed the MAPSJS-2660_ClipPlanesCleanup branch 5 times, most recently from 4405328 to 468619d Compare July 23, 2021 16:44
@atomicsulfate atomicsulfate force-pushed the MAPSJS-2660_FixNearPlaneSphere branch 4 times, most recently from d1c4127 to ef367ba Compare July 26, 2021 16:13
@atomicsulfate atomicsulfate force-pushed the MAPSJS-2660_FixNearPlaneSphere branch 3 times, most recently from f9bae6b to 1cd6e56 Compare July 27, 2021 11:05
Copy link
Member

@nzjony nzjony left a comment

Choose a reason for hiding this comment

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

Review comments from first commit

@here/harp-mapview/lib/ClipPlanesEvaluator.ts Show resolved Hide resolved
): number | undefined {
const fwdDir = camera.getWorldDirection(tmpVectors[0]);
const nearPlaneTanPoint = tmpVectors[1].copy(fwdDir).multiplyScalar(-R);
const camToTan = nearPlaneTanPoint.sub(camera.position);
Copy link
Member

Choose a reason for hiding this comment

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

I would rename camToTan to be at least camToTanPoint to make it a bit clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

camToTanVec isn't quite accurate, because a vector is a relative displacement whereas a point is a fixed location. But I won't block this if you want to keep it.

@here/harp-mapview/lib/ClipPlanesEvaluator.ts Outdated Show resolved Hide resolved
@here/harp-mapview/lib/ClipPlanesEvaluator.ts Outdated Show resolved Hide resolved
@here/harp-mapview/lib/ClipPlanesEvaluator.ts Outdated Show resolved Hide resolved
@here/harp-mapview/lib/ClipPlanesEvaluator.ts Outdated Show resolved Hide resolved
@here/harp-mapview/lib/ClipPlanesEvaluator.ts Outdated Show resolved Hide resolved
@here/harp-mapview/lib/ClipPlanesEvaluator.ts Outdated Show resolved Hide resolved
@here/harp-mapview/lib/ClipPlanesEvaluator.ts Outdated Show resolved Hide resolved
@here/harp-mapview/lib/ClipPlanesEvaluator.ts Outdated Show resolved Hide resolved
Base automatically changed from MAPSJS-2660_ClipPlanesCleanup to master July 30, 2021 14:42
@atomicsulfate atomicsulfate force-pushed the MAPSJS-2660_FixNearPlaneSphere branch from 1cd6e56 to b3adf99 Compare July 30, 2021 17:28
Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
minGeometryHeight and maxGeometryHeight were undefined for fake elevation
DataSource. Because of this, calculation of min/max geometry height at
MapView.updateCameras() was returning NaN.

Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #2245 (4d84b6c) into master (c579784) will increase coverage by 0.12%.
The diff coverage is 97.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2245      +/-   ##
==========================================
+ Coverage   67.70%   67.82%   +0.12%     
==========================================
  Files         312      313       +1     
  Lines       27660    27690      +30     
  Branches     6188     6198      +10     
==========================================
+ Hits        18726    18780      +54     
+ Misses       8934     8910      -24     
Impacted Files Coverage Δ
@here/harp-mapview/lib/ClipPlanesEvaluator.ts 65.04% <96.29%> (+11.68%) ⬆️
@here/harp-mapview/lib/FixedClipPlanesEvaluator.ts 100.00% <100.00%> (ø)
@here/harp-mapview/lib/MapMaterialAdapter.ts 92.14% <0.00%> (-0.72%) ⬇️

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 c579784...4d84b6c. Read the comment docs.

@atomicsulfate atomicsulfate force-pushed the MAPSJS-2660_FixNearPlaneSphere branch 2 times, most recently from 117daf9 to 5d10d76 Compare August 2, 2021 09:53
@atomicsulfate
Copy link
Collaborator Author

@harpgl-bot retest this please

Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
@atomicsulfate atomicsulfate merged commit 8ecd5f7 into master Aug 2, 2021
@atomicsulfate atomicsulfate deleted the MAPSJS-2660_FixNearPlaneSphere branch August 2, 2021 12:26
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.

None yet

2 participants