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

Examples: Add ground projected env map #24311

Merged
merged 19 commits into from
Jul 19, 2022

Conversation

FarazzShaikh
Copy link
Contributor

@FarazzShaikh FarazzShaikh commented Jul 4, 2022

Fixed #23512

Description

Adds ground projected Env map helper to examples/jsm/objects/GroundProjectedEnv.js. Impl based on @react-three/drei's. Samples an env map based on ray casting result from camera to a SDF sphere (for background) and disc (for the ground).

Example use:

import { GroundProjectedEnv } from "three/jsm/objects/GroundProjectedEnv";
...
const env = new GroundProjectedEnv(envMap);
env.scale.setScalar(100);
scene.add(env);
...
env.radius = ...;
env.height = ...;

Example added to examples/webgl_materials_envmaps_ground-projected.html
image

Copy link
Collaborator

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

This looks great! Here's a live link to the demo:

http://raw.githack.com/FarazzShaikh/three.js/feat/envmaps-ground-projected/examples/index.html?q=ground#webgl_materials_envmaps_ground-projected

Two nits:

  • Can we remove the build files and package-lock changes from the PR?
  • It would be nice to use the project code style for the js and html files but as far as I understand that is not blocking for examples.

@FarazzShaikh
Copy link
Contributor Author

Thanks Garrett, Of course I can make those changes!

@Mugen87 Mugen87 added this to the r143 milestone Jul 5, 2022
@LeviPesin
Copy link
Contributor

LeviPesin commented Jul 5, 2022

Please fix broken lint (in files.json, new class, and the example).

@FarazzShaikh
Copy link
Contributor Author

FarazzShaikh commented Jul 5, 2022

ran npm run lint-fix on examples/jsm/misc/GroundProjectedEnv.js no lint problems were reported by the lint command on other related files

@WestLangley
Copy link
Collaborator

Suggestions:

  1. How about a car, instead?

  2. And set reasonable limits for the GUI parameters.

  3. And limit the controls to prevent it from flying.

Screen Shot 2022-07-05 at 8 32 18 AM

@FarazzShaikh
Copy link
Contributor Author

Of course, I can do those

@LeviPesin
Copy link
Contributor

no lint problems were reported by the lint command on other related files

There are problems remaining in files.json (just revert the changes to it and add example), GLSL code in the class (like missing spaces, you can fix that manually), and in the example (also just missing spaces).

@FarazzShaikh
Copy link
Contributor Author

FarazzShaikh commented Jul 6, 2022

  • Using car instead of the helmet now.
  • Limited the controls and GUI sliders.
  • Fixed GLSL formatting in the class

image

@@ -141,6 +141,7 @@
"webgl_materials_displacementmap",
"webgl_materials_envmaps",
"webgl_materials_envmaps_exr",
"webgl_materials_envmaps_ground-projected",
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to replace - in the example name with _.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that would make the sidebar section title materials / envmaps / ground / projected and not materials / envmaps / ground-projected

@LeviPesin
Copy link
Contributor

LeviPesin commented Jul 6, 2022

Lint is again broken in the class (still including GLSL) and the example.

@FarazzShaikh
Copy link
Contributor Author

No worries, will revert rotation feature. My bad i forgot the reflections wouldn't rotate even if the env did. Perhaps we can add it back later when env map rotation is supported in core.

@Mugen87 Mugen87 merged commit da167b1 into mrdoob:dev Jul 19, 2022
@Mugen87 Mugen87 mentioned this pull request Jul 19, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jul 20, 2022

Thanks!

@joshuaellis joshuaellis mentioned this pull request Jul 28, 2022
4 tasks
@ctliz
Copy link

ctliz commented Aug 19, 2022

image

In my project
const HDR = useLoader(THREE.TextureLoader, './bg_1.jpeg');
HDR.encoding = THREE.sRGBEncoding;
var env = new GroundProjectedEnv( HDR );
env.scale.setScalar( 100 )
three.scene.add( env );

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 19, 2022

@blokingsmall Please use the forum first. If it turns out to be an issue with the implementation, this should be reported as a new GitHub issue.

@ctliz
Copy link

ctliz commented Aug 19, 2022

@Mugen87 ok i'll try it

@FarazzShaikh
Copy link
Contributor Author

Please tag me in the issue if this is indeed a bug in the implementation

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 19, 2022

@WestLangley
Copy link
Collaborator

/ping @FarazzShaikh See #24520. I am assuming the issue needs further investigation.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Aug 22, 2022

According to this comment disabling mipmaps on the env map fixes the issue. It seems react-three-fiber automatically enables mipmaps on textures. But the issue here is that the UVs for sampling are dynamically generated and the derivative of the passed value is used to derive which mipmap to use. And because it's such a stark jump from UV 0.0 to 1.0 at the wrap around seam. This leads to a very high dervaitive value and likely the lowest LoD mipmap being selected. Normally this isn't an issue when mipmaps are disabled or the UV seam is at a triangle boundary. See here:

Wireframe UVs Solid UVs
image image

You can see that the UV seam occurs mid triangle.

And if you take a look at the derivatives per-pixel you can see the seam in the same spot:

My recommendation here is to call this "not a bug" and require users to pass in a texture with mipmaps disabled or generate a geometry such that this UV seam is always on a triangle edge.

Also if you're a user submitting a bug report please make sure it's using vanilla three.js. Giving repros based on environments like react-three-fiber that adjust defaults values especially when it's not clear what's being used... it makes understanding issues like this so much harder and is why we couldn't repro it in the official example... 😭

@WestLangley
Copy link
Collaborator

or generate a geometry such that this UV seam is always on a triangle edge.

@gkjohnson Are you implying that replacing IcosahedronGeometry with SphereGeometry( 1, 32, 16 ) will fix the problem?

@gkjohnson
Copy link
Collaborator

Are you implying that replacing IcosahedronGeometry with SphereGeometry( 1, 32, 16 ) will fix the problem?

I'm not so familiar with the internals of the grounded env map but I don't believe it's so simple - at least not without some more extensive changes to the the class. Just from the wireframe of the mesh it looks like it serves as more of just a rasterization surface with procedural computations being used for the envmap "ground" projection. If the surface of the mesh is used directly then it's likely you'll see artifacts as the result of the geometry resolution in the visuals of the env map though it might not be all that noticeable. I'll have to let @FarazzShaikh speak on what might be doable here - I've just run into this issue of unexpected procedural mipmap calculations before 😁

@WestLangley
Copy link
Collaborator

or generate a geometry such that this UV seam is always on a triangle edge.

Thing is, the shader does not use the geometry uv's. So I am not sure the alignment of the triangles matters...

@gkjohnson
Copy link
Collaborator

It doesn't currently but it could if the geometry were formed appropriately.

@WestLangley
Copy link
Collaborator

WestLangley commented Aug 22, 2022

It doesn't currently but it could if the geometry were formed appropriately.

I see. So to rephrase your proposed solution:

  1. modify the shader to use the geometry UVs, and
  2. insure the new sphere geometry triangles are aligned with the seam.

If you are correct, and that works, then we may need to revisit the built-in "cube equirect shader" on which this is based. I do not recall seeing the cube equirect shader have the same artifacts, however...

@gkjohnson
Copy link
Collaborator

Yes both of those are required to support the solution of "generate a geometry such that this UV seam is always on a triangle edge".

we may need to revisit the built-in "cube shader" on which this is based. I do not recall seeing the cube shader have the same artifacts, however...

I don't know all the shader code that's involved in the existing implementations but from a brief check it looks like the cube map sampling uses the builtin textureCube function so it shouldn't be an issue. Though I don't about the ENVMAP_TYPE_CUBE_UV case.

@drcmda
Copy link
Contributor

drcmda commented Aug 22, 2022

Giving repros based on environments like react-three-fiber that adjust defaults values especially when it's not clear what's being used

hmm, that would be very odd. we do not change defaults, a texture in fiber is just a THREE.Texture. it doesn't know what three is technically, i don't see how it would decide to change mipmaps of all things. there is not a single prototype hack or object default altered. i do understand it makes sense to convert to vanilla here, but if it's important to find the cause it's not that.

@FarazzShaikh suggested to me that the mipmap thing may come from PMREMGenerator.

@FarazzShaikh
Copy link
Contributor Author

I will take a detailed look at this when some time opens up between work. Admittedly I do not have very deep understanding of low level Textures and Mipmaps

@WestLangley
Copy link
Collaborator

@gkjohnson I misspoke, I meant to say equirect shader -- not cube shader.

@WestLangley
Copy link
Collaborator

@drcmda I am not sure the PMREM-related code in GroundProjectedEnv.js belongs there...

@gkjohnson
Copy link
Collaborator

hmm, that would be very odd. we do not change defaults, a texture in fiber is just a THREE.Texture. it doesn't know what three is technically, i don't see how it would decide to change mipmaps of all things.

My mistake. It looks like the difference here is that the official example uses RGBELoader whereas the repro case is using TextureLoader, which is likely where the difference in mipmap setting comes from.

@gkjohnson I misspoke, I meant to say equirect shader -- not cube shader.

As far as I understand the equirect map is converted to a cube map:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLBackground.js#L29

@WestLangley
Copy link
Collaborator

As far as I understand the equirect map is converted to a cube map:

Not in this example.

The problem is caused by uv = equirectUv( direction ) when mips are in use. And as you said, the problem occurs at the UV seam.

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Add ground projected env map

* fix: Update screenshot

* fix: Fix imports

* fix: reset build and lock files

* Update webgl_materials_envmaps_ground-projected.html

Update title.

* fix: lint

* feat: Add car to and limit GPU/camera in ground projected env example

* Reset files.json

* add webgl_materials_envmaps_ground-projected to files.json

* fix: glsl formatting in GroundProjectedEnv class

* Update screenshot for webgl_materials_envmaps_ground-projected

* lint fix

* Restrict controls furthur

* Lint GLSL and example

* Add rotation option; Move class to examples/objects

* remove rotation option

Co-authored-by: Michael Herzog <michael.herzog@human-interactive.org>
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
* Add ground projected env map

* fix: Update screenshot

* fix: Fix imports

* fix: reset build and lock files

* Update webgl_materials_envmaps_ground-projected.html

Update title.

* fix: lint

* feat: Add car to and limit GPU/camera in ground projected env example

* Reset files.json

* add webgl_materials_envmaps_ground-projected to files.json

* fix: glsl formatting in GroundProjectedEnv class

* Update screenshot for webgl_materials_envmaps_ground-projected

* lint fix

* Restrict controls furthur

* Lint GLSL and example

* Add rotation option; Move class to examples/objects

* remove rotation option

Co-authored-by: Michael Herzog <michael.herzog@human-interactive.org>
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.

Ground projected HDR environment map
9 participants