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: added particle system with morph targets demo #16109

Closed
wants to merge 3 commits into from

Conversation

looeee
Copy link
Collaborator

@looeee looeee commented Mar 31, 2019

@looeee
Copy link
Collaborator Author

looeee commented Mar 31, 2019

@WestLangley

Morph target support for PointsMaterial was added in #14107 and the example was updated to demonstrate the new feature.
I'm not sure if you want to keep one, both, or merge them. I do not have a strong opinion either way.

Hmm, I missed that one. Well, I'm OK with any of those options too, let's see what @mrdoob says.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 19, 2019

Closing. It's better to update the existing example webgl_morphtargets_sphere when #16110 gets merged.

@Mugen87 Mugen87 closed this Jun 19, 2019
@looeee
Copy link
Collaborator Author

looeee commented Jun 19, 2019

If this example has been rejected and people would rather keep the morphing sphere that's ok, but I still think we should at least rename the old example.

It's not at all clear that webgl_morphtargets_sphere is demonstrating how to use morph target with points, unless you examine the code.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 19, 2019

I'm fine with that. It's best to make this change directly in #16110.

@WestLangley
Copy link
Collaborator

@looeee I think your horse example is a great example and is worthy of being merged somehow -- either as webgl_morphtargets_points or webgl_points_morphs.

@looeee
Copy link
Collaborator Author

looeee commented Jun 20, 2019

webgl_morphtargets_points

That name is better actually. Would you prefer to keep the webgl_morphtargets_sphere example as well, or replace it?

It's best to make this change directly in #16110.

Let's keep things simple and get that PR merged first, we can update the examples after. In any case I'm currently up a mountain in Myanmar with no laptop, so I'm in no position to make any changes to these PRs for a couple of weeks.

BTW thanks for the support, @WestLangley. I'll make any necessary changes and reopen this PR for further consideration once I'm back to work and #16110 has been merged.

@mrdoob
Copy link
Owner

mrdoob commented Jun 23, 2019

@looeee how about implementing this in webgl_morphtargets_horse? so we can see the mesh and points side by side.

@looeee
Copy link
Collaborator Author

looeee commented Jun 24, 2019

Sure, that's fine although it might take away a bit from the visual appearance. Not a big deal though, and I can probably tweak it to look good enough.

On the other hand, doesn't that mean that we'd still have no obvious example demonstrating morph targets with points? The reason I submitted this example in the first place was that I didn't know we already had example code demonstrating this, hidden inside webgl_morphtargets_sphere.

@WestLangley
Copy link
Collaborator

how about implementing this in webgl_morphtargets_horse?

I do not think webgl_morphtargets_horse is a very good file name. Likewise, webgl_morphtargets_sphere.

Better names are webgl_morphtargets_points and webgl_morphtargets_mesh -- or just webgl_morphtargets.

And for the benefit of users, having the example demonstrate a single feature is best -- unless there is some reason to combine them.

I do not recall hearing complaints that the examples are too simple. :-)

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 24, 2019

Renaming the examples is a good idea!

@looeee
Copy link
Collaborator Author

looeee commented Jul 9, 2019

OK, #16110 has been merged and I'm back down from the mountains so I'll move on with this. I don't want to redo this example just to have it rejected again though, so can we come to a consensus first? Here's the options:

  1. Rename the webgl_morphtargets_sphere example to webgl_morphtargets_points (or something else?)
  2. Replace webgl_morphtargets_sphere with the horse example and rename it to webgl_morphtargets_points
  3. Do nothing

Thoughts?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 9, 2019

I vote for option 1

@looeee
Copy link
Collaborator Author

looeee commented Jul 9, 2019

I vote for option 1

OK, well obviously my preference is option 2 here, for a couple of reasons. It's visually more interesting, and it would also allow us to remove the AnimatedMorphSphere.gltf model from this repo, since it's already available on the Khronos repo.

On the other hand, it's slightly more complex (150 LOC old example, 194 LOC this example). But both examples are less complex than many others so I don't think we need to worry about that.

@Mugen87, what's your reason for preferring the old example over this one?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 9, 2019

Eventually, both examples are so little different from each other that I don't see any reason to replace the existing one. Hence, I think renaming is totally sufficient...

@looeee
Copy link
Collaborator Author

looeee commented Jul 9, 2019

both examples are so little different from each other

Technically, no. Visually they are very different, and personally I think that the updated examples looks much better than the old one, or I wouldn't be pushing so hard for this. Do you not think that visual appearance is one of the criteria we should consider for the examples?

On the other hand, it's hard to be objective about one's own work, so if you don't like the appearance of the new one then please feel free to say so 😅

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