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.

Sign up for GitHub

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

Jump to bottom

materials: Convert to es6. #20100

Merged
merged 5 commits into from
Feb 17, 2021
Merged

materials: Convert to es6. #20100

merged 5 commits into from
Feb 17, 2021

Conversation

linbingquan
Copy link
Contributor

@linbingquan linbingquan commented Aug 17, 2020 •
edited
Loading

Clean up for #19986 (comment)

@linbingquan linbingquan changed the title materials: Covert to es6. materials: Convert to es6. Aug 17, 2020
@linbingquan
Copy link
Contributor Author

linbingquan commented Aug 17, 2020 •
edited
Loading

TBH, I am not good at English.

@Mugen87 This PR is not allow to change examples/jsm's files like #19989 (comment) . It that right?

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 17, 2020

At least not LineMaterial. In general, don't modify files in examples/jsm which have counterparts in examples/js.

Please keep in mind that such files are always authored in examples/js and the respective modules generated with the modularization script.

@DefinitelyMaybe DefinitelyMaybe mentioned this pull request Aug 20, 2020
Closed
43 tasks
@mrdoob mrdoob added this to the r121 milestone Aug 23, 2020
@mrdoob mrdoob modified the milestones: r121, r122 Sep 29, 2020
@mrdoob mrdoob modified the milestones: r122, r123 Oct 28, 2020
@mrdoob mrdoob modified the milestones: r123, r124 Nov 25, 2020
@mrdoob mrdoob modified the milestones: r124, r125 Dec 24, 2020
@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@DefinitelyMaybe
Copy link
Contributor

DefinitelyMaybe commented Feb 9, 2021 •
edited
Loading

Hey @linbingquan, for this PR, could we please stick to src/ files for the moment? Like a couple of my PR's there's also been a number of changes to the files since the PR was created which creates all the current conflicts

Mugen87
Mugen87 requested changes Feb 17, 2021
View reviewed changes
Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

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

@linbingquan This PR can't be merged in its current form. Please focus on src and resolve #20100 (comment).

@linbingquan
Copy link
Contributor Author

@linbingquan This PR can't be merged in its current form. Please focus on src and resolve #20100 (comment).

Thank you for your reply, I had changed it.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2021

The PR still modifies examples/.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2021

LineMaterial is derived from ShaderMaterial. So you can't convert this class.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2021 •
edited
Loading

Sorry, you should really focus more when you work on these PRs. The last changes were quite sloppy.

@linbingquan
Copy link
Contributor Author

Sorry, you should really focus more when you work on these PRs. The last changes were quite sloppy.

I am so sorry, wasted your valuable time, I make a mistake at first commit, I am confusion why this PR not pass the E2E testing, I am not strictly review by myself.

image

@linbingquan
Copy link
Contributor Author

linbingquan commented Feb 17, 2021 •
edited
Loading

LineMaterial is derived from . So you can't convert this class.ShaderMaterial

@Mugen87 Class LineMaterial extends Class ShaderMaterial and Class ShaderMaterial extends Class Material, other class material extends Class Material.

If not to change examples/jsm/lines/LineMaterial.js, It is hard to covert to ES6...

@linbingquan linbingquan marked this pull request as draft February 17, 2021 14:39
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2021 •
edited
Loading

Well, it seems it's necessary to stall this PR until LineMaterial can be converted.

Or you decide to undo the changes to LineMaterial, ShaderMaterial and Material.

@linbingquan linbingquan marked this pull request as ready for review February 17, 2021 16:00
@linbingquan
Copy link
Contributor Author

Or you decide to undo the changes to LineMaterial, ShaderMaterial and Material.

@Mugen87 Thank you for your help. I think this PR is OK now.

@linbingquan linbingquan requested a review from Mugen87 February 17, 2021 16:07
@mrdoob mrdoob merged commit 60ccc81 into mrdoob:dev Feb 17, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 17, 2021

Thanks!

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 20, 2021 •
edited
Loading

This PR broke: GLTFMeshStandardSGMaterial

three.js/examples/js/loaders/GLTFLoader.js

Lines 1063 to 1065 in a036601

function GLTFMeshStandardSGMaterial( params ) {
THREE.MeshStandardMaterial.call( this );

@linbingquan I suggest you revert the class conversion of MeshStandardMaterial and MeshPhysicalMaterial .

@Mugen87 Mugen87 mentioned this pull request Feb 20, 2021
Merged
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 20, 2021 •
edited
Loading

@linbingquan Never mind: #21319

@mrdoob
Copy link
Owner

mrdoob commented Feb 20, 2021

Good catch!

@Mugen87 Mugen87 mentioned this pull request Feb 21, 2021
Merged
@donmccurdy
Copy link
Collaborator

Sorry that GLTFLoader held this back — can we change GLTFMeshStandardSGMaterial to an ES6 class as well, or is there more to it than that?

@mrdoob
Copy link
Owner

mrdoob commented Feb 23, 2021

It's all good! I think we made quite a lot of progress this month in the transition regardless.

@linbingquan linbingquan deleted the dev-es6-materials branch February 28, 2021 10:45
@Mugen87 Mugen87 mentioned this pull request Feb 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcofugaro marcofugaro marcofugaro left review comments

@Mugen87 Mugen87 Mugen87 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
r126
Development

Successfully merging this pull request may close these issues.
6 participants
@linbingquan @Mugen87 @DefinitelyMaybe @mrdoob @donmccurdy @marcofugaro