Skip to content

Conversation

@mrdoob
Copy link
Owner

@mrdoob mrdoob commented May 26, 2023

Related issue: #26131 #25494

Description

This PR reverts #26131 because performance was really bad on mobile.

It also reverts #25494 as I am still not convinced the blinking artefacts we see with royal_esplanade_1k.hdr are good enough reason to sacrifice the quality in materials with low roughness.

/cc @elalish

@mrdoob mrdoob added this to the r153 milestone May 26, 2023
@github-actions
Copy link

github-actions bot commented May 26, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
642.8 kB (159.1 kB) 642.9 kB (159.1 kB) +86 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
432.8 kB (104.8 kB) 432.9 kB (104.8 kB) +80 B

@mrdoob mrdoob merged commit ec549b1 into dev May 26, 2023
@mrdoob mrdoob deleted the transmission branch May 26, 2023 02:51
@mrdoob
Copy link
Owner Author

mrdoob commented May 26, 2023

After merging this I noticed that mobile (Pixel 7) was still a little bit slow so I wondered if we could get away with using the WebGL 1 path (floorPowerOfTwo):

_this.getDrawingBufferSize( _vector2 );
_transmissionRenderTarget.setSize( floorPowerOfTwo( _vector2.x ), floorPowerOfTwo( _vector2.y ) );
getDrawingBuffer floorPowerOfTwo
Screenshot 2023-05-26 at 20 27 33 Screenshot 2023-05-26 at 20 27 37

Interestingly, the black parts of the lightbulb filament look pretty much the same but the bright part of the filament becomes much more blurred/pixelated.

Does anybody understand why this happens?

/cc @elalish @WestLangley @N8python

@WestLangley
Copy link
Collaborator

As an experiment, I hacked the transmission shader code to constrain the LOD to no more than 6 for rough materials. roughness is 1 here. Plane mesh.

Something is not right...

Dev version Constrained LOD
May-26-2023 07-52-38 May-26-2023 07-54-16

@mrdoob
Copy link
Owner Author

mrdoob commented May 26, 2023

Here's a couple of videos that may help:

Video 1

6.N8.Programs.Twitter.2.mp4

Video 2

I think this video shows the issue pretty well (click to download, displays on MacOS):

Screenshot 2023-05-27 at 00 39 03

https://github.com/mrdoob/three.js/files/11576775/6.N8.Programs.Twitter.mp4.zip

We did them while we were working on #25483.

@elalish
Copy link
Contributor

elalish commented May 26, 2023

Agreed with @WestLangley - that's super strange. Maybe a bad blur that isn't hitting all the pixels?

@mrdoob, the reason the filament looks more blocky is that it has values >>1, next to values <1 - they're getting interpolated linearly, and then tone-mapped after. This results in looking almost like nearest interpolation. All the pixel interpolation and antialiasing (stuff for perceptual niceness) really needs to happen after tonemapping to work properly.

Also @mrdoob, your second video appears to be an image. Do you have any idea what's causing what you see? Is the blur based on depth or something?

@mrdoob
Copy link
Owner Author

mrdoob commented May 29, 2023

Also @mrdoob, your second video appears to be an image. Do you have any idea what's causing what you see? Is the blur based on depth or something?

Damn! So hard to share that video... I've updated the post.

@mrdoob
Copy link
Owner Author

mrdoob commented May 29, 2023

I think I understand the issue.

As you say, it's linear interpolating and it doesn't have enough pixels to interpolate to such high values and ends up being blocky/pixelated.

I feel like reverting to 1024x1024 texture just happened to produce better results with that environment, but the issue is still there.

@WestLangley
Copy link
Collaborator

It doesn't happen when the background is flat. Comment out EquirectangularReflectionMapping.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 30, 2023

Where you can see the flickering pretty well is in webgl_loader_gltf_lights when you move the camera such that the transmissive lamp shade moves over light and dark areas of the background.

If we can't find a final solution for this issue, I vote to revert to the r152 state so there is no change in behavior. The intense blinking in webgl_loader_gltf_lights feels buggy whereas the the lower (1024x1024) resolution more like a quality issue.

@mrdoob
Copy link
Owner Author

mrdoob commented May 31, 2023

It doesn't happen when the background is flat. Comment out EquirectangularReflectionMapping.

Made a screen recording to try to understand... What is it that doesn't happen?

Screen.Recording.2023-05-31.at.23.54.00.mov

@WestLangley
Copy link
Collaborator

When I tested flat background and panned the scene when roughness > 0.8 or so, there was no so-called "flashing" of the transmissive object.

@mrdoob
Copy link
Owner Author

mrdoob commented May 31, 2023

Any chance you could do a screen recording...?

@mrdoob
Copy link
Owner Author

mrdoob commented May 31, 2023

@Mugen87

If we can't find a final solution for this issue, I vote to revert to the r152 state so there is no change in behavior. The intense blinking in webgl_loader_gltf_lights feels buggy whereas the the lower (1024x1024) resolution more like a quality issue.

As far as I remember, we originally reverted to 1024x1024 because @WestLangley mentioned the color changes in webgl_materials_physical_transmission and now there's the blinking issue in webgl_loader_gltf_lights.

At the same time, @elalish's feedback is that model-viewer's transmission looks bad.

I fear our examples are not representative of most projects that use transmission out there...

Before reverting again I would like to hear more from the community.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 31, 2023

Sounds good!

@WestLangley
Copy link
Collaborator

Any chance you could do a screen recording...?

To test a "flat" background, I modified the example like so:

//texture.mapping = THREE.EquirectangularReflectionMapping;
texture.offset.set( 0.67, 0.55 );
texture.repeat.set( 0.125, 0.125 );

Sorry about the ugly banding. It is not evident in practice...

Flat Background

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.

4 participants