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

OutlinePass: correct gaussian probability density function input #24262

Merged
merged 1 commit into from
Jun 28, 2022
Merged

OutlinePass: correct gaussian probability density function input #24262

merged 1 commit into from
Jun 28, 2022

Conversation

ingun37
Copy link
Contributor

@ingun37 ingun37 commented Jun 22, 2022

OutlinePass' blur material uses too small step when they calculate weighted sum. As a consequence it becomes practically a box filter. As you can see when you thicken the outline, the pointy parts' outline resembles a box.
before

I think they meant to step discretely( e.i by 1, instead of like 1/1024 ) so I corrected it. Now you can see the pointy parts resembles a circle (the shape of gaussian kernel).
step-sigma

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 22, 2022

@spidersharma03 What do you think about this change?

@ingun37
Copy link
Contributor Author

ingun37 commented Jun 22, 2022

Especially this part becomes really pretty
from
before 2
to
step-sigma 2

@ingun37
Copy link
Contributor Author

ingun37 commented Jun 22, 2022

Let me elaborate a bit more
This is the gaussian function they use when kernelRadius = 1.
Screen Shot 2022-06-23 at 1 23 49 AM
The graph by itself looks like a gaussian kernel of radius of (at least) 3. But technically can be used as kernel of radius of 1. Whichever radius you choose, the sampling step (or sampling rate) has to be reasonably in the range of 0.5 ~ 1. (decided by MAXIMUM_RADIUS, the sample size)
But the step the code takes is in the unit of 1/texSize. Although the concept is correct, e.i 1px, the magnitude is way too small. The situation suggests that the original intention was to take discrete step not proportional.

@WestLangley
Copy link
Collaborator

@bhouston Do you have the reference for the OutlinePass implementation?

@bhouston
Copy link
Contributor

I think this was just never done properly. It seems like this was just skipped over as not a major concern in the original PR.

Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

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

When comparing this version with prod, the new style looks indeed cleaner. Especially when setting edgeStrength and edgeThickness to high values.

@Mugen87 Mugen87 added this to the r142 milestone Jun 23, 2022
@mrdoob mrdoob merged commit 4e75e85 into mrdoob:dev Jun 28, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jun 28, 2022

Thanks!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
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.

5 participants