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

HARP-10881: Support partial disabling of building edges #1721

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

ninok
Copy link
Contributor

@ninok ninok commented Jul 23, 2020

No description provided.

@ninok ninok force-pushed the disabled-edge-ibct branch 2 times, most recently from 160ee4b to 3aba419 Compare July 27, 2020 06:25
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #1721 into master will decrease coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1721      +/-   ##
==========================================
- Coverage   64.11%   64.10%   -0.01%     
==========================================
  Files         279      279              
  Lines       25310    25328      +18     
  Branches     5708     5719      +11     
==========================================
+ Hits        16227    16237      +10     
- Misses       9083     9091       +8     
Impacted Files Coverage Δ
@here/harp-datasource-protocol/lib/DecodedTile.ts 45.19% <ø> (ø)
...e/harp-mapview/lib/geometry/TileGeometryCreator.ts 70.78% <75.00%> (+0.01%) ⬆️
...e/harp-omv-datasource/lib/OmvDecodedTileEmitter.ts 60.04% <100.00%> (+0.18%) ⬆️
@here/harp-utils/lib/Logger/ConsoleChannel.ts 69.23% <0.00%> (-15.39%) ⬇️
@here/harp-mapview/lib/Tile.ts 85.17% <0.00%> (-0.64%) ⬇️

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 4df3fdd...7521113. Read the comment docs.

@ninok ninok marked this pull request as ready for review July 27, 2020 09:38
@@ -177,7 +178,7 @@ function mapViewFeaturesRenderingTest(

const grid = new THREE.GridHelper(gridSize, gridDivisions, 0xff0000, 0x00ff00);
grid.geometry.rotateX(Math.PI / 2);
(grid as MapAnchor).geoPosition = position;
(grid as MapAnchor).anchor = position;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just removes an annoying deprecation warning

@atomicsulfate atomicsulfate self-requested a review July 27, 2020 10:21
@@ -931,6 +931,7 @@ export class TileGeometryCreator {

if (renderDepthPrePass) {
const depthPassMesh = createDepthPrePassMesh(object as THREE.Mesh);
this.addUserData(tile, srcGeometry, technique, depthPassMesh);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need user data for depth prepass meshes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

b/c we also need to generate the appropriate draw calls for the enabled buildings in the depth-pre-pass.
Visually it makes no difference but the depth buffer would be polluted with all buildings (instead of only the enabled ones) and that might break usecases that rely on a proper depth buffer (e.g. car position marker).

Nevertheless, you have a point that we could reuse the userData from the "main" mesh so we would also avoid to re-compute the draw ranges again. Same for the outlines.
I will create a dedicated ticket for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it again, it should even make a difference visually in a highly tilted view. Unfortunately that's not covered by our testcases

@ninok ninok merged commit 336862f into master Jul 27, 2020
@ninok ninok deleted the disabled-edge-ibct branch July 27, 2020 10:49
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