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

Enable shadows on WebTiles #2125

Merged
merged 3 commits into from
Mar 2, 2021
Merged

Enable shadows on WebTiles #2125

merged 3 commits into from
Mar 2, 2021

Conversation

FraukeF
Copy link
Contributor

@FraukeF FraukeF commented Feb 26, 2021

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!

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #2125 (7747cf0) into master (e3a9ec3) will increase coverage by 0.00%.
The diff coverage is 84.61%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2125   +/-   ##
=======================================
  Coverage   67.68%   67.69%           
=======================================
  Files         298      298           
  Lines       26384    26389    +5     
  Branches     5968     5972    +4     
=======================================
+ Hits        17858    17864    +6     
+ Misses       8526     8525    -1     
Impacted Files Coverage Δ
@here/harp-mapview/lib/MapView.ts 73.65% <0.00%> (-0.07%) ⬇️
@here/harp-mapview/lib/geometry/AddGroundPlane.ts 100.00% <100.00%> (ø)
...-vectortile-datasource/lib/VectorTileDataSource.ts 71.59% <100.00%> (+2.62%) ⬆️
@here/harp-webtile-datasource/lib/WebTileLoader.ts 100.00% <100.00%> (ø)

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 e3a9ec3...2b734cd. Read the comment docs.

@FraukeF FraukeF force-pushed the webtile-shadows branch 3 times, most recently from 2befe61 to 5a4c65a Compare March 1, 2021 12:22
Signed-off-by: Frauke Fritz <frauke.fritz@here.com>
@FraukeF FraukeF marked this pull request as ready for review March 1, 2021 14:35
@FraukeF FraukeF changed the title DRAFT: Enable shadows on WebTiles Enable shadows on WebTiles Mar 1, 2021
@gairik gairik closed this Mar 1, 2021
@gairik gairik reopened this Mar 1, 2021
@@ -61,6 +62,12 @@ function swapCamera() {
shadowCameraHelper.visible = !shadowCameraHelper.visible;
}

const hereWebTileDataSource = new HereWebTileDataSource({
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for expanding the example!

material,
true,
false
this.dataSource.dataSourceOrder,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not sure if this is quite correct, because the dataSourceOrder is not the renderOrder, and this value is used internally as the renderOrder.

I think it would make more sense to remove the renderOrder from this function and just set the order to some small value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, the datasourceOrder is evaluated somewhere else. I ll remove it

Signed-off-by: Frauke Fritz <frauke.fritz@here.com>
@FraukeF FraukeF closed this Mar 2, 2021
@FraukeF FraukeF reopened this Mar 2, 2021
@FraukeF FraukeF closed this Mar 2, 2021
@FraukeF FraukeF reopened this Mar 2, 2021
Signed-off-by: Frauke Fritz <frauke.fritz@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.

3 participants