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

HARP-13235: Fix altitude in mercator #2055

Merged
merged 3 commits into from
Jan 28, 2021

Conversation

atomicsulfate
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #2055 (a339006) into master (b20f388) will increase coverage by 0.16%.
The diff coverage is 82.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2055      +/-   ##
==========================================
+ Coverage   66.65%   66.81%   +0.16%     
==========================================
  Files         297      297              
  Lines       26369    26378       +9     
  Branches     5966     5969       +3     
==========================================
+ Hits        17575    17624      +49     
+ Misses       8794     8754      -40     
Impacted Files Coverage Δ
...rp-datasource-protocol/lib/TechniqueDescriptors.ts 95.16% <ø> (ø)
...re/harp-datasource-protocol/lib/TechniqueParams.ts 70.27% <ø> (ø)
@here/harp-vectortile-datasource/lib/OmvUtils.ts 84.90% <66.66%> (-3.10%) ⬇️
...vectortile-datasource/lib/VectorTileDataEmitter.ts 64.26% <80.00%> (+4.04%) ⬆️
@here/harp-datasource-protocol/lib/DecodedTile.ts 55.04% <100.00%> (+4.08%) ⬆️
.../harp-datasource-protocol/lib/StyleSetEvaluator.ts 89.31% <0.00%> (+0.25%) ⬆️
@here/harp-datasource-protocol/lib/Techniques.ts 72.64% <0.00%> (+0.94%) ⬆️
... and 1 more

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 b20f388...a339006. Read the comment docs.

@@ -92,7 +92,7 @@ class MercatorProjection extends Projection {
result.y =
(MercatorProjection.latitudeClampProject(geoPoint.latitudeInRadians) * 0.5 + 0.5) *
this.unitScale;
result.z = geoPoint.altitude ?? 0;
result.z = (geoPoint.altitude ?? 0) * this.getScaleFactor(result);
Copy link
Member

@robertoraggi robertoraggi Jan 20, 2021

Choose a reason for hiding this comment

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

i don't think this is a good idea. This will enforce applying the scaling factor to Z at projection time, resulting always in different heights for objects with large latitude span. So many objects will have oblique planes and their normal vector will also be different, effecting bounding boxes, shading and tessellation. Also, this change will also conflict with the semantics of extruded-technique's constantHeight.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed, I now scale the height on decoding depending on the technique parameter constantHeight, which now is available in all techniques.

@atomicsulfate atomicsulfate force-pushed the HARP-13235_FixMercatorAltitude branch 3 times, most recently from 5b3a689 to 8416574 Compare January 22, 2021 16:50
@atomicsulfate atomicsulfate marked this pull request as ready for review January 25, 2021 09:08
robertoraggi
robertoraggi previously approved these changes Jan 25, 2021
If datasource geometry is 3D and technique's constantHeight is
set to true or defaults to true (for store levels lower than 12), then
projected geometry height is scaled by target projection's scale factor.
@atomicsulfate atomicsulfate merged commit c4790fb into master Jan 28, 2021
@atomicsulfate atomicsulfate deleted the HARP-13235_FixMercatorAltitude branch January 28, 2021 14:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants