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: Make all parameters optional. #22536

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Sep 14, 2021

Related issue: #22513

Description

ExtrudeGeometry can now be created without ctor paramters. The default shape is a quad.

The PR also improves the default values of depth, bevelThickness and bevelSize which were out of scale compared to other geometry generators.

This change does not affect TextGeometry which overwrites these default values in its ctor.

@mrdoob mrdoob added this to the r133 milestone Sep 14, 2021
@mrdoob mrdoob merged commit b968f48 into mrdoob:dev Sep 14, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 14, 2021

Thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 16, 2021

This change does not affect TextGeometry which overwrites these default values in its ctor.

TextGeometry is a difficult case since the engine needs a default font if no one is provided. However, embedding a font inside the core is not an option because of the increase in size. (+ ~100 KB).

I'm not sure how to handle TextGeometry 🤔 . This class is also problematic to serialize/deserialize because of the dependency to a font...

@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2021

Maybe we should move it out of core?

@joshuaellis joshuaellis mentioned this pull request Sep 18, 2021
23 tasks
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 21, 2021

Considering #22559, it's probably best doing so. Should we also move Font and FontLoader to the examples? AFAIK in almost all cases these classes are used in combination with TextGeometry.

@mrdoob
Copy link
Owner

mrdoob commented Sep 21, 2021

I think so yes 👍

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.

2 participants