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

[Dev→master] Billboard updates #1856

Merged
merged 2 commits into from
Mar 22, 2018

Conversation

keveleigh
Copy link
Contributor

Port of #1817.

Overview

Added support for all combinations of axes. I rearranged the enum to preserve existing behavior (the previous Free was actually only the X and Y axes).

Changes

@keveleigh keveleigh self-assigned this Mar 21, 2018
david-c-kline
david-c-kline previously approved these changes Mar 21, 2018
Copy link

@david-c-kline david-c-kline left a comment

Choose a reason for hiding this comment

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

Approved, with minor style guidelines suggestions

@@ -22,7 +29,7 @@ public class Billboard : MonoBehaviour
/// The axis about which the object will rotate.
/// </summary>
[Tooltip("Specifies the axis about which the object will rotate.")]
public PivotAxis PivotAxis = PivotAxis.Free;
public PivotAxis PivotAxis = PivotAxis.XY;

Choose a reason for hiding this comment

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

Let's follow the new guidelines wrt fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Updated both your comments.

@@ -22,7 +29,7 @@ public class Billboard : MonoBehaviour
/// The axis about which the object will rotate.
/// </summary>
[Tooltip("Specifies the axis about which the object will rotate.")]
public PivotAxis PivotAxis = PivotAxis.Free;
public PivotAxis PivotAxis = PivotAxis.XY;

[Tooltip("Specifies the target we will orient to. If no Target is specified the main camera will be used.")]

Choose a reason for hiding this comment

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

Can we get a summary tag here?

Copy link
Contributor

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

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

I'm sure @SimonDarksideJ might have something to say about that Enum, but I don't mind 😛

@StephenHodgson StephenHodgson merged commit 019d854 into microsoft:Patch4_Dev Mar 22, 2018
@keveleigh keveleigh deleted the BillboardUpdates branch March 24, 2018 02:51
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.

3 participants