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

Allow users to define clipping regions with polygons #11750

Merged
merged 31 commits into from
Apr 30, 2024
Merged

Allow users to define clipping regions with polygons #11750

merged 31 commits into from
Apr 30, 2024

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Jan 9, 2024

Description

The PR implements a new API for clipping that allows user to define a collection of polygonal areas rather than defining individual clipping planes.

This allows:

  • An easier-to-use API for clipping regions more inline with how clipping is implemented in Cesium for Unreal
    • Users now specify polygon positions rather than planes with transforms
    • In particular clipping for global tilesets such as Google's Photogrammetric 3D Tiles becomes much simpler
  • Users can now define multiple regions instead of just one
  • Users can now define concave regions
  • Users can toggle an inverse property to isolate regions instead of clipping them

Out of scope for this PR:

  • Holes: It'd be possible to integrate holes into the existing implementation. In particular the inside/outside and distance algorithm would support holes as-is. The sticking point is mainly scope – Our existing PolygonHierarchy (what would really should use to keep the API cohesive) definition supports an unlimited recursive list of holes, and how we pack that into the texture used for the compute command would need some work to support holes. It's also totally feasible to add a hierarchy option at a later date. Given all that, I would suggest keeping it out of the scope of this PR and see if the feature is requested.
  • Shadows: Currently, clipping polygons are not taken into account for shadows. They are not implemented for clipping planes either, so it may make sense to tackle this together outside of this PR.

image

With building inset Without building inset
image image

Sandcastle Example

Concave clipping region Inverted clipping regions
image image

Sandcastle Example

This is implemented very similar to clipping planes, where we:

  • Add a new ClippingPolygon, defined by a list of three or more positions, and ClippingPolygonCollection for managing the list and state
  • This collection can be applied one or more tilesets, or a model
  • Use intersections with 3D tiles bounding volumes to determine if a tile is being clipped or not
  • Pack the details of the clipping area into a texture
  • Add a new model pipeline stage and modifications to GlobeFS which add lines the fragment shader to discard the area within the polygon

To avoid doing inside/outside checks for each fragment, we instead use a ComputeCommand to generate a texture containing the signed distance from the polygons' edges, and the fragment shader instead samples the results from this pre-computed texture. Additionally, the spherical approximation method is borrowed from our implementation of texture mapping for groundPrimitives, and should be enough precision for most cases.

Issue number and link

Fixes #8751

Testing plan

For both 3D Tiles and the globe (this sandcastle allows you to toggle between the two):

  1. Verify multiple polygons can be clipped at once.
  2. Verify polygons far away clip.
  3. Verify polygons intersecting one another clip.
  4. Verify inverse clipping for multiple polygons.
  5. Verify concave polygons clip correctly.
  6. Verify concave polygons inverse clip correctly.
  7. Verify removing polygons from a collection.
  8. Verify performance (FPS) is comparable for a polygon with many positions, using:
    viewer.scene.debugShowFramesPerSecond = true;
  9. Verify clipping resolution when zoomed in

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code
  • Performance testing
  • Exclude clipped areas from shadows
  • Testing plan
  • Polished Sandcastle
  • Jagged edges on large polygons
  • Move AEC tileset into the CesiumJS ion account
  • Squash commits

@tbenazzi
Copy link

tbenazzi commented Feb 9, 2024

Hi @ggetz
Massive thank you for this PR. Your work represents a significant step forward in improving asset integration capabilities, and we are incredibly excited about the potential it unlocks for us and community.

Your implementation could greatly enhance the way users interact with external 3D integration into 3Dtileset. This is why your contribution is viewed as a key feature for us and the community.

We found a glitch with shadows where the hidden part always casts shadows, which is quite annoying when we want to do a shadow analysis of an integrated CAD or BIM model.

Looking forward to your thoughts and any further actions you deem necessary to bring this PR to completion. Let's make this amazing feature available to everyone in the CesiumJS community!

@Masty88
Copy link

Masty88 commented Feb 14, 2024

Very interesting tool! @cesiumgs-admin do you know when will be avalaible?

@ggetz
Copy link
Contributor Author

ggetz commented Feb 14, 2024

Thanks @tbenazzi and @Masty88 for the interest!

We are reviewing some performance impact of this feature currently, and I'm aiming to get it into the March 1 release.

We found a glitch with shadows where the hidden part always casts shadows, which is quite annoying when we want to do a shadow analysis of an integrated CAD or BIM model.

This is a good point. We will evaluate whether its possible to include in this iteration. If not, we will certainly open a feature request for it. Thanks for the idea!

@saadatali48
Copy link

Hi,
Any idea when this feature will get released?

@ggetz
Copy link
Contributor Author

ggetz commented Mar 12, 2024

@saadatali48 Sorry for the delay. This PR is next on my list, and ideally will go out with the next release. Thanks for the interest!

@ggetz
Copy link
Contributor Author

ggetz commented Mar 25, 2024

@jjhembd This should be ready for a first pass. Please note the remaining TODO's include finishing up the unit tests and jagged edges on large polygons which corresponds with the one TODO comment in the code.

Copy link
Contributor

@jjhembd jjhembd left a comment

Choose a reason for hiding this comment

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

Hi @ggetz, I'm still reviewing, but here's a few minor comments / questions from what I've read so far.

packages/engine/Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
packages/engine/Source/Core/Rectangle.js Outdated Show resolved Hide resolved
packages/engine/Specs/Core/RectangleSpec.js Show resolved Hide resolved
Copy link
Contributor

@jjhembd jjhembd left a comment

Choose a reason for hiding this comment

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

A few more comments & questions on the code.

packages/engine/Source/Scene/ClippingPolygonCollection.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/ClippingPolygonCollection.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/ClippingPolygonCollection.js Outdated Show resolved Hide resolved
packages/engine/Specs/Scene/Cesium3DTilesetSpec.js Outdated Show resolved Hide resolved
@jjhembd
Copy link
Contributor

jjhembd commented Apr 11, 2024

I'm getting some strange results with large polygons on terrain.
image
The polygon has just 4 points.

The same polygon on 3D Tiles doesn't seem to clip anything.

@ggetz
Copy link
Contributor Author

ggetz commented Apr 12, 2024

I'm getting some strange results with large polygons on terrain.

@jjhembd Thanks for pointing this out. Was this the only polygon in the scene or were there multiple?

@jjhembd
Copy link
Contributor

jjhembd commented Apr 12, 2024

Was this the only polygon in the scene or were there multiple?

Good question. I can only trigger the problem if there are multiple polygons. Here is the result with only one large polygon (looks fine):
image

And here is the result if I first draw a smaller polygon in the NW corner, then a large one in the center:
image

@ggetz
Copy link
Contributor Author

ggetz commented Apr 15, 2024

I'm getting some strange results with large polygons on terrain.

Thanks for pointing this out @jjhembd. I think this should be resolved now.

The same polygon on 3D Tiles doesn't seem to clip anything.

Something seems to be very different about the behavior of root tiles on Google P3DT. I'm looking into that now.

@ggetz
Copy link
Contributor Author

ggetz commented Apr 17, 2024

Thanks for the thorough review @jjhembd! I believe I've addressed all of you feedback and this is ready for another look.

@jjhembd
Copy link
Contributor

jjhembd commented Apr 22, 2024

Thanks @ggetz, the code is looking good, and the issue with large polygons is fixed.

I have two more smaller comments on the Sandcastles.

AEC Clipping Sandcastle

This one works well, and it's a very interesting demo!
A minor nitpick: the initial camera position is awkward for me. The region of interest is mostly out of view, below the camera.
image

Clipping Regions Sandcastle

The clipping polygon seems to lose resolution when zooming out to a lower LOD in the underlying data. For example, here I clipped out part of Michigan:
image

But when I zoom out, one point in the polygon appears to be lost:
image

Zooming out further leaves only 4 points in the polygon, and the original shape is gone:
image

This aggressive polygon simplification is similar in both Terrain and 3D Tiles.

@ggetz
Copy link
Contributor Author

ggetz commented Apr 23, 2024

Thanks @jjhembd! I believe I've addressed your feedback. You can now see 1) a better view of the AEC clipping region, and 2) Michigan at scale.

@ggetz
Copy link
Contributor Author

ggetz commented Apr 26, 2024

@jjhembd I've made some additional adjustments based on your feedback.

  1. There was a bug with removing then adding polygons with the same number of positions, which was a simple fix in the update function.
  2. To remove artifacts, I increased the amount of padding defining the polygon extents. My concern was this would affect performance, but when measuring I could not detect any effect on the framerate.

Copy link
Contributor

@jjhembd jjhembd left a comment

Choose a reason for hiding this comment

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

Thanks @ggetz, this looks good to me! I am now not able to break it, even with many large and complex polygons.
image

@jjhembd jjhembd merged commit 606574e into main Apr 30, 2024
9 checks passed
@jjhembd jjhembd deleted the clip-region branch April 30, 2024 03:34
@Masty88
Copy link

Masty88 commented May 2, 2024

Super thank you!

@catnuko
Copy link

catnuko commented Jul 11, 2024

Is clippingPolygon supported for VoxelPrimitive?

@ggetz
Copy link
Contributor Author

ggetz commented Jul 11, 2024

@catnuko Not presently, but we can open a feature request for this. Would you mind giving us a few details about your use case?

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

Successfully merging this pull request may close these issues.

Add support for concave /convex clipping planes + holes in CesiumJS
6 participants