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: Simplify default value for ExtrudeGeometry options. #28365

Closed
wants to merge 1 commit into from

Conversation

linbingquan
Copy link
Contributor

Related issue: #XXXX

Description

As the title says.

Copy link

github-actions bot commented May 13, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
678.6 kB (168.1 kB) 678.6 kB (168.1 kB) -70 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
457.8 kB (110.4 kB) 457.8 kB (110.4 kB) +0 B

const extrudePath = options.extrudePath;

const uvgen = options.UVGenerator !== undefined ? options.UVGenerator : WorldUVGenerator;
const { curveSegments, steps, depth, extrudePath, UVGenerator: uvgen } = options;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain the UVGenerator: uvgen pattern? What exactly does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain the UVGenerator: uvgen pattern? What exactly does it?

doc: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

other link: https://hacks.mozilla.org/2015/05/es6-in-depth-destructuring/

In my Chinglish to explain, most chances are that people do not understand...
These two links should explain

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I interpret it like so: Since uvgen is not directly in options, it can't be unpacked from options. So UVGenerator is unpacked and assigned to uvgen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code does not use ES6 syntax, is it easy to accept?

Copy link
Collaborator

@Mugen87 Mugen87 May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I leave this up to @mrdoob.

Sometimes, "dumb" code is easier to read but I understand it's also a matter of taste. I approve code syntax related PRs only then when I know @mrdoob has approved the style in general.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still prefer the "dumb" code version yeah...

@mrdoob mrdoob closed this May 16, 2024
@linbingquan linbingquan deleted the dev-extrude-geometry branch May 17, 2024 10:50
@Mugen87 Mugen87 added this to the r165 milestone May 17, 2024
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

3 participants