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

Examples: Convert loaders to ES6 Part III. #21616

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Apr 9, 2021

Related issue: -

Description

More loader conversions.

I did not convert all loader to code to let/const since some code depends on the specific scoping of var. Since I'm not 100% familiar with the format of the following loaders, I would like to ask other devs for help and remove the remaining usage of var in 3DMLoader, EXRLoader, GCodeLoader, VTKLoader and LDrawLoader.

/ping @tentone, @yomboprime, @fraguada, @sciecode

@Mugen87 Mugen87 added this to the r128 milestone Apr 9, 2021
@Mugen87 Mugen87 merged commit 9ff1484 into mrdoob:dev Apr 9, 2021
@yomboprime
Copy link
Collaborator

I've tested the LDrawLoader exhaustively (all models and options) and seems to be working right.

@mrdoob
Copy link
Owner

mrdoob commented Apr 9, 2021

Thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 9, 2021

What I have meant is that there is still code in the above loaders using var that needs to be converted.

@tentone
Copy link
Contributor

tentone commented Apr 9, 2021

Looks okay, On the GCodeLoader we can replace all var with let without problem.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 9, 2021

@tentone Unfortunately that did not work in GCodeLoader. At the following place the linter says line is not defined when using let/const.

if ( line.extruding ) {

The entire function addSegment() should be refactored so it does not rely on variables outside its scope. There are few places in the code base where such changes are necessary.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 9, 2021

This is another confusing place:

function addObject( vertex, extruding ) {
var geometry = new BufferGeometry();
geometry.setAttribute( 'position', new Float32BufferAttribute( vertex, 3 ) );
var segments = new LineSegments( geometry, extruding ? extrudingMaterial : pathMaterial );
segments.name = 'layer' + i;
object.add( segments );
}

Where does i come from? There are multiple loops in the outer function and it's hard to understand which i is valid in addObject().

@tentone
Copy link
Contributor

tentone commented Apr 9, 2021

The methods should be moved into the loop. i variable comes from there, I will create a branch and open a PR too clean these up.

Thanks a lot!

@mrdoob
Copy link
Owner

mrdoob commented Apr 13, 2021

@Mugen87 @marcofugaro do you guys get this when running npm run build-examples?

Screenshot 2021-04-13 at 21 39 55

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 13, 2021

I had the same error lately because of an outdated rollup version. Run npm i and try it again.

@mrdoob
Copy link
Owner

mrdoob commented Apr 13, 2021

That was it. Thanks!

@fraguada
Copy link
Contributor

@Mugen87 starting to look at this now to fix the remaining var. Thanks!

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.

5 participants