-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add clipping planes to ModelExperimental
#10250
Conversation
Thanks for the pull request @j9liu!
Reviewers, don't forget to make sure that:
|
@j9liu in regards to that tileset that's crashing, it's due to the BIM model being a glTF 1.0 asset which is not currently supported. Try This modified Sandcastle -- everything else is running fine. However, I notice one thing that seems off. If you choose the Point Cloud example, the clipping planes widget's translation seems to move much faster than the mouse. Does this have to do with bounding spheres being too large? or something else? (#10244) |
@ptrgags it seems that the plane's translation is faster than the most whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@j9liu had a few comments about the code, but the examples seem to be working correctly.
Source/Shaders/ModelExperimental/ModelClippingPlanesStageFS.glsl
Outdated
Show resolved
Hide resolved
const clippingPlanes = options.clippingPlanes; | ||
if (defined(clippingPlanes) && clippingPlanes.owner === undefined) { | ||
ClippingPlaneCollection.setOwner(clippingPlanes, this, "_clippingPlanes"); | ||
} else { | ||
this._clippingPlanes = clippingPlanes; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@j9liu, @lilleyse and I have been discussing this offline, not sure the best way we want to handle this. It would be nice to make clipping planes shareable, but that might require putting more responsibility on the user since it's hard to tell when to destroy the GPU resources.
See also #6599 (comment).
Not sure if this discussion has to hold up this PR (any changes could come later). just wanted to write it down somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided that it would be better to handle this with some sort of reference count mechanism. I'll write up the discussion in a separate issue and link it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yesterday afternoon I opened #10257 for this.
Source/Shaders/ModelExperimental/ModelClippingPlanesStageFS.glsl
Outdated
Show resolved
Hide resolved
Specs/Scene/ModelExperimental/ModelClippingPlanesPipelineStageSpec.js
Outdated
Show resolved
Hide resolved
@j9liu everything looks good, and #10257 will be a separate PR so it doesn't need to hold this one up. The Mount St. Helens example looks a lot better now!, it's easier to see the difference:
I checked that the Thanks @j9liu, I'm merging! |
…lanes Add clipping planes to `ModelExperimental`
Closes #10100.
This PR implements clipping planes in
ModelExperimental
, following how they are implemented inModel.js
. Some sandcastles to test:ModelExperimental
is second option in the dropdown menu)enableModelExperimental = true
Unit tests were added accordingly -- I realized I forgot to add a
ImageBasedLightingPipelineStageSpec
in my IBL PR, so it's included in these changes.EDIT: also fixes #8234