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

Raycaster: Add Interpolated Normals #25566

Merged
merged 6 commits into from
Mar 8, 2023
Merged

Conversation

zalo
Copy link
Contributor

@zalo zalo commented Feb 26, 2023

This PR exposes barycentrically interpolated/smoothed normal vectors in raycast intersection objects.

These normals are useful to have on hand when calculating accurate reflections in optical systems; helping to mitigate "the disco ball effect" that can come from flat facets.

You can see in this example scene I set up (attempting to mimic Paul Bourke's Mirror Dome):
https://raw.githack.com/zalo/three.js/feat-curved-reprojection-mirrordome/examples/webgl_materials_reprojection.html

Enable and Disable the Use Interpolated Normals checkbox to see the difference between normals from flat faces and normals from interpolating the normals attribute.

@LeviPesin
Copy link
Contributor

LeviPesin commented Feb 26, 2023

Seems like this breaks (or changes the screenshot) webgl_raycaster_bvh and webgl_geometry_csg. I think they are using the Triangle.getUV method.

@zalo
Copy link
Contributor Author

zalo commented Feb 26, 2023

Ahhh, perhaps the library they import from unpkg calls it. I've restored Triangle.getUV() as an alias for Triangle.getInterpolation().

src/math/Triangle.js Outdated Show resolved Hide resolved
src/math/Triangle.js Fixed Show resolved Hide resolved
zalo and others added 2 commits February 25, 2023 20:10
Co-authored-by: Levi Pesin <35454228+LeviPesin@users.noreply.github.com>
@zalo
Copy link
Contributor Author

zalo commented Feb 26, 2023

It's weird, that last commit required the Action to be run twice to pass the E2E test on Windows...
I guess the placement of the cubes in the Oimo demo is non-deterministic somehow 🤔

Pictures of the Sporadic End to End Test

physics_oimo_instancing-actual
physics_oimo_instancing-expected
physics_oimo_instancing-diff

Either way, seems like all the E2E tests are set with the changes.

@LeviPesin
Copy link
Contributor

Maybe not always all cubes there are able to appear in time... So maybe we should increase idleTime.

src/math/Triangle.js Outdated Show resolved Hide resolved
@mrdoob mrdoob added this to the r151 milestone Mar 1, 2023
@Mugen87 Mugen87 merged commit efb6281 into mrdoob:dev Mar 8, 2023
@zalo zalo deleted the feat-raycast-normals branch March 8, 2023 15:20
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 8, 2023

@gkjohnson This change is relevant for three-bvh-csg since webgl_geometry_csg now spams the console with deprecation warnings.

https://rawcdn.githack.com/mrdoob/three.js/dev/examples/webgl_geometry_csg.html

@mrdoob
Copy link
Owner

mrdoob commented Mar 9, 2023

@Mugen87 Should we add a temp bar to only log it once?

@prideout
Copy link
Contributor

prideout commented Mar 9, 2023

Niiiice! Technically this should be a slerp, but that would be crazy expensive. I wonder if it should at least do a normalize after the interpolate. I think they call that a "nlerp".

@zalo
Copy link
Contributor Author

zalo commented Mar 9, 2023

Niiiice! Technically this should be a slerp, but that would be crazy expensive. I wonder if it should at least do a normalize after the interpolate. I think they call that a "nlerp".

I considered it, but then I realized that models can technically have all kinds of crazy non-unit normals in some cases...

I guess the debate is between the number of people who will stub their toes on non-unit normals vs. the number of people who will have to edit the engine to get them if they need them...

(Also, I don't believe Slerp works on more than 2 components... but I suppose I could use the torque average from one of my favorite papers... https://matthias-research.github.io/pages/publications/stablePolarDecomp.pdf )

@prideout
Copy link
Contributor

prideout commented Mar 9, 2023

What kinds of models have non-unit normals?

@zalo
Copy link
Contributor Author

zalo commented Mar 9, 2023

What kinds of models have non-unit normals?

I believe some anime/toon shaded models warp/distort normals to achieve specific shading contours...

Not sure who else ¯\_(ツ)_/¯ ; it's been a while since I've interacted with any technical artists...

It might be worth the headache for them to normalize; feel free to open a PR with it xD

@mrdoob
Copy link
Owner

mrdoob commented Mar 9, 2023

I think it'll be better to not normalize until we get enough people bumping into that.

@gkjohnson
Copy link
Collaborator

I think it'll be better to not normalize until we get enough people bumping into that.

It should probably be explicitly called out in the docs, then, that the "normal" vector will likely not be normalized even if all the vertex normals are since barycentrically interpolating three normalized vectors does not guarantee a normalized result.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 9, 2023

@Mugen87 Should we add a temp bar to only log it once?

Yes, that makes sense. I'll file a PR.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 9, 2023

Since a normalization is cheap, I would add it to the code so we have a mathematical correct result.

This pull request was closed.
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.

6 participants