Skip to content

Conversation

@hamdiwanis
Copy link
Contributor

PR Checklist

What is the current behavior?

View can only rotate around z axis

What is the new behavior?

View can now rotate around x, y or z

Fixes/Implements/Closes #[Issue Number].

@ghost ghost added the ♥ community PR label Jun 13, 2018
@ns-bot
Copy link

ns-bot commented Jun 13, 2018

Please sign CLA at http://www.nativescript.org/cla

@ns-bot
Copy link

ns-bot commented Jun 13, 2018

CLA signature found, happy contributing!

@ns-bot ns-bot added cla: yes and removed cla: no labels Jun 13, 2018
@vakrilov
Copy link
Contributor

vakrilov commented Jun 14, 2018

Hi, thanks for the effort.
Look like there are some tslint errors - you can run npm run tslint to see what they are and fix them.
Also - please read the guide for submitting a PR. It is a good idea to open an issue/feature request before submitting a PR. As most of our styling properties are inspired by CSS it might be a good idea to an API similar to CSS 3d transforms.

@hamdiwanis
Copy link
Contributor Author

thanks, I have updated it to follow css api.

/**
* Defines a point in 3d space (x, y and z) for rotation in 3d animations.
*/
export interface Point {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would name it something like Point3D or something similar so that it is clear that this is not a 2D point but three dimensional.

@ghost ghost assigned vakrilov Jun 19, 2018
@ghost ghost added in progress and removed ♥ community PR labels Jun 19, 2018
@speigg
Copy link
Contributor

speigg commented Jun 19, 2018

This is awesome. Would it be difficult to add 3D translation to this PR? Or even make it possible to apply a 3D transformation matrix on a View? Seems like it would be closely related to these changes.

@speigg
Copy link
Contributor

speigg commented Jun 19, 2018

It’s also not obvious the what the euler order is, so it should probably be documented. A transformation matrix would be least ambiguous of course, and then you also get the ability to apply a shear transformation to a View.

@hamdiwanis
Copy link
Contributor Author

Thanks Gheric, you are right and i was already thinking about adding transformation matrix option but didn't had time so if you want to help i will appreciate it :)

@speigg
Copy link
Contributor

speigg commented Jun 27, 2018

Unfortunately I don’t have time either, but it would be a great addition!

@vakrilov
Copy link
Contributor

Hey @hamdiwanis
First of all - let me say you've done an awesome job with this PR! Thanks a lot for this effort!

We did a through code review and it turns out there are a quite a few areas of the code that are affected by 3d rotations.

Over the last few days I've:

  1. Merged @hamdiwanis 's PR in widgets: feat: Add 3d rotaion tns-core-modules-widgets#126. It is already live in tns-core-modules-widgets@next
  2. Rebased the PR to the latest master
  3. Refactored the code a bit and fixed some issues - mainly with android animations and keyframe parsing and passing the rotate values.
  4. Added 2 demo pages in the e2e/animations project. I've used them for testing and they can be used later for e2e tests. Here are the tests:
    1. 3d-rotatate - tests properties applied to view directly (in XML) + animations with code (view.animate({ rotate: {x: ..., y: ..., z: ...}}))
    2. css-animations -> 3d-rotate - tests properties applied to view with CSS + keyframe animations defined in CSS

What's Left

I'll try to summarize the work that remains to be done:

Feature Android IOS
setting rotateX/Y properties to views ❌ (1)
animations with code (view.animate()) ❌ (2)
keyframe animations ✅(3) ❌ (2)

(1) Noticed that only one of the rotate/rotateX/rotateY properties can be applied at a time. For example rotate is the strongest one: if it is set, the other two are ignored. Here is the relevant code that needs to be changed: https://github.com/NativeScript/NativeScript/pull/5950/files#diff-2775a71872c1630de8108bf058a1628eR275

(2) Animating the rotateX/rotateY properties does not work in IOS. I have left some TODO: comments in animations.ios.ts of the places of the code that will have to be extended.

(3) When creating keyframe animations with multiple transforms - we rely on creating and decomposing a matrix to calculate the final transformation. This is currently not handling 3d rotations. You can observe the bug if you define the keyframe like that.

I'll be very happy if you or @speigg or another contributor can find a way to move this forward and ultimately get this merged.

@cla-bot
Copy link

cla-bot bot commented Mar 5, 2019

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Hamdi Wanis.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot removed cla: yes docs needed Additional documentation on this issue/PR is needed e2e test needed in progress ♥ community PR labels Mar 5, 2019
@ghost ghost added the ♥ community PR label Mar 5, 2019
@cla-bot
Copy link

cla-bot bot commented Mar 6, 2019

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Hamdi Wanis.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@madsb
Copy link

madsb commented Mar 19, 2019

Any progress on this? Would be great to have!

@cla-bot
Copy link

cla-bot bot commented Mar 22, 2019

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Hamdi Wanis.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@ghost ghost added the ♥ community PR label Mar 22, 2019
@hamdiwanis
Copy link
Contributor Author

@madsb it looks like it's only 3 of us who like to have this feature :D

@cla-bot
Copy link

cla-bot bot commented Mar 22, 2019

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Hamdi Wanis.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@ghost ghost added the ♥ community PR label Mar 22, 2019
@rynop
Copy link
Contributor

rynop commented Oct 14, 2019

@hamdiwanis I'd like this feature as well. Can you update your gitconfig per the cla-bot so this can get by the CI?

@manoldonev
Copy link
Contributor

@rynop

@hamdiwanis I'd like this feature as well. Can you update your gitconfig per the cla-bot so this can get by the CI?

In case @hamdiwanis could not continue working on this PR, would you consider forking the work and taking over it in a new PR?
We'd be happy if you or another contributor can find a way to ultimately bring this feature to completion -- at this point I am not sure where there are any pending items from #5950 (comment) and #5950 (comment) besides the CLA.

@rynop
Copy link
Contributor

rynop commented Nov 21, 2019

@manoldonev sure I'll give it a stab next week. I won't have much time to dev on it but I can get it caught up to master and do some code cleanup.

@rynop
Copy link
Contributor

rynop commented Nov 25, 2019

Whew that took longer than I thought. @manoldonev I've done what you asked - now I need some help ;)

Please see #8136 for what I need help with.

@etabakov @madsb @vakrilov @speigg @hshristov

@vakrilov
Copy link
Contributor

closing in favor of #8136

@vakrilov vakrilov closed this Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants