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

fix: Fix screenCoordinates outline rendering in WebGL #1492

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Conversation

0b5vr
Copy link
Contributor

@0b5vr 0b5vr commented Sep 13, 2024

Description

  • ea1e946: This commit adds new materials with outline to feature-test.html and webgpu-feature-test.html.
    • I suspect that we will have an issue that outline does not appear in r169 (next) WebGPURenderer. This commitment is to prepare to fix the issue.
    • I planned to just do this change in this PR but I spotted an issue I described below,,,
  • cd6c501: This commit fixes the screenCoordinates outline in WebGL.

The screenshot of the new feature-test.html:

image

I suspect that we will have an issue that outline does not appear in r169 WebGPURenderer

Also I spotted a bug in WebGL outline when the mode is screenCoordinates
@0b5vr 0b5vr added bug Something isn't working examples Good examples helps people a lot labels Sep 13, 2024
@0b5vr 0b5vr added this to the next milestone Sep 13, 2024
@0b5vr 0b5vr requested a review from yue4u September 13, 2024 10:44
@0b5vr 0b5vr self-assigned this Sep 13, 2024
@0b5vr
Copy link
Contributor Author

0b5vr commented Sep 13, 2024

I suspect that we will have an issue that outline does not appear in r169 (next) WebGPURenderer. This commitment is to prepare to fix the issue.

This was because this line fails if we load both three.module.js and three.webgpu.js in a catastrophic way.

if (!(surfaceMaterial instanceof THREE.Material)) {

And this easily happens when we write our importmap in this way:

<script type="importmap">
  {
    "imports": {
      "three": "https://cdn.jsdelivr.net/npm/three@0.168.0/build/three.module.js",
      "three/webgpu": "https://cdn.jsdelivr.net/npm/three@0.168.0/build/three.webgpu.js",
      // ...
    }
  }
</script>

maybe we should consider changing the instanceof line to more unsophisticated way? I think the Three.js codebase actually doesn't use instanceof against any Three.js classes.

outlineTex = texture2D( outlineWidthMultiplyTexture, outlineWidthMultiplyTextureUv ).g;
#endif

#ifdef OUTLINE_WIDTH_WORLD
Copy link
Contributor

@yue4u yue4u Sep 18, 2024

Choose a reason for hiding this comment

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

so OUTLINE_WIDTH_WORLD is no longer needed?

Copy link
Contributor Author

@0b5vr 0b5vr Sep 20, 2024

Choose a reason for hiding this comment

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

Yes, while I agree that it is too implicit to acknowledge that OUTLINE && !OUTLINE_WIDTH_SCREEN == OUTLINE_WIDTH_WORLD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up removing the line @ 1426a61

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe leave a comment or something in shader code would help future us? Anyway LGTM.

vec2 projectedNormal = normalize( clipNormal.xy );
projectedNormal.x *= projectionMatrix[ 0 ].x / projectionMatrix[ 1 ].y;
gl_Position.xy += 2.0 * outlineWidthFactor * outlineTex * projectedNormal.xy;
outlineOffset *= vViewPosition.z / projectionMatrix[ 1 ].y;
Copy link
Contributor

Choose a reason for hiding this comment

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

outlineOffset has been fixed and using the same logic in tsl

@0b5vr 0b5vr merged commit a1df947 into dev Sep 24, 2024
5 checks passed
@0b5vr 0b5vr deleted the examples-outline branch September 24, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working examples Good examples helps people a lot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants