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

Proposal: Fixing a bug the causes FaceOrigin collections to face the wrong direction when collections are rotated. #2227

Closed
Ecnassianer opened this issue May 31, 2018 · 4 comments

Comments

@Ecnassianer
Copy link
Contributor

Overview

Previously collections with FaceOrigin selected as their Orientation Type which were not perfectly vertical (i.e, transform.up == Vector3.up) would align their elements in crazy ways. I fixed this for my team's project and am I proposing pulling similar changes into the MRTK June release.

Vertical sphere:
image

Sphere rotated 90 degrees:
image

ಠ_ಠ

Cylinders also have a strange layout when rotated:
image

I'm proposing consolidating all the per-shape orientation methods into a single method that behaves uniformly regardless of shape type or orientation. This means the "FaceOrigin" Orientation Type will always face collection elements toward the origin regardless of surface type. This is likely an undesirable Orientation Type for vertically aligned Cylinders which previously relied on FaceOrigin to face each element toward their central axis, rather than toward their origin (I think we can assume no one was using the odd rotated cylinder layout). IMO, this was incorrect behavior for a "FaceOrigin" type, even though a "correct" behavior for FaceOrigin on a Cylinder is probably not very desirable (it creates a weird flattened sphere where the higher or lower you are on the cylinder the more angled down or up you are, even though all elements are vertically aligned).

To restore the previously desirable behavior of Cylinder collections, I propose adding a new Orientation Type called "FaceCenterAxis". On Cylinders this restores the old behavior. On spheres this creates a new potentially useful arrangement:
image

The same is true of FaceOrigin Plane collections, however the old behavior of FaceOrigin for planes is exactly the same as the FaceForward Orientation Type, so no new Orientation Type is required.

This should be considered a breaking change, since all Cylinder and Plane collections that want the old behavior would have to be switched to use the FaceCenterAxis or FaceForward orientations. (They cannot be updated in place, since FaceOrigin needs to remain the same in the enum so spheres don't break).

In my team's project, I have built on top of @paseb 's addition of Radial collections, that make cool fan-like things, as well as two new orientation types FaceParentDown, and FaceParentUp. This easiest way to do this pull request would be to bring those features along as well. They are purely additive and would be very low risk.

Expected Behavior

FaceOrigin always faces the origin's collection, regardless of its shape.
Collections can be rotated and their element layouts rotate with them.

Actual Behavior

Crazytown.

Steps to reproduce

Rotate a sphere collection and Update its layout.

@Ecnassianer
Copy link
Contributor Author

Here's a quick and dirty integration of my proposed solution. If this proposal is approved, I'll add tests/examples before putting in a pull request:

Ecnassianer@9454011

@david-c-kline
Copy link

@Ecnassianer, please feel free to submit a PR!

@StephenHodgson
Copy link
Contributor

StephenHodgson commented Jun 1, 2018

@Ecnassianer looks great! Yes, please submit a PR when you can. It'll be a great addition.

Actual Behavior
Crazytown.

I laughed way too hard at this.

@Ecnassianer
Copy link
Contributor Author

I updated the example map, I switched the primitives collection to show off the FaceCenterAxis orientation on a sphere collection (every shape keeps a vertical orientation):

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants