Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BGDIINF_SB-2980: add GeoJSON and KML layer components for 3d #426

Merged
merged 9 commits into from
Aug 17, 2023

Conversation

vladyslav-tk
Copy link
Contributor

@vladyslav-tk vladyslav-tk commented Jul 10, 2023

@vladyslav-tk vladyslav-tk force-pushed the feat-BGDIINF_SB-2980_add_3d_geoJson_component branch 5 times, most recently from da91163 to 7a2cc91 Compare July 14, 2023 11:22
@vladyslav-tk vladyslav-tk requested a review from pakb July 14, 2023 11:52
@vladyslav-tk vladyslav-tk changed the title BGDIINF_SB-2980: WIP: add GeoJSON layer component for 3d BGDIINF_SB-2980: add GeoJSON layer component for 3d Jul 14, 2023
@vladyslav-tk vladyslav-tk marked this pull request as ready for review July 14, 2023 11:53
@vladyslav-tk vladyslav-tk force-pushed the feat-BGDIINF_SB-2980_add_3d_geoJson_component branch 4 times, most recently from c460d16 to fd77cbc Compare July 17, 2023 07:25
@vladyslav-tk vladyslav-tk changed the title BGDIINF_SB-2980: add GeoJSON layer component for 3d BGDIINF_SB-2980: add GeoJSON and KML layer components for 3d Aug 2, 2023
@vladyslav-tk
Copy link
Contributor Author

@pakb I added the KML layer component to this PR because it has related changes

Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

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

I've some comment regarding the code, but my biggest concern is that it is not up to standard on the rendering side.

Here's what it looks in 2D
image
And here is the same place after a switch in 3D
image

I don't mind that labels are not rendered (although they should), but I will not accept that the numbers and "pills" are clipped by the terrain, we have to find a solution to this.

src/modules/map/components/cesium/CesiumMap.vue Outdated Show resolved Hide resolved
src/modules/map/components/cesium/CesiumMap.vue Outdated Show resolved Hide resolved
tests/e2e-cypress/integration/3d/layers.cy.js Show resolved Hide resolved
tests/e2e-cypress/support/commands.js Outdated Show resolved Hide resolved
@vladyslav-tk
Copy link
Contributor Author

@pakb

I don't mind that labels are not rendered (although they should)

for cesium, we use predefined style resolution. Currently, it is set to 100 which is why labels are not shown, I will update it

I will not accept that the numbers and "pills" are clipped by the terrain, we have to find a solution to this.

There are a few points about this

  1. New terrain has problems with some tiles, and initially, points can be placed wrong image
  2. Cesium works like this, in some cases points will be cut by terrain anyway. It works the same in the current geo.admin
    image
    image
  3. Only way to always show points is to use disableDepthTestDistance but in this case, it will be visible throw the terrain
    image
    image

@vladyslav-tk
Copy link
Contributor Author

@pakb I added disableDepthTestDistance and set it to 75km, please test. Should we keep it like this, adjust the distance, or remove it?

@vladyslav-tk vladyslav-tk force-pushed the feat-BGDIINF_SB-2980_add_3d_geoJson_component branch from efffbec to de5b0f4 Compare August 14, 2023 13:01
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

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

@pakb I added disableDepthTestDistance and set it to 75km, please test. Should we keep it like this, adjust the distance, or remove it?

I think it's a good enough solution, you can go with this idea (just need to move the 75km into the config and explain what it is)
Thanks!

@vladyslav-tk vladyslav-tk force-pushed the feat-BGDIINF_SB-2980_add_3d_geoJson_component branch from de5b0f4 to f41d033 Compare August 16, 2023 14:31
found some interesting discussion on the Cesium forum, with some solutions for Billboard and Label placement on top of terrain to avoid clipping.
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

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

🚀
I found some interesting discussion on the Cesium forum, with some solutions regarding terrain clipping with Billboard and Labels, looking good!

@pakb pakb merged commit 6c048a7 into develop Aug 17, 2023
3 checks passed
@pakb pakb deleted the feat-BGDIINF_SB-2980_add_3d_geoJson_component branch August 17, 2023 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants