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

[2017.2.1.4] Fix building on Unity 5.6 and 2017.1 #1963

Merged
merged 5 commits into from
Apr 17, 2018
Merged

[2017.2.1.4] Fix building on Unity 5.6 and 2017.1 #1963

merged 5 commits into from
Apr 17, 2018

Conversation

david-c-kline
Copy link

@david-c-kline david-c-kline commented Apr 16, 2018

There have been a number of instances of APIs used in recent master and Patch4_Dev branch checkins that are not supported on Unity 5.6 and 2017.1. Add #if version checks around them, and where appropriate use a preprocessor warning directive to alert users to the functionality limitations.

@david-c-kline
Copy link
Author

Add fix for #1964.

Matrix4x4 m = Matrix4x4.Rotate(objectToBound.transform.rotation);
for (int i = 0; i < bounds.Count; ++i)
{
bounds[i] = m.MultiplyPoint(bounds[i]);
bounds[i] += boxInstance.TargetBoundsCenter;
}

#else
#warning "GetBounds() using rotation is not supported on this version of Unity. Recommend updating to 2017.1 or newer."
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should warn people about this, in this way.

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough. I can remove those.

@@ -260,6 +261,9 @@ private void ApplyRotation(Quaternion currentHandOrientation)
transformToAffect.localRotation = initialRotation;
transformToAffect.Rotate(axis, angle * 5.0f);
}
#else
#warning "ApplyRotation(Quaternion currentHandOrientation) is not supported on this version of Unity. Recommend updating to 2017.1 or newer."
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should warn people about this, in this way.

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.

Not sure about leaving warnings this way for people.

@david-c-kline david-c-kline dismissed StephenHodgson’s stale review April 17, 2018 16:44

Updated based on feedback

@@ -9,6 +9,8 @@ namespace HoloToolkit.Unity
{
public static class SpriteAtlasExtensions
{
// SpriteAtlast requires Unity 2017.1 or later
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo: SpriteAtlast

@david-c-kline
Copy link
Author

Removed the comment that contained the spelling error... wasn't really helpful anyhow :)

@david-c-kline david-c-kline merged commit 7b9053f into microsoft:Patch4_Dev Apr 17, 2018
@david-c-kline david-c-kline deleted the fix1958 branch April 17, 2018 22:11
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