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

MAPSJS-2660: Support off-center projection in getFitBoundsDistance() #2294

Merged
merged 3 commits into from
Aug 25, 2021

Conversation

atomicsulfate
Copy link
Collaborator

No description provided.

Also add more unit tests.

Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
@atomicsulfate atomicsulfate force-pushed the MAPSJS-2660_FitBoundsDistance branch 2 times, most recently from c522597 to 1ffdbe5 Compare August 23, 2021 15:44
Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #2294 (6b79257) into master (a3b58b6) will increase coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2294      +/-   ##
==========================================
+ Coverage   67.96%   68.23%   +0.27%     
==========================================
  Files         315      315              
  Lines       27775    27755      -20     
  Branches     6219     6220       +1     
==========================================
+ Hits        18877    18940      +63     
+ Misses       8898     8815      -83     
Impacted Files Coverage Δ
@here/harp-mapview/lib/Utils.ts 80.08% <100.00%> (-0.89%) ⬇️
@here/harp-mapview/lib/ClipPlanesEvaluator.ts 72.46% <0.00%> (+0.48%) ⬆️
@here/harp-mapview/lib/poi/PoiManager.ts 14.28% <0.00%> (+1.42%) ⬆️
@here/harp-mapview/lib/image/MipMapGenerator.ts 95.91% <0.00%> (+2.04%) ⬆️
@here/harp-mapview/lib/image/Image.ts 93.33% <0.00%> (+2.22%) ⬆️
@here/harp-mapview/lib/poi/PoiRenderer.ts 72.59% <0.00%> (+29.36%) ⬆️

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 a3b58b6...6b79257. Read the comment docs.

tilt: 80,
heading: 30
},
ppalPoint: { x: -0.8, y: 0.1 }
Copy link
Member

Choose a reason for hiding this comment

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

please add a test at the extremes, i.e. -1, 1, and make sure they don't break etc.

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

// |<-------->| Ps
// constD pEyeZ| /| ^
// |<-->|<--->| / | |
// | | | / | | |ndcY-O.y|*h/2
Copy link
Member

Choose a reason for hiding this comment

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

please document what h is to remove any potential ambiguity

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

// | _-` Ps - P projected on screen.
// P-` O - Principal point.
//
// Diagram showing how to calculate the camera distance on the camera YZ plane so that a
Copy link
Member

Choose a reason for hiding this comment

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

I didn't understand the diagrams till I had read the description, and even then it wasn't fully clear.

They should have some title, and the second diagram should have some description that highlights that we are computing the factor to move C from C0 to C1. Also, it should be highlighted that Ps is on the edge of the screen. I know it is described later, but it would be good to have it documented here too.

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

Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
@atomicsulfate atomicsulfate merged commit cdb3d50 into master Aug 25, 2021
@atomicsulfate atomicsulfate deleted the MAPSJS-2660_FitBoundsDistance branch August 25, 2021 08:51
ThFischer pushed a commit that referenced this pull request Aug 30, 2021
…2294)

* MAPSJS-2660: Simplify getFitBoundsDistance implementation.

Also add more unit tests.

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

* MAPSJS-2660: Support off-center projection in getFitBoundsDistance.

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

* MAPSJS-2660: Address review comments.

Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
Signed-off-by: Fischer, Thomas <thomas.fischer@here.com>
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