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

[breaking] remove geometry.mergeTo #3191

Merged
merged 1 commit into from
Oct 25, 2017
Merged

Conversation

ngokevin
Copy link
Member

@ngokevin ngokevin commented Oct 25, 2017

Description:

  • Better/more efficiently done as a manual operation (e.g., within a component, all three.js code) versus declarative.
  • Properly would need to support buffer geometries which would be hard to do declaratively since we'd need to pre-define the vertex float array.
  • Not often used (if I ever run into the case I need to merge, I'll throw up a Glitch).
  • Currently, doesn't seem to work (five issues filed)

Changes proposed:

  • Remove geometry.mergeTo.

@donmccurdy
Copy link
Member

Did mergeTo never support BufferGeometry? I wrote a function to merge arbitrary descendants of a THREE.Object3D here:

https://github.com/donmccurdy/three-to-cannon/blob/master/index.js#L287-L334

It's trickier than I expected, getting the transformations right. 😕

What do we want to recommend in the place of mergeTo? We could write up that code (complexity and all), or do a simpler example that is easier to understand but less all-purpose.

Another option, in the vein of Inspector plugins, would be providing a way to select an entity in the inspector and export it to glTF via THREE.GLTFExporter, merging everything then. This would let you create a scene declaratively, but then "bake" it into a single glTF. But GLTFExporter does not automatically merge anything, either.

@ngokevin
Copy link
Member Author

I was working on changing the example to manually do it in three.js, but it got complex even for an arbitrary example of tons of cubes with the same material. I think we can just link to three.js mergeTo examples and your code. It's an advanced step that fits a small number of situations (instanced geometries seems more of a flexible/alternative perf boost). If I ever run into the use case, I'll share some code or maybe a component.

@dmarcos
Copy link
Member

dmarcos commented Oct 25, 2017

I implemented mergeTo based on the hypothesis that will be useful, easy to use and understand. I think the evidence shows the opposite. I'm happy to jettison the feature and keep thinking on alternative ways for people to optimize their scenes. Using the inspector for that purpose is a good idea.

@dmarcos dmarcos merged commit 30bfe4f into aframevr:master Oct 25, 2017
@donmccurdy
Copy link
Member

One other idea would be writing some examples showing how to use @takahirox's instancing component. It may be under-utilized at the moment.

But the most obvious draw call overloads I've seen have been models from sources like Revit, where a single OBJ might have several thousand meshes... that's probably a problem of educating users and pointing them to the right simplification tools, more than adding features to A-Frame.

@ngokevin
Copy link
Member Author

@donmccurdy
Copy link
Member

Nice! It looks like that still has the "no BufferGeometry" limitation, if so it may be worth documenting the distinction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants