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(MeshReflectionMaterial)!: add features/docs, reorganize #377

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

andretchen0
Copy link
Contributor

@andretchen0 andretchen0 commented Apr 6, 2024

Problem

Some of MeshReflectionMaterial's features – e.g., features related to "blur" – weren't working.

Solution

Rewrite MeshReflectionMaterial

Playground at /materials/reflection-material


Problem

MeshReflectionMaterial was undocumented.

Solution

Document MeshReflectionMaterial at /guide/materials/mesh-reflection-material.html


Problem

blurPass didn't have a way of disposing its resources, leading to memory leaks.

Solution

Add dispose to blurPass and dispose it when necessary.


Breaking changes

The issue of breaking changes was brought up on the Discord. It was decided that since the material was undocumented up to this point, users probably don't rely on the current implementation, and there was little benefit in avoiding breaking changes.

  • blurPass and convolutionMaterial move inside MeshReflectionMaterial and their implementations change. These files were/are undocumented. Their implementations are relatively tightly bound to MeshReflectionMaterial's implementation, so they're not generally useful as written.
  • The PR activates a lot of previously non-working features of MeshReflectionMaterial. The visual output of the material is much different, for the same props.
  • I added some extra props, to hopefully make it easier to get the desired effects. For example, now the user can control how the strength of the blur effect on rough and smooth surfaces, separately. But that meant changing prop names.

Related issues

Some NOTE:/TODO:s have been left in the files concerning issues filed against the Tres core.

In particular, the Tres core currently doesn't reattach recompiled materials. Some props changes have to trigger a recompile of the material in order to take effect. Some of those are due to THREE.MeshStandardMaterial's implementation, so can't be worked around here, while still retaining the desired functionality/reactivity.

The material logs warnings if users change props in ways that trigger a recompile specific to this material – e.g., toggling the blur or "depth".


Room for improvement?

The material extends MeshStandardMaterial.

The material ran into reactivity/recompile issues when "inherited" props/defaults were not written out in MeshStandardMaterial.


closes #371

Copy link

stackblitz bot commented Apr 6, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@andretchen0 andretchen0 mentioned this pull request Apr 6, 2024
Closed
16 tasks
@andretchen0 andretchen0 added feature New feature or request breaking-change p3-significant High-priority enhancement (priority) labels Apr 6, 2024
@andretchen0 andretchen0 mentioned this pull request Apr 7, 2024
4 tasks
Copy link
Member

@JaimeTorrealba JaimeTorrealba left a comment

Choose a reason for hiding this comment

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

Question as this one have breaking changes, it would be merged into v4? or no problem

@andretchen0
Copy link
Contributor Author

Question as this one have breaking changes, it would be merged into v4? or no problem

Hey @JaimeTorrealba , I'm merging into v4 because of the breaking changes.

Do you see a problem with that?

@JaimeTorrealba
Copy link
Member

Not at all, is just I didn't see the label v4,

@andretchen0 andretchen0 added the v4 label Apr 8, 2024
@andretchen0
Copy link
Contributor Author

andretchen0 commented Apr 8, 2024

Not at all, is just I didn't see the label v4,

Ah, gotcha. I set the "merge into" branch to v4 in Github, but had not added the label.

Sorry for the confusion. I just added the v4 label.

@andretchen0 andretchen0 merged commit 6722b69 into v4 Apr 16, 2024
5 checks passed
@andretchen0 andretchen0 deleted the fix/mesh-reflection-material branch April 16, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change feature New feature or request p3-significant High-priority enhancement (priority) v4
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants