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

Change Three v79 to v80 #187

Merged
merged 1 commit into from
Oct 4, 2016
Merged

Change Three v79 to v80 #187

merged 1 commit into from
Oct 4, 2016

Conversation

gchoqueux
Copy link
Contributor

the Three library now uses ES6 Modules

peppsac
peppsac reviewed Sep 23, 2016
View reviewed changes
Copy link
Contributor

@peppsac peppsac left a comment

Choose a reason for hiding this comment

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

I'm not sure that using import * as THREE from 'THREE'; is a good idea.
I'd prefer to explicitely list imported elements ; what do you think?

src/Renderer/ThreeExtented/GlobeControls.js
@@ -225,8 +224,9 @@ function GlobeControls(camera, domElement, engine) {


if(enableTargetHelper)

{
Copy link
Contributor

Choose a reason for hiding this comment

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

if (enableTargetHelper) {

src/Renderer/ThreeExtented/GlobeControls.js
@@ -445,7 +445,9 @@ function GlobeControls(camera, domElement, engine) {
var delta = coord.altitude() - (bbox.top() + radiusCollision);

if(delta<0)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

if (delta < 0) {

@gchoqueux
Copy link
Contributor Author

gchoqueux commented Sep 27, 2016 •
edited
Loading

I'm not sure that using import * as THREE from 'THREE'; is a good idea.

Yes, I think too import * as THREE from 'THREE'; is not a best idea.

For example in https://github.com/iTowns/itowns2/blob/ThreeV80/src/Renderer/BasicMaterial.js#L8

import * as THREE from 'THREE';

becomes

import {RawShaderMaterial,Color,Matrix4} from 'THREE';

But we lose name's module

THREE.RawShaderMaterial.call(this);

becomes

RawShaderMaterial.call(this);

I didn't find import method to load specific member and keeping name's module :(
I think, it's better to keep name's module THREE

@tbroyer
Copy link
Contributor

tbroyer commented Sep 27, 2016

What's wrong with import * as THREE from 'three'? That's exactly what the pull request making the change to Three.js suggests.

@peppsac
Copy link
Contributor

peppsac commented Sep 27, 2016

In most language wildcard imports are considered a bad practice, hence my question.
In Javascript these imports are scoped thanks to the as XXX syntax so I suppose that's not an issue.

So it looks good but I need to test before giving my approval :-)

@gchoqueux gchoqueux force-pushed the RecastGlobeControl branch 3 times, most recently from 1b5c442 to 7b94f76 Compare October 3, 2016 14:57
@gchoqueux
Change Three v79 to v80
3f920f4 
the Three library now uses ES6 Modules
@gchoqueux gchoqueux changed the base branch from RecastGlobeControl to master October 4, 2016 15:50
@peppsac
Copy link
Contributor

peppsac commented Oct 4, 2016

LGTM

@peppsac peppsac merged commit 68107d4 into master Oct 4, 2016
@peppsac peppsac deleted the ThreeV80 branch October 4, 2016 16:13
NikoSaul pushed a commit to NikoSaul/itowns2 that referenced this pull request Jul 4, 2017
@peppsac
Merge pull request iTowns#187 from iTowns/ThreeV80
986d212 
Change Three v79 to v80
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peppsac peppsac peppsac left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@gchoqueux @tbroyer @peppsac