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

changes to Vector3 for ES6 modules #101

Closed
pixelzoom opened this issue Mar 25, 2020 · 5 comments
Closed

changes to Vector3 for ES6 modules #101

pixelzoom opened this issue Mar 25, 2020 · 5 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 25, 2020

Noted while using Vector3 in Natural Selection, and working on #100.

In Vector3:

import dot from './dot.js';
import './Utils.js';
import './Vector3.js';
...
  slerp: function( start, end, ratio ) {
    // NOTE: we can't create a require() loop here
    return dot.Quaternion.slerp( new dot.Quaternion(), dot.Quaternion.getRotationQuaternion( start, end ), ratio ).timesVector3( start );
  },

Now that's we're using ES6 modules:

  • Can the odd imports be changed/removed?
  • Can the "NOTE" comment be removed/revised?
  • Can dot.Quaternion be replaced with an import?
@pixelzoom pixelzoom changed the title change to Vector3 for ES6 modules changes to Vector3 for ES6 modules Mar 25, 2020
@pixelzoom
Copy link
Contributor Author

Looks like there are other uses of the dot import that should be reviewed:

166 return Math.acos( dot.clamp( this.normalized().dot( v.normalized() ), -1, 1 ) );
492 return new dot.Vector2( this.x, this.y );
502 return new dot.Vector4( this.x, this.y, this.z, 1 );
781 return this.setXYZ( dot.Utils.roundSymmetric( this.x ),
782      dot.Utils.roundSymmetric( this.y ),
783      dot.Utils.roundSymmetric( this.z ) );

@zepumph
Copy link
Member

zepumph commented Feb 22, 2023

All the imports seemed used to me, I removed the NOTE that is now covered by the explanation of the ts-expect-error. Also there is still a need for the Quaternion workaround as far as I could see. Anything else @jonathanolson?

jonathanolson added a commit to phetsims/keplers-laws that referenced this issue Feb 22, 2023
@jonathanolson
Copy link
Contributor

The workaround isn't needed, it would just mean we'd have to use dot/js/imports.ts. So far it seems fine to leave it as-is, but maybe sometime we'll rip off that band-aid?

Cleaned up a few other things above. Look good @zepumph ?

@zepumph
Copy link
Member

zepumph commented Feb 22, 2023

Yes, looks nice, I also made #118 for when the time is right. Ready to close?

@zepumph zepumph assigned jonathanolson and unassigned zepumph Feb 22, 2023
@jonathanolson
Copy link
Contributor

Looks good to close, since we have the separate issue for circularity. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants