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

Intersection option for WebGLRenderer clipping method #9470

Merged
merged 10 commits into from Oct 14, 2016
Merged

Conversation

PWhiddy
Copy link
Contributor

@PWhiddy PWhiddy commented Aug 8, 2016

Currently custom clipping planes remove anything in the union of the planes' clip space, however it is sometimes desirable to clip off only the intersection of their clipping.
This adds an option for WebGLRenderer's global clipping to use this method
Here are screenshots showing clipping

The regular way

geometry 11

And with renderer.clipIntersection = true;

geometry 10

Not sure if @tschw has more plans for clipping in threejs, but it would also be nice if this option could be available at the local (material) clipping level

@mrdoob
Copy link
Owner

mrdoob commented Aug 8, 2016

@tschw what do you think?

@Wilt
Copy link
Contributor

Wilt commented Aug 9, 2016

I like this idea.

@tschw
Copy link
Contributor

tschw commented Aug 9, 2016

Nice! And the implementation looks good too.

Just as a thought, could allow the setting on a per-material basis at some later point...

@WestLangley
Copy link
Collaborator

An example of this enhancement would be great, too ! : - )

@mrdoob
Copy link
Owner

mrdoob commented Aug 9, 2016

An example of this enhancement would be great, too ! : - )

Yes please!

@PWhiddy
Copy link
Contributor Author

PWhiddy commented Aug 12, 2016

@mrdoob @tschw
Just added an example and added a line in the docs.
As tschw said, it would be nice eventually to have this available on a per-material basis, but I am not sure how best to implement this. For now local clipping does not work when the setting is enabled.

@PWhiddy
Copy link
Contributor Author

PWhiddy commented Aug 16, 2016

@mrdoob Does it look good now?

@Wilt
Copy link
Contributor

Wilt commented Aug 16, 2016

I like the example that you created 👍
The checkbox doesn't do anything though...

@tschw
Copy link
Contributor

tschw commented Aug 17, 2016

@PWhiddy

Very nice!

As tschw said, it would be nice eventually to have this available on a per-material basis, but I am not sure how best to implement this. For now local clipping does not work when the setting is enabled.

It's the same code at shader level. Both data paths already exist in the renderer, so should be fairly straightforward after reading into it. Just come back with specific questions, in case you want to give it a go and run into problems.

For now local clipping does not work when the setting is enabled.

Odd... Why? I figure it would just combine clipping differently in that case!?

@PWhiddy
Copy link
Contributor Author

PWhiddy commented Aug 18, 2016

I like the example that you created 👍
The checkbox doesn't do anything though...

@Wilt The example doesn't work at that link because the threejs build is not updated in my PR (the docs say not to do this).

It will look like this with built library

screenshot 5

As for the local clipping, I think the reason that the global always overrides the local may be because in WebGLClipping I always set both clipIntersection and scope.clipIntersection with the renderers global setting.

I was not sure if it made more sense to add an extra clipIntersection parameter to all materials or if in the long term all materials should have just one property with an object that would contain planes (or different geometries), and other options bundled together.

Something maybe like:

material.clipping = { planes: [ p1, p2 ], boxes: [ b1 ], respectGlobal: false, clipIntersecton: true };

I wasn't sure if this was the direction that clipping in threejs was headed, so I went with the simple version that maintains current functionality, but is more limited with the new feature for now.

@tschw
Copy link
Contributor

tschw commented Aug 18, 2016

For the global vs. local part, the intended use is global clipping for viewport restriction (e.g. portal culling) and local clipping for modeling purposes. That's why there's no shadow clipping for global planes (what is not visible can still cast a shadow) and no automatic object culling for local planes.

I guess it's fine to just have a global union vs. intersection switch, but it would be preferable to specify the planes on the materials in the examples to match the design rationale. Ideally, though we'd have to touch the GLSL for that, intersection mode would only apply to local planes.

@PWhiddy
Copy link
Contributor Author

PWhiddy commented Oct 7, 2016

@tschw @mrdoob
I finally was able to make the clipping option available locally on materials rather than globally!
Also updated the example and the API docs.
Tested the advanced clipping example and everything seems to be working.
Feel free to check for for yourselves, but should be ready to merge!

@tschw
Copy link
Contributor

tschw commented Oct 7, 2016

Interesting...

I find the interplay of global and local planes with this implementation somewhat questionable: The material setting now dictates how global clipping planes are to be interpreted. Does this make any sense? I fail to find a use case for these semantics - not even a far-fetched one, which does not necessarily mean there is none.
Since this behavior is also undocumented, I get the feeling it's random weirdness, driven by the structure of the existing implementation. Then however, making things even weirder, the local setting is considered during FOV culling, at this point only for global planes, which is correct - too correct for being undefined / accidental behavior or an intentional limitation. So I'm left a little puzzled. Please correct me in case I'm missing something.

If there is a union vs. intersection switch on a per-material basis, I think it shouldn't affect the global planes and the global union vs. intersection setting. Basically what this means is that the vertex shader would have to have two independent loops checking for clipping - one for the global planes and one for the local planes. The vec4 array uniform can be shared, since global planes always come first, however, both code blocks would have to be conditional, switched off based on the presence of respective global or local planes.

The more limited alternative (implementation-wise simpler than this code, so I'm kind-of sorry to say) would be a global union vs. intersection switch that just applies to all the planes.

A global switch only applying to the local planes would even be nicer, in this case, changes to FOV culling (a likely hotspot for complex scenes) could be left out (since it only considers global planes) but again aforementioned vertex shader modifications would be required.

All these three variants would make more sense to me than what we see implemented here. The second one being the weakest, but easiest to implement from where we're at. I haven't quite decided on whether I like the first or the third better: The first is very complete but needs an extra material property, which BTW also needs serialization support - missing here, the third OTOH is more limited but should suffice to capture the most typical use cases reasonably well.

@PWhiddy
Copy link
Contributor Author

PWhiddy commented Oct 8, 2016

Thanks for your detailed response! I did not intentionally mean for the per-material clipping mode to affect the global planes, although now I think I see your point that since local and global are currently combined before being given to the shader they would need to be separated.

I think the second method you described was basically my first implementation from August (which doesn't appear to be visible here because my recent pushes must have overridden it)
I could revert to this if it's acceptable for the library, or perhaps if you gave me a couple pointers (maybe describe the changes that would be needed) I could try to implement the first or third option (would just vertex need modification or fragment too?).
As to which variant would be most practical, if you decide which you like best I'm willing to take a shot at it.
Also good catch on the serialization, I can update that as well.

@tschw
Copy link
Contributor

tschw commented Oct 8, 2016

would just vertex need modification or fragment too?

Confused me: In fact, it's only in the fragment shader. Would be, if WebGL had clipping support like desktop GL has.

if you gave me a couple pointers (maybe describe the changes that would be needed) I could try to implement the first or third option (would just vertex need modification or fragment too?

OK, after thinking about this a bit, it could actually end up a lot easier than it sounds described in my previous post:

I think it makes sense to have global clipping restricted to the union mode. Then there is no need to touch FOV culling (a likely hotspot) and also simplifies the implementation as described in the next paragraph.

Local clipping may either clip the union or the intersection of the planes, so shader-wise it comes down to just adding code that does the intersection part - without having it affect union clipping. Since the global planes are always the first ones in the array that goes into the uniform and aforementioned restriction of global planes to union mode, we can use a single define to control it all (just like in your previous implementation, trading the flag for an integer):

// DISCLAIMER: For illustration - keep the details like in the existing code.
#if NUM_CLIP_PLANES_UNION
for ( int i = 0; i < NUM_CLIP_PLANES_UNION; ++i )
    if ( dot( clipPlanes[ i ], coord ) < 0.0 ) discard;
#endif

#if NUM_CLIP_PLANES_UNION < NUM_CLIP_PLANES_TOTAL
for ( int i = NUM_CLIP_PLANES_UNION; i < NUM_CLIP_PLANES_TOTAL; ++i ) {
    if ( dot( clipPlanes[ i ], coord ) < 0.0 ) break;
    if ( i == NUM_CLIP_PLANES_TOTAL - 1 ) discard;
}
#endif

Now for clipping the union of all local planes (like now), NUM_CLIP_PLANES_UNION = NUM_CLIP_PLANES_TOTAL.

For clipping the intersection of all local planes, NUM_CLIP_PLANES_UNION would be the number of global clipping planes.

These values are around in the renderer code paths for updating - and I guess it should be fairly easy to get it in. Transform to view space would be in place without having to change anything. Only thing left to decide is whether to control the local mode globally or on a per-material basis.

@PWhiddy
Copy link
Contributor Author

PWhiddy commented Oct 13, 2016

I like your idea, it is a very good solution!
Here is a new implementation, following this idea. Global clipping always uses union mode, and the clipIntersection material property applies only to local clipping.
This works well because the shader clipping loop is essentially unchanged when intersection is not enabled, and when it is the clipping loop is simply split into two parts and they share the same uniform array.

How does it look?

@PWhiddy
Copy link
Contributor Author

PWhiddy commented Oct 13, 2016

Also, you previously mentioned serialization support for the new material property. I added the clipIntersection property to the "copy" function, but does it also need to be added to toJSON? It seems quite a few properties are missing there

@mrdoob
Copy link
Owner

mrdoob commented Oct 14, 2016

@tschw looks good?

Copy link
Contributor

@tschw tschw left a comment

Choose a reason for hiding this comment

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

Looks good.

@@ -151,6 +154,7 @@ function WebGLClipping() {
}

scope.numPlanes = nPlanes;
// Add clipIntersection
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't get this comment. Leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was leftover. Just removed it

@tschw
Copy link
Contributor

tschw commented Oct 14, 2016

does it also need to be added to toJSON

I just double-checked. I though the planes were in the JSON already, but it seems they are currently not serialized, so I guess we also don't have to serialize the flag.

The reason for that, and also why I've been so insistent on making both modes able to coexist, is that we may want to lift the planes off the material to become nodes in the scene graph that clip their children at some point.

@PWhiddy
Copy link
Contributor Author

PWhiddy commented Oct 14, 2016

Okay, that makes sense.

Thank you for all the guidance tschw, it was very helpful and wouldn't have been possible otherwise!

@mrdoob mrdoob merged commit 449d18a into mrdoob:dev Oct 14, 2016
@mrdoob
Copy link
Owner

mrdoob commented Oct 14, 2016

Thanks guys!

@tschw tschw mentioned this pull request Oct 18, 2016
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.

None yet

5 participants