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

Toggle visibility + Quaternion Support #439

Merged
merged 10 commits into from
Feb 25, 2019
Merged

Conversation

cfneuhaus
Copy link
Contributor

I have added:

  • The possibility to toggle visibility of objects. This should be a more efficient way to en/disable rendering of objects, than v-if, since it does not recreate the objects.
  • Support for rotation quaternions to specify rotation of objects.

@cfneuhaus
Copy link
Contributor Author

Mh. Not sure about these failing checks. Any ideas?

@h-ikeda
Copy link
Member

h-ikeda commented Feb 19, 2019

Thank you very much for your creating PR !
It's a linting error.
See https://circleci.com/gh/vue-gl/vue-gl/1839?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link

@cfneuhaus
Copy link
Contributor Author

Ok, I have fixed the linting errors. Please tell me if further changes are required. Thank you!

@h-ikeda
Copy link
Member

h-ikeda commented Feb 24, 2019

Well, I'm wondering about rotationQuaternion naming. Since three.js uses the name just quaternion, we might prefer the quaternion than rotationQuaternion. Could you tell me why you named it so if you have any ideas?

The quaternion will overwrite the rotation configuration at initializing. That should be documented in the prop comment. (Comment will appear in published documentation page.)

And, could you modify the test case? What you need to do is just adding new propsData and assertions to test/core/vgl-object3d.spec.js .

Thank you :-)

@cfneuhaus
Copy link
Contributor Author

cfneuhaus commented Feb 24, 2019

I have added the tests and comments. It does not make much sense to use rotation and rotationQuaternion in at the same time, so I stated this in the documentation. Technically yes, rotationQuaternion overrides rotation in the initialization, however if you dynamically change the props at some point, the behavior would be reaaally confusing and weird if you used both at the same time.
For this reason, I added additional tests to test rotationQuaternion separately.

As for the name of the prop: To be honest, I feel like the original property in Three.js is not well-named. 'quaternion' is a similar term as 'matrix' or 'vector. And you wouldn't call a some specific vector or rotation matrix just 'vector' or 'matrix' in a property, would you? Therefore I feel like rotationQuaternion is clearer. If you prefer the original name for consistency, I can also change it back. Whatever you prefer.

@h-ikeda
Copy link
Member

h-ikeda commented Feb 25, 2019

Thank you for good explanation about naming. Actually, I couldn't catch the nuance since I usually don't use English. I agree to clearer naming.
I'm merging this :-)

@h-ikeda h-ikeda merged commit c33ddd0 into vue-gl:master Feb 25, 2019
@cfneuhaus cfneuhaus deleted the merge_req branch May 13, 2019 08:52
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.

2 participants