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

HARP-13329: Fix antimeridian cracks on ground planes. #2018

Merged
merged 5 commits into from
Jan 6, 2021

Conversation

atomicsulfate
Copy link
Collaborator

It does not include:

  • Fix for sphere projection.
  • Fix for cracks on HERE raster tile source images.

Thank you for contributing to harp.gl!

Before requesting a pull request, please remember to check the following documents:

If you are adding new functionality we would highly appreciate if you can describe what is the capability you are adding and even better if you can add some examples. Please also remember to add tests for it.

CI Check

Our bots will check whether your PR can be directly integrated into the mainline. We have some internal integration tests running on the background, our bots will inform you of the next steps and someone from our team will take a look and help if needed!

And please do not forget to sign-off your commit! You can read more about DCO here. But, in short, you just need to use git commit -s or append --signoff when you are committing to the repo.

Happy contributing!

@atomicsulfate atomicsulfate force-pushed the HARP-13329_FixAntiMerCracksPlanar branch from 36d2b9b to 307afc9 Compare December 16, 2020 15:26
@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #2018 (1d92354) into master (628ae16) will increase coverage by 0.15%.
The diff coverage is 97.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2018      +/-   ##
==========================================
+ Coverage   66.26%   66.41%   +0.15%     
==========================================
  Files         294      297       +3     
  Lines       26313    26336      +23     
  Branches     5930     5939       +9     
==========================================
+ Hits        17435    17492      +57     
+ Misses       8878     8844      -34     
Impacted Files Coverage Δ
@here/harp-mapview/lib/BoundsGenerator.ts 95.87% <ø> (ø)
...re/harp-mapview/lib/geometry/RegisterTileObject.ts 81.25% <81.25%> (ø)
@here/harp-geometry/lib/SubdivisionModifier.ts 78.49% <100.00%> (+0.47%) ⬆️
@here/harp-mapview/lib/BackgroundDataSource.ts 97.67% <100.00%> (ø)
@here/harp-mapview/lib/geometry/AddGroundPlane.ts 100.00% <100.00%> (ø)
...rp-mapview/lib/geometry/ProjectTilePlaneCorners.ts 100.00% <100.00%> (ø)
...e/harp-mapview/lib/geometry/TileGeometryCreator.ts 70.52% <100.00%> (-0.01%) ⬇️
@here/harp-webtile-datasource/lib/WebTileLoader.ts 100.00% <100.00%> (ø)
... and 4 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 628ae16...1d92354. Read the comment docs.

@atomicsulfate
Copy link
Collaborator Author

@harpgl-bot retest this please

2 similar comments
@atomicsulfate
Copy link
Collaborator Author

@harpgl-bot retest this please

@atomicsulfate
Copy link
Collaborator Author

@harpgl-bot retest this please

@atomicsulfate atomicsulfate force-pushed the HARP-13329_FixAntiMerCracksPlanar branch 3 times, most recently from 82dbee1 to 98fef34 Compare December 18, 2020 15:07
@atomicsulfate atomicsulfate changed the title HARP-13329: Fix antimeridian cracks on ground planes for planar. HARP-13329: Fix antimeridian cracks on ground planes. Jan 6, 2021
It does not include:
- Fix for sphere projection.
- Fix for cracks on HERE raster tile source images.
Use Float64Array for position attribute during geometry subdivision
and coord transformation.
@atomicsulfate atomicsulfate force-pushed the HARP-13329_FixAntiMerCracksPlanar branch from 98fef34 to fcb32cc Compare January 6, 2021 09:19
@atomicsulfate atomicsulfate force-pushed the HARP-13329_FixAntiMerCracksPlanar branch from fcb32cc to 1d92354 Compare January 6, 2021 10:58
);

if (!shouldSubdivide) {
return new THREE.Mesh(geometry, material);
Copy link
Member

Choose a reason for hiding this comment

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

You need to convert the Float64Array back to Float32Array also in this case.

Copy link
Collaborator Author

@atomicsulfate atomicsulfate Jan 6, 2021

Choose a reason for hiding this comment

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

If !shouldSubdivide, then useLocalTargetCoords == true. You can see in createPlaneGeometry() that in that case whe use FLoat32Array:

const bufferArray = useLocalTargetCoords ? new Float32Array(12) : new Float64Array(12);

Copy link
Member

Choose a reason for hiding this comment

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

I'm not such a fan of having the useLocalTargetCoords control the transformation to local coordinates in the createPlaneGeometry function, I think it makes it easier to read when this transformation is explicitly outside of the function, like it was previously with moveTileCenter, so that it isn't responsible for too many things. But because @FraukeF already approve, I won't block it.

);

if (!shouldSubdivide) {
return new THREE.Mesh(geometry, material);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not such a fan of having the useLocalTargetCoords control the transformation to local coordinates in the createPlaneGeometry function, I think it makes it easier to read when this transformation is explicitly outside of the function, like it was previously with moveTileCenter, so that it isn't responsible for too many things. But because @FraukeF already approve, I won't block it.

@atomicsulfate atomicsulfate merged commit 2c4ebb6 into master Jan 6, 2021
@atomicsulfate atomicsulfate deleted the HARP-13329_FixAntiMerCracksPlanar branch January 6, 2021 14:16
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.

3 participants