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

Ss shadow pass pr selection #2

Draft
wants to merge 2 commits into
base: SSShadowPassPr
Choose a base branch
from

Conversation

lyonsno
Copy link

@lyonsno lyonsno commented Apr 26, 2022

Add selective option

The new 'selects' parameter should be an array of objects to which the effect will be applied. If an array is passed in the effect will only apply to objects contained in the array.

@lyonsno lyonsno marked this pull request as draft April 26, 2022 23:37
@lyonsno
Copy link
Author

lyonsno commented Apr 26, 2022

@gonnavis I'm thinking of adding a GUI element to the example demonstrating this feature, what do you think?

@gonnavis
Copy link
Owner

gonnavis commented Apr 27, 2022

Add selective option

The new 'selects' parameter should be an array of objects to which the effect will be applied. If an array is passed in the effect will only apply to objects contained in the array.

Thanks! Works very well.

PS: test link: https://raw.githack.com/lyonsno/three.js/SSShadowPassPr-selection/examples/webgl_postprocessing_ssshadow.html

@gonnavis
Copy link
Owner

@gonnavis I'm thinking of adding a GUI element to the example demonstrating this feature, what do you think?

What GUI element do you mean?
Not this?
image

@lyonsno
Copy link
Author

lyonsno commented Apr 28, 2022

Thanks! Works very well.

Thank you! The way it works currently, non-selected objects will still cast, but not receive the effect. I hope this is proper behavior because removing them as casters would be trickier.

PS: test link: https://raw.githack.com/lyonsno/three.js/SSShadowPassPr->selection/examples/webgl_postprocessing_ssshadow.html

I should include this in PRs right? How should I modify the example to demonstrate an effect like this? This kind of brings me to the question below:

@gonnavis I'm thinking of adding a GUI element to the example demonstrating this feature, what do you think?

What GUI element do you mean? Not this? image

I meant a toggle to turn the effect on/off for certain elements to demonstrate. Not sure if this feature is important enough that that's necessary though. The SSR example doesn't have a GUI for it I don't think.

--

Next I'm actually pumped to start thinking about refraction, it's a tricky problem. Have you done any brainstorming on it?

@gonnavis
Copy link
Owner

gonnavis commented Apr 28, 2022

PS: test link: https://raw.githack.com/lyonsno/three.js/SSShadowPassPr-selection/examples/webgl_postprocessing_ssshadow.html

I should include this in PRs right? How should I modify the example to demonstrate an effect like this?

It's a link generated by raw.git ( or brower extension ), just for easy testing.
You can keep committing to your SSShadowPassPr-selection branch and it'll auto update after about 20 minutes.

I meant a toggle to turn the effect on/off for certain elements to demonstrate. Not sure if this feature is important enough that that's necessary though. The SSR example doesn't have a GUI for it I don't think.

I think you can either put the toggle in GUI like this https://raw.githack.com/gonnavis/three.js/SSRPassSupportDifferentMetalnessPr/examples/webgl_postprocessing_ssr.html
image
Or by click the objects directly like this https://raw.githack.com/gonnavis/three.js/SSRrPassPr/examples/webgl_postprocessing_ssrr.html
image

Next I'm actually pumped to start thinking about refraction, it's a tricky problem. Have you done any brainstorming on it?

I've already did it mrdoob#21420
But currently prefer to use transmission mrdoob#23569

@lyonsno
Copy link
Author

lyonsno commented Apr 29, 2022

I've already did it mrdoob#21420
But currently prefer to use transmission mrdoob#23569

Ah I see. What about meshBasicMaterial objects like the ones used in the example? When their opacity is set to < 1, right now the SSShadows aren't visible through them. Do you think that should be fixed? It might not be worth the extra pass it would probably take, I'm not sure.

@gonnavis
Copy link
Owner

gonnavis commented Apr 29, 2022

When their opacity is set to < 1, right now the SSShadows aren't visible through them. Do you think that should be fixed? It might not be worth the extra pass it would probably take, I'm not sure.

That's the hard part. When doing SSRrPass, I faced the same problem.

I render BeautyPass first, then SSRrPass which based on RefractivePass. Thus I can make such as box and bunny be transparent ( opacity < 1 ).
I've already added more Passes in the above process, but still can't see the bunny through the box.
If want to achieve this effect, I feel must add extra pass to each transparent object separately.

image

image

image

image

@gonnavis
Copy link
Owner

For SSShadow, maybe ignore transparent/opacity, just do opaque first.

@lyonsno
Copy link
Author

lyonsno commented May 10, 2022

For SSShadow, maybe ignore transparent/opacity, just do opaque first.

Sorry I had to drop this for a bit. In that case it should be pretty much ready to merge after I add the feature to the example GUI right?

@gonnavis
Copy link
Owner

Yes, I tested that the selects works fine and very happy to merge, thanks!

But by the way, I don't know if you intentionally submit PR to my fork repo https://github.com/gonnavis/three.js , instead of Mr.Doob's official repo https://github.com/mrdoob/three.js.
So even I merged, your codes will just go into the commits list of my PR mrdoob#21951 , and we'll still need to go through strictly checking and need to handle reviews if any.

@lyonsno
Copy link
Author

lyonsno commented Jul 15, 2022 via email

@gonnavis
Copy link
Owner

@lyonsno You can make a fresh new PR to https://github.com/mrdoob/three.js directly.

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.

2 participants