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

ExtrudeGeometry: Remove option "frames" #13587

Merged
merged 1 commit into from
Mar 14, 2018
Merged

ExtrudeGeometry: Remove option "frames" #13587

merged 1 commit into from
Mar 14, 2018

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Mar 14, 2018

I'd like to suggest another simplification of ExtrudeGeometry: The removal of options.frames or the possibility to apply custom Frenet–Serret frames to the constructor.

Analogous to TubeGeometry frames are now always calculated by the given curve via Curve.computeFrenetFrames.

var frames = path.computeFrenetFrames( tubularSegments, closed );

option.frames was never used in the examples so there is no need for modifications.

@Mugen87 Mugen87 changed the title ExtrudeGeometry: Remove option frames ExtrudeGeometry: Remove option "frames" Mar 14, 2018
@mrdoob mrdoob added this to the r91 milestone Mar 14, 2018
@mrdoob mrdoob merged commit 84bdd96 into mrdoob:dev Mar 14, 2018
@mrdoob
Copy link
Owner

mrdoob commented Mar 14, 2018

Agreed. Thanks!

@WestLangley
Copy link
Collaborator

Rather than removing the feature, I think it would be better to add an example demonstrating it.

What if you want to extrude a "winding yellow-brick road", and you want it to remain level so you can walk on it -- or a race track, and you want to control the embankment so you can drive on it.

The parallel transport frames implementation (#1569) does not give you that kind of control.

I recommend reverting this.

screen shot 2018-03-14 at 11 24 40 am

@mrdoob
Copy link
Owner

mrdoob commented Mar 14, 2018

Okay. We should definitely add a example.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 14, 2018

My long-term intention was to simplify ExtrudeGeometry a bit since its implementation feels a bit overwhelming. But I'm okay if we keep this feature and demonstrate it with an example.

Would it be okay for you to make the public methods .addShapeList(), .addShape() and .getArrays() private? 😇

@WestLangley
Copy link
Collaborator

I support anything you want to do to improve it. I just did not think removing the particular feature you removed was a good idea. The user does not have control over the first frame, so the 2D shape that is extruded can start out being rotated unexpectedly.

But I can see how one could argue that the feature is too complicated... and proper modeling software is a better choice.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 14, 2018

There is another thing that bothers me. In my perfect world, we would remove all bevel related code from ExtrudeGeometry and have a BevelModifier instead. That would make the implementation of ExtrudeGeometry much more straightforward.

Unfortunately, the implementation of a generic BevelModifier is complicated. I've tried to study this topic last year but I've found almost no references. And reading the Blender source code is...well...

@WestLangley
Copy link
Collaborator

@Mugen87 @mrdoob After further thought, I think setting the frames is too tedious, so I now agree that the feature can be removed.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 22, 2018

That's great. Removing frames makes it also a bit easier to serialize ExtrudeGeometry.

@mrdoob
Copy link
Owner

mrdoob commented Mar 27, 2018

Should we revert the revert? 😁

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 27, 2018

I vote for yes 😁

@mrdoob
Copy link
Owner

mrdoob commented Mar 27, 2018

Done!

@Mowsh
Copy link

Mowsh commented Apr 5, 2018

My current project is using ExtrudeGeometry to create conveyors along arbitrary paths, the conveyors need to stay upright to match how they look and work in real life.

I feel like the frames option is the solution to a problem I've run into where at some angles my conveyors would choose strange rotations which look unrealistic. The roads that @WestLangley posted above look like exactly what is needed to make my conveyors look how I expect.

Is there a plan to replace this with equivalent functionality? Or to make ExtrudeGeometry not rotate in a way that users don't expect?

OT: @WestLangley Could you share the source code for the road image you posted above? I think this would be very useful for me.

Thanks

@WestLangley
Copy link
Collaborator

@Mowsh three.js is not modeling software. You really should be using modeling software, I think.

I have discovered it is very tedious specifying tangent frames; my examples above were just one-off hacks.

As an alternative, if your conveyor is flat, can you create a flat shape (like a worm) and extrude the thickness, rather than extruding the length?

@zz85
Copy link
Contributor

zz85 commented Apr 6, 2018

Hmm three.js may not be intended to be a modeller's tool in general but it should be easy to build a modelling software could be built on top of three.js? eg. specifying tangent frames may be tedious but the frames can be generated by another tool in js.

but removing this isn't all bad since we don't know how often this is used, and if someone requires this option for modelling for example, the Class could easily be copied and modified

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.

None yet

5 participants