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

Option to clamp point cloud points to terrain #6714

Open
ggetz opened this issue Jun 21, 2018 · 19 comments
Open

Option to clamp point cloud points to terrain #6714

ggetz opened this issue Jun 21, 2018 · 19 comments

Comments

@ggetz
Copy link
Contributor

ggetz commented Jun 21, 2018

Requested on the forum: https://groups.google.com/forum/#!topic/cesium-dev/Wb4d5ud7nFw

I have a pointcloud that spans the globe but some of the points clip below the surface. I know this would be an edge-case for point cloud support but I would love the ability to have points conditionally work like GroundPrimitive and move to the top of the surface when it is clipped.

Currently my code works as follow: https://pastebin.com/F67gwjc0 By sampling the terrain and then moving the point before load if it is below the surface. The problem with this solution is loading becomes very slow and is not efficient in adjusting points.

Overall I would like the ability to handle points living below the surface. This could be done by adjusting the terrain or the points as long as the hidden points are visible. My other idea would be to test the point against cesiums terrain before generating the tile and adding a meta property that will be added on load.

@hpinkos
Copy link
Contributor

hpinkos commented Jun 21, 2018

If we were to address this use case, I feel like this is something that would happen as part of the conversion to 3D tiles, not runtime in CesiumJS. @lilleyse what do you think?

@ProjectBarks
Copy link
Contributor

ProjectBarks commented Jun 21, 2018

@hpinkos That was what I thought about doing as well. Similar to Cesium.sampleTerrainMostDetailed works but offline. My concern with this solution is if the terrain is modified the points would have to be recomputed.

The other solution I thought of was tying the tile loading process to the pointcloud and when a higher resolution tile is loaded find the intersecting pointcloud tiles from the oct-tree and offset the points positions. My problem with this idea is I feel as if it would be a lot of wasted compute time.

The best solution would be a higher resolution version of approximateTerrainHeights.json that transforms the points at loadtime.

Although I would love to hear @hpinkos and @lilleyse thoughts

@ProjectBarks
Copy link
Contributor

After doing further research I now believe the proper solution would to be a shader level toggle that offsets the point above the terrain at runtime; unless some faster endpoint was exposed to the user.

@lilleyse
Copy link
Contributor

The main problem with the shader approach is terrain heights are not available there. I can't think of a way around that right now.

What you're doing now is probably the best way to do this dynamically but I agree with @hpinkos that it would be easiest to clamp to terrain on the content creation side. This is what we do for 3D buildings which face a similar sort of problem. Your concern is valid though, but maybe not a limiting factor.

One last idea I have is to still set depthTestAgainstTerrain to false, but in the point cloud shader discard points whose depth is far enough from the depth texture value. Kind of like a fuzzy depth test that shows points just under the surface but hides points that are clearly occluded. I'm thinking of your screenshot in CesiumGS/3d-tiles#319. This idea likely has its own edge cases and draw backs though.

@ProjectBarks
Copy link
Contributor

ProjectBarks commented Jun 22, 2018

@lilleyse Would it be possible to see a code snippet for the implementation for fuzzy point testing? The content creation idea works but is way too slow to run at scale. So I think a fuzzy depth tester would be the best of both solutions.

@lilleyse
Copy link
Contributor

lilleyse commented Jun 25, 2018

Honestly the infrastructure isn't really set up for my idea, you would need to make the framebuffer's depth texture available to the point cloud shader. But after that it should be something like this in the fragment shader (with depth test disabled):

float depth = texture2D(u_depthTexture, gl_FragCoord.xy);
float depthEC = -czm_windowToEyeCoordinates(gl_FragCoord.xy, depth).z;
float pointDepthEC = -czm_windowToEyeCoordinates(gl_FragCoord).z;
if (pointDepthEC - depthEC > 10.0)
{
    // Point is 10 meters behind an occluder
    discard;
}

@ProjectBarks
Copy link
Contributor

ProjectBarks commented Jul 2, 2018

@lilleyse is there any middle ground where points aren't rendered on the other side of the earth but are not necessarily depth tested against terrain? I mean hypothetically this could be solved with tiles that are not too large. However, that kinda beats the purpose of tiles in add mode. Generally a way of solving CesiumGS/3d-tiles#319 but without the occlusion caused by depthTestAgainstTerrain.

The way I see this being done is creating a globe of some scalar, then comparing the points against that rather than the terrain itself. I mean the terrain itself would maybe work better since its more true to the world.

@lilleyse
Copy link
Contributor

lilleyse commented Jul 3, 2018

@ProjectBarks - the points on the opposite side should be occluded, but I think there is a bug in the command execution. It might be an easy fix.

Some background - when depthTestAgainstTerrain is false Cesium renders a depth plane across the globe so objects on the opposite side are still able to fail the depth test. But due to the command execution order, 3D Tiles commands are rendered before the depth plane so they show through.

@bagnell do you know if it would be possible to render the depth plane before 3D Tiles and not interfere with inverse classification etc? This may have come up before... I forget.

@bagnell
Copy link
Contributor

bagnell commented Jul 3, 2018

We execute 3D Tiles before the depth is cleared and the depth plane is rendered. This is so they can depth test against the terrain which is useful for 3D Tiles that are terrain or buildings. You could add an option to do it before 3D Tiles are rendered. The invert classification should be fine since it only applies to a single tileset.

@ProjectBarks
Copy link
Contributor

@bagnell any thoughts on a good place to stick this property?

@lilleyse
Copy link
Contributor

lilleyse commented Jul 3, 2018

@bagnell pointed me to 1fda8c0. It's possible that if we revert that commit it will fix the occlusion problem and not interfere with classification volumes or other areas.

@ProjectBarks
Copy link
Contributor

ProjectBarks commented Jul 3, 2018

@lilleyse this works until

contextOptions: {
    requestWebgl2: true
 }

is added. (Note this is on the 1.46 release -- but this also holds true in the 1.47 release).

Demos
WebGL 2 vs WebGL and the corresponding branch with the rollback.

@lilleyse
Copy link
Contributor

lilleyse commented Jul 5, 2018

@lilleyse this works until

Interesting... DepthPlane and upsampled terrain both set DISABLE_GL_POSITION_LOG_DEPTH.

So maybe this bug and #6756 are related.

@lilleyse
Copy link
Contributor

lilleyse commented Jul 5, 2018

@ProjectBarks do you want to open a PR with the rollback? Just make sure to revert all the changes that were made in 1fda8c0, including in DepthPlane.js.

After reading through the dicussion in #5770 I think the rollback is safe. This issue is basically the "edge case" that I didn't think would happen.

@ProjectBarks
Copy link
Contributor

ProjectBarks commented Jul 5, 2018

@lilleyse I think the change to DepthPlane.js is fine right? That's just a code change to shallow clone the radii. Once we clarify that I will submit the PR.

Also for reference, the way around the render bug when using webgl 2 is the following:

viewer.scene.globe.depthTestAgainstTerrain = false;
viewer.scene.logarithmicDepthBuffer = false;

@lilleyse
Copy link
Contributor

lilleyse commented Jul 5, 2018

The previous behavior before 1fda8c0 pushed the depth plane out based on the ground primitive distance, which is important to add back if the order in Scene changes.

Basically just make sure the PR reverses everything in 1fda8c0.

@ProjectBarks
Copy link
Contributor

#6779

@ggetz
Copy link
Contributor Author

ggetz commented Sep 26, 2023

Also requested in #11544.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants