Skip to content

Conversation

@WestLangley
Copy link
Collaborator

Euler.onChange() and Quaternion.onChange() are used internally for synchronization, are not intended to be public.

As proposed in #16731 (comment).

@WestLangley
Copy link
Collaborator Author

The following have been left unchanged:

In Euler.d.ts

setFromRotationMatrix( m: Matrix4, order?: string, update?: boolean ): Euler;
setFromQuaternion( q: Quaternion, order?: string, update?: boolean ): Euler;

In Quaternion.d.ts

setFromEuler( euler: Euler, update?: boolean ): Quaternion;

If the boolean args can be removed without breaking anything, I'd be for doing that.

@WestLangley WestLangley added this to the r106 milestone Jun 11, 2019
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 11, 2019

If the boolean args can be removed without breaking anything, I'd be for doing that.

Let's say if update is removed from setFromEuler() but users still apply it, they will get TS errors like:

Expected 1 argument(s), but got 2.

However, since it's an optional parameter than was presumably not used very often I think it's okay if it is removed. We should just note this change in the migration guide.

@WestLangley
Copy link
Collaborator Author

Let's say if update is removed from setFromEuler() but users still apply it

The update flag is not for users to apply. That is the whole point of this PR.

Is there a problem if the arg is removed from the TS signature but the library passes the arg internally?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 11, 2019

Is there a problem if the arg is removed from the TS signature but the library passes the arg internally?

No, that's not a problem for the lib. However, users might the mentioned TS error.

@WestLangley
Copy link
Collaborator Author

No, that's not a problem for the lib.

Then it seems best to me to remove the update arg from the TS signature. No?

However, users might the mentioned TS error.

I don't understand what you are trying to say...

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 12, 2019

I don't understand what you are trying to say...

When a user currently does this in his TS code (for whatever reasons):

q.setFromEuler( e, true );

Then the user would get a TS error if the second parameter is removed from the TS declaration file.

@WestLangley
Copy link
Collaborator Author

Then the user would get a TS error if the second parameter is removed from the TS declaration file.

We do not want users to pass the update flag; it is for internal use only.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 12, 2019

Like I said, I would remove it from the TS file and note the change in the migration guide (just for the case somebody complains so we can refer to it).

@WestLangley
Copy link
Collaborator Author

I'll merge this, then, and update the TS flies and migration docs in a separate PR so the TS files can easily be reverted if need be.

@WestLangley WestLangley merged commit b6cd8de into mrdoob:dev Jun 12, 2019
@WestLangley WestLangley deleted the dev_onchange_callback branch June 12, 2019 19:10
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 12, 2019

Sounds good.

@mrdoob
Copy link
Owner

mrdoob commented Jun 12, 2019

Thanks for cleaning this up!

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.

3 participants