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

No overlap when drawing rotate gizmo and draw start / end segments #11550

Merged
merged 2 commits into from
Aug 29, 2022

Conversation

guillaume-haerinck
Copy link
Contributor

Signed-off-by: guillaume-haerinck guillaume.haerinck@outlook.com

What does this PR do?

Fix for #11308

  1. Limit rotation circle drawing to 2Pi to prevent overlap (else the alpha of the color is increased)
  2. Draw start and end radius segments for better feedback

Note : In blender 2.8+, colors overlaps with further rotation. It stays visible as the radius segments have a bigger width, might be a good future change.

How was this PR tested?

Checked with all angles of the gizmo

Fix in action

Signed-off-by: guillaume-haerinck <guillaume.haerinck@outlook.com>
@guillaume-haerinck guillaume-haerinck requested a review from a team as a code owner August 25, 2022 15:45
@hultonha hultonha added the status/needs-ux-approval The UX SIG needs to approve the UI/UX design for this feature before it can be merged. label Aug 25, 2022
@hultonha
Copy link
Contributor

Added needs-ux-approval label - would be good to have them sign-off before accepting this (I did intentionally have the alpha increase initially as it does show the angle is still increasing (and Unity does this too) but I'm also happy with the change (the lines are a nice addition too! 🙂)

Right now this feedback is only added for the rotation manipulator used for entities but would be cool to add for the rotation manipulator used for PhysX colliders too if you're interested 🙂

Thanks very much for looking into this and for the contribution! 👍

(tagging @amzn-leenguy, @bhanuja-s, @yuyihsu and @rainbj for visibility)

Copy link
Contributor

@hultonha hultonha left a comment

Choose a reason for hiding this comment

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

Change looks great, I just have a few minor suggestions. It would just be good to get UX sign-off and then I can approve. Nice change thanks!

Signed-off-by: guillaume-haerinck <guillaume.haerinck@outlook.com>
@guillaume-haerinck
Copy link
Contributor Author

guillaume-haerinck commented Aug 25, 2022

Added needs-ux-approval label - would be good to have them sign-off before accepting this (I did intentionally have the alpha increase initially as it does show the angle is still increasing (and Unity does this too) but I'm also happy with the change (the lines are a nice addition too! 🙂)

Right now this feedback is only added for the rotation manipulator used for entities but would be cool to add for the rotation manipulator used for PhysX colliders too if you're interested 🙂

Thanks very much for looking into this and for the contribution! 👍

(tagging @amzn-leenguy, @bhanuja-s, @yuyihsu and @rainbj for visibility)

Good idea it would be nice to have it as well for the physX collider, I'll check this when I can 👍

@amzn-leenguy
Copy link

I love this update! I also like your forward thinking on the colour change on further rotations. Regardless, what i see in the video is great. Sign off from me (UX)

@amzn-leenguy amzn-leenguy requested review from amzn-leenguy and removed request for amzn-leenguy August 25, 2022 21:21
@amzn-leenguy
Copy link

Note: I can only review from UX Pov. I don't want to approve the code since it's not my field :)

@pollend
Copy link
Member

pollend commented Aug 26, 2022

that looks very nice, I like it :D.

The note about blender 2.8 doesn't seem that much harder. you divide and also take the modulus. the modulus is the reminder on the next rotation so you draw that section in with the darker color and the division is for the intensity of the overlap color. you need to render the overlapping slice and the remaining. I think you can easily do it in this change if you wanted to :?

@guillaume-haerinck
Copy link
Contributor Author

that looks very nice, I like it :D.

The note about blender 2.8 doesn't seem that much harder. you divide and also take the modulus. the modulus is the reminder on the next rotation so you draw that section in with the darker color and the division is for the intensity of the overlap color. you need to render the overlapping slice and the remaining. I think you can easily do it in this change if you wanted to :?

Thanks :) Well without my changes, the color will increase in intensity, the problem is that when it does, the radius segments become hard to see and it can looks like a bug. In my opinion the best solution would be - in the future - to increase the line width of every gizmos in order to enable the overlap (similar to Unreal, Blender, Godot, etc)

Example in blender :

In Blender

@hultonha
Copy link
Contributor

Agree we can always add more refinements in future but for now this is a definite improvement and would be great to get in, nice work @guillaume-haerinck! 🥳

Copy link
Contributor

@hultonha hultonha left a comment

Choose a reason for hiding this comment

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

LGTM! I'll kick off a build now 👍

@hultonha
Copy link
Contributor

Build is here - https://jenkins.build.o3de.org/job/O3DE/view/change-requests/job/PR-11550/1/ (FYI there are some flaky tests and some environmental flakiness right now just as a heads-up... we're working on improving this but it might take 2-3 runs to pass, if it fails for a spurious reason let me know and I'll follow-up 👍)

@cgalvan cgalvan merged commit 1f9c9c6 into o3de:development Aug 29, 2022
@guillaume-haerinck guillaume-haerinck deleted the fix-11308 branch August 29, 2022 15:38
@rainbj
Copy link

rainbj commented Aug 31, 2022

Hi, @guillaume-haerinck and @hultonha my only question would be about the base use case here. I could imagine that if this tooling was used to create shapes, then yes we would want to limit the number of rotations. But I can also imagine if this was used for something like base rotation, it could limit the user from collecting the specific rotational value that might be needed for a Script Canvas or a grouped object that changes over several rotations. As for the visual side of the function, I think it looks great! Looking forward to hearing more about this functioanility.

@hultonha
Copy link
Contributor

hultonha commented Sep 2, 2022

Hi @rainbj, this was purely a visual improvement to how the quadrant of the rotation (angular) manipulator filled-up, glad you like the changes! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/needs-ux-approval The UX SIG needs to approve the UI/UX design for this feature before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants