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: Fix displacement map usage of velocity shader. #26324

Merged
merged 1 commit into from
Jun 24, 2023

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jun 24, 2023

Related issue: -

Description

The displacement map for the velocity shader in webgl_materials_channels is broken since the displacement defines are not properly generated. This can easily be fixed by adding a member displacementMap to the custom material.

@Mugen87 Mugen87 marked this pull request as ready for review June 24, 2023 09:18
@Mugen87 Mugen87 added this to the r154 milestone Jun 24, 2023
@Mugen87 Mugen87 merged commit 80118ab into mrdoob:dev Jun 24, 2023
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 24, 2023

@mrdoob The handling of VelocityShader is error-prone and clumsy right now. I suggest to add MeshVelocityMaterial as originally suggested in #23784. I've implemented this in a separate branch and improved the code a bit:

Mugen87@9d23df9

Look how this approach simplifies the usage of the material in the example. I think it's totally fine of having MeshVelocityMaterial next to MeshNormalMaterial or MeshDepthMaterial. Considering that it has great importance for motion blur, TRAA (which I'm hacking around at the moment) and XR, I would say having it in core is appropriate. At least it has similar relevance as MeshNormalMaterial which is in core, too.

@mrdoob
Copy link
Owner

mrdoob commented Jul 6, 2023

@Mugen87 MeshVelocityMaterial sounds good to me 👍

EDIT: But does it need to be in src/?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 6, 2023

Without an integration of MeshVelocityMaterial in the core, there is no simplification on app level.

We could place MeshVelocityMaterial in the examples folder like MeshGouraudMaterial. However, I think it's wrong doing this when modules like WebGLRenderer, WebGLMaterials and WebGLPrograms have velocity specific code. Against this backdrop, I would apply the same rules to MeshVelocityMaterial like for MeshNormalMaterial and MeshDepthMaterial.

BTW: It isn't actually a big change. A PR would add around 200 loc to the core. That is essentially LatheGeometry. If we want to save space, I would argue to include MeshVelocityMaterial and move the less tightly coupled LatheGeometry (and CapsuleGeometry) to the examples.

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.

2 participants