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

Decide future of "mesh-smooth" component #411

Closed
diarmidmackenzie opened this issue Jan 11, 2023 · 3 comments
Closed

Decide future of "mesh-smooth" component #411

diarmidmackenzie opened this issue Jan 11, 2023 · 3 comments

Comments

@diarmidmackenzie
Copy link
Member

diarmidmackenzie commented Jan 11, 2023

We've been reviewing all the components in this repo, and retiring a few that no longer seem valuable - see #395.

It's not clear what to do with 'mesh-smooth'.

As per this comment, it seems to still be compatible with the latest three.js, but we have no examples showing what it does, and from my testing I've not been able to get it to do much of any use.

When applied to a gltf, it seems to either have no effect, or (in one case) make the whole mesh go black with no materials - not very helpful. See this glitch example.

Looking back to when it was introduced, it seems it was primarily tested with json models.
#165

The original proposal was a parameter on the json-model loader.
#163

That PR was rejected, and instead a generic component was proposed on the basis that "this would work for any file format, too". But is it actually useful for any other fomat?

The JSON model loader has subsequently been removed

Only pause for hesitation before deleting the 'mesh-smooth' component is this comment from @donmccurdy at the end of #163 "Actually I just ran into a use case for this with a glTF model, too." - though no detail on what this use case was...

It's such a simple component, I don't see any real loss from removing it, and I'd rather not clutter the repo with stuff where we don't understand the purpose or have any working examples. If that causes anyone any problems, they can explain their use case & we can re-instate.

@vincentfretin - I propose we remove it as per your previous suggestion. If you're OK with that I'll do a PR.

@vincentfretin
Copy link
Member

Let's remove it for 7.0.0. If someone depends on this component, they can copy the previous code in their project or contribute a better one as a separate repository.

@donmccurdy
Copy link
Collaborator

Just some background — Knowing more now than I did then, it's true that the mesh-smooth component is format-agnostic. It doesn't generalize to all (most?) models though, because it relies on an assumption that vertices have already been welded. A somewhat more robust implementation would:

  1. remove existing normals
  2. weld vertices with some error tolerance
  3. compute vertex normals as before

That's a more expensive set of steps, and will still fail if vertices on adjacent faces have UV seams or vertex colors that prevent welding.

tl;dr — I think it's totally reasonable to remove the component, and I support that and any other reorganization you all choose to do here!

@vincentfretin
Copy link
Member

Thanks @donmccurdy for your feedback!

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

No branches or pull requests

3 participants