Fixing a bug the causes FaceOrigin collections to face the wrong direction when collections are rotated (and some bonuses) TAKE 2 #2246
Conversation
…face the wrong direction when collecitons are rotated. Incidentally adds Radial collections. Previously collections with FaceOrigin selected as their Orientation Type which were not perfectly vertical would align their elements in crazy ways. This commit fixes this, but slightly changes the meaning of FaceOrigin so elements always face the origin, even if that means angling the element up or down to face the origin. This could be considered a breaking change, since all existing FaceOrigin object collections will change behavior. This is particularly true of Cylinder collections which are FaceOrigin orientation. They can be returned to the old behavior by switching to FaceCenterAxis, which keeps elements aligned with the collection's up vector. FaceCenterAxis also works well with spheres. In the process I also consolidated all the orientation switch statements into a single function that handles the orientation on all objection collections. Bringing these changes over also necessitated bringing in @paseb 's addition of Radial collections, that make cool fan-like things. Bonus feature! Co-Authored-By: Patrick Sebring <[email protected]>
getting master June18 dev into this fork
Update from upstream repo microsoft/MixedRealityToolkit-Unity@june18_dev
June18 dev
There was a problem hiding this comment.
Conditional approval, pending @paseb's response to @StephenHodgson's inquiries pasted in the PR body.
|
@StephenHodgson, I couldn't add directly to the comments but here my responses to the above comments. Radial mapping represents a radial menu so the grid source is not what is being used though there's no really need to make a new vector so assigning it directly is fine. Since this is a single relative axis rotation gimbal lock isn't an issue. If we were going to do any multi-axis rotations we should be using matrix transforms. |
|
Cool, so it looks like addressing the "update source vs new vector creation" concern is all that's needed here for approval. |
|
|
||
| /// <summary> | ||
| /// Margin between objects horizontally. | ||
| /// </summary> |
There was a problem hiding this comment.
Summary tags should be above the public properties not the attributes for the private fields
| centerAxis = Vector3.Project(node.transform.position - this.transform.position, this.transform.up); | ||
| pointOnAxisNearestNode = this.transform.position + centerAxis; | ||
| node.transform.rotation = Quaternion.LookRotation(node.transform.position - pointOnAxisNearestNode, this.transform.up); | ||
|
|
There was a problem hiding this comment.
a couple of extra lines before breaks.
| { | ||
| Radius = radius >= 0 ? Radius : radius; | ||
|
|
||
| Vector3 newPos = new Vector3(0f, 0f, (Radius/Rows) * row); |
There was a problem hiding this comment.
can we just assign the vector instead of creating a new one?
Updating based on feedback
Reverting some breaking changes
I dug myself into a githole in that other branch. I gave up trying to revert my reverts of merges of merges and just put my changes in a new branch. Sorry for making you check it twice. It should have all the changes I made in response to previous review feedback. The only bit we should be waiting for is @paseb to weigh in on the RadialMapping() question. I'll poke him in person tomorrow.
This is a pull request for the completion of my proposal:
#2227