[spritekit] Update for Xcode 9 beta 1, 2 & 3 #2331

VincentDondain · 2017-07-17T19:08:04Z

Added Quaternion support to XI runtime (simd_quatf).

- Added Quaternion support to XI runtime (simd_quatf).

monojenkins · 2017-07-17T20:27:42Z

Build failure

dalexsoto

I am not sure if changing parameter names is considered a breaking change (question for @spouliot) but if we allow it, we should also include the unit of measure in the new parameter name

dalexsoto · 2017-07-17T20:17:56Z

Make.config

@@ -60,7 +60,7 @@ MIN_MONO_URL=https://bosstoragemirror.blob.core.windows.net/wrench/mono-2017-04/
 # Minimum Visual Studio version
 MIN_VISUAL_STUDIO_URL=https://bosstoragemirror.blob.core.windows.net/wrench/monodevelop-lion-dogfood-vNext/8f/8f1c13cb983138ee63bd53e09908ea5e737988cd/VisualStudioForMac-Preview-7.0.0.2728.dmg
 MIN_VISUAL_STUDIO_VERSION=7.0.0.2728
-MAX_VISUAL_STUDIO_VERSION=7.2.99
+MAX_VISUAL_STUDIO_VERSION=7.3.99


Why is this required? I think we should keep the same as 15.3 unless we require this

The message says that we can bump whenever we test with a more recent version and it works (:

Yeah, I completely agree when you are not in a release branch but I think that message is more intended for master

It's already not the same version as the d15.3 branch, I really think it doesn't matter: https://github.com/xamarin/xamarin-macios/blob/d15-3/Make.config#L63

Yeah, I think you changed that in the first Xcode9 bump 24cd73a#diff-f682ea58f8badb13b9f8ed17cda4deb7R63 but yeah I think it does not matter much at this point unless we really want to be close to dev 15.3

The change itself is fine, but should go in a different PR, it has nothing to do with sprite kit 😄

dalexsoto · 2017-07-17T20:20:44Z

src/spritekit.cs

@@ -1954,28 +1991,28 @@ partial interface SKAction : NSCoding, NSCopying {

 		// These are in a category
 		[Static, Export ("moveByX:y:duration:")]
-		SKAction MoveBy (nfloat deltaX, nfloat deltaY, double sec);
+		SKAction MoveBy (nfloat deltaX, nfloat deltaY, double duration);


@spouliot is this a breaking change? I am in favor of the change if we can allow it but @VincentDondain I would use durationInSeconds instead if seconds is the unit of measure of duration

Mmh API diff considers this a breaking change. I don't really understand why it would be one...

Because you can do

var action = MoveBy (deltaX: 1, deltaY: 2, sec: 100);

If anyone had the above code you will break them

But I still consider this is a good one, it really helps to better understand the API and even more if we add the measurement unit to the parameter name

Aaaaaah you're right, I didn't think about that, I never name anything explicitly in C# :P

It's a breakage change and it's less clear than (already less than perfect) sec, please keep the existing name.

dalexsoto · 2017-07-17T20:23:35Z

src/spritekit.cs


 		[Static, Export ("moveBy:duration:")]
 		SKAction MoveBy (CGVector delta, double duration);

 		[Static, Export ("moveTo:duration:")]
-		SKAction MoveTo (CGPoint location, double sec);
+		SKAction MoveTo (CGPoint location, double duration);


same as above

dalexsoto · 2017-07-17T20:23:41Z

src/spritekit.cs


 		[Static, Export ("moveToX:duration:")]
-		SKAction MoveToX (nfloat x, double sec);
+		SKAction MoveToX (nfloat x, double duration);


same as above

dalexsoto · 2017-07-17T20:23:46Z

src/spritekit.cs


 		[Static, Export ("moveToY:duration:")]
-		SKAction MoveToY (nfloat y, double sec);
+		SKAction MoveToY (nfloat y, double duration);


same as above

dalexsoto · 2017-07-17T20:26:55Z

src/spritekit.cs


 		[iOS (8,0), Mac(10,10)]
 		[Static, Export ("reachTo:rootNode:velocity:")]
 		SKAction ReachTo (CGPoint position, SKNode rootNode, nfloat velocity);

 		[iOS (8,0), Mac(10,10)]
 		[Static, Export ("reachToNode:rootNode:duration:")]
-		SKAction ReachToNode (SKNode node, SKNode rootNode, double sec);
+		SKAction ReachToNode (SKNode node, SKNode rootNode, double duration);


same as above

dalexsoto · 2017-07-17T20:26:58Z

src/spritekit.cs


 		[iOS (8,0), Mac(10,10)]
 		[Static, Export ("reachToNode:rootNode:velocity:")]
 		SKAction ReachToNode (SKNode node, SKNode rootNode, nfloat velocity);

 		[iOS (8,0), Mac (10,10)] // this method is missing the NS_AVAILABLE macro, but it shows up in the 10.10 sdk, but not the 10.9 sdk.
 		[Static, Export ("strengthTo:duration:")]
-		SKAction StrengthTo (float /* float, not CGFloat */ strength, double sec);
+		SKAction StrengthTo (float /* float, not CGFloat */ strength, double duration);


same as above

dalexsoto · 2017-07-17T20:27:03Z

src/spritekit.cs


 		[iOS (8,0), Mac (10,10)] // this method is missing the NS_AVAILABLE macro, but it shows up in the 10.10 sdk, but not the 10.9 sdk.
 		[Static, Export ("strengthBy:duration:")]
-		SKAction StrengthBy (float /* float, not CGFloat */ strength, double sec);
+		SKAction StrengthBy (float /* float, not CGFloat */ strength, double duration);


same as above

dalexsoto · 2017-07-17T20:27:07Z

src/spritekit.cs

@@ -2150,7 +2187,7 @@ partial interface SKAction : NSCoding, NSCopying {
 		[iOS (8,0), Mac(10,10)]
 		[Static]
 		[Export ("falloffTo:duration:")]
-		SKAction FalloffTo (float falloff, double sec);
+		SKAction FalloffTo (float falloff, double duration);


same as above

dalexsoto · 2017-07-17T20:31:37Z

src/spritekit.cs

+		void Render (CGRect viewport, IMTLRenderCommandEncoder renderCommandEncoder, MTLRenderPassDescriptor renderPassDescriptor, IMTLCommandQueue commandQueue);
+
+		[Export ("updateAtTime:")]
+		void UpdateAtTime (double currentTime);


Maybe just Update?

Agreed. 👍

spouliot

Please do not mix new bindings with other changes. If needed they should be in separate PR.

Also make sure the tests are run on devices before committing (simd stuff can behave differently).

spouliot · 2017-07-18T01:04:58Z

runtime/bindings-generator.cs

+					Comment = " // void func (Quaternion)",
+					Prefix = "simd__",
+					Variants = Variants.NonStret,
+					Parameters = new ParameterData[] {


minor: space before []

👍 Copied it from code above :P

spouliot · 2017-07-18T01:08:55Z

src/spritekit.cs

@@ -1954,28 +1991,28 @@ partial interface SKAction : NSCoding, NSCopying {

 		// These are in a category
 		[Static, Export ("moveByX:y:duration:")]
-		SKAction MoveBy (nfloat deltaX, nfloat deltaY, double sec);
+		SKAction MoveBy (nfloat deltaX, nfloat deltaY, double duration);


It's a breakage change and it's less clear than (already less than perfect) sec, please keep the existing name.

spouliot · 2017-07-18T01:13:20Z

tests/monotouch-test/SpriteKit/SKTransformNodeTest.cs

+			Vector3 V3;
+
+			using (var obj = new SKTransformNode ())
+			{


minor: should be on the previous line

spouliot · 2017-07-18T01:13:27Z

tests/monotouch-test/SpriteKit/SKTransformNodeTest.cs

+		public void RotationMatrix ()
+		{
+			using (var obj = new SKTransformNode ())
+			{


spouliot · 2017-07-18T01:14:07Z

tests/monotouch-test/SpriteKit/SKTransformNodeTest.cs

+			Quaternion Q;
+
+			using (var obj = new SKTransformNode ())
+			{


same (the IDE should set it up properly, double check your settings and file a bug if set to mono style).

rolfbjarne · 2017-07-18T12:03:59Z

Make.config

@@ -60,7 +60,7 @@ MIN_MONO_URL=https://bosstoragemirror.blob.core.windows.net/wrench/mono-2017-04/
 # Minimum Visual Studio version
 MIN_VISUAL_STUDIO_URL=https://bosstoragemirror.blob.core.windows.net/wrench/monodevelop-lion-dogfood-vNext/8f/8f1c13cb983138ee63bd53e09908ea5e737988cd/VisualStudioForMac-Preview-7.0.0.2728.dmg
 MIN_VISUAL_STUDIO_VERSION=7.0.0.2728
-MAX_VISUAL_STUDIO_VERSION=7.2.99
+MAX_VISUAL_STUDIO_VERSION=7.3.99


The change itself is fine, but should go in a different PR, it has nothing to do with sprite kit 😄

rolfbjarne · 2017-07-18T12:07:15Z

tests/monotouch-test/SpriteKit/SKTransformNodeTest.cs

+		public void VersionCheck ()
+		{
+			if (!TestRuntime.CheckXcodeVersion (9,0))
+				Assert.Inconclusive ("Requires Xcode9+");


minor (for next time): there's a one-liner for this:

TestRuntime.AssertXcodeVersion (9,0);

rolfbjarne · 2017-07-18T12:08:43Z

tests/monotouch-test/SpriteKit/SKTransformNodeTest.cs

+				Asserts.AreEqual (Vector3.Zero, obj.EulerAngles, "1 EulerAngles");
+				V3 = new Vector3 (1, 2, 3);
+				obj.EulerAngles = V3;
+				Assert.AreEqual (-2.14159298f, obj.EulerAngles.X, "#x1");


You're setting a vector of [1,2,3] and getting something back that looks like random numbers. Are you sure the test isn't finding a bug somewhere?

Same values as native Swift example.

rolfbjarne · 2017-07-18T12:10:18Z

tests/monotouch-test/SpriteKit/SKTransformNodeTest.cs

+			using (var obj = new SKTransformNode ())
+			{
+				obj.RotationMatrix = Matrix3.Zero;
+				Asserts.AreEqual (Matrix3.Identity, obj.RotationMatrix, "RotationMatrix");


Also: setting Matrix3.Zero, and getting back Matrix3.Identity. This doesn't look random, but it still looks strange (so I'd test with Xcode, and added a comment about it).

Tested with Xcode already (found that weird too), same behavior. I'll add a comment that it does the same thing with native code.

monojenkins · 2017-07-18T16:05:56Z

Build failure

monojenkins · 2017-07-18T16:10:56Z

Build failure

monojenkins · 2017-07-18T17:29:27Z

Build failure

VincentDondain · 2017-07-18T18:00:51Z

introspection and monotouch tests ran on device, both pass.

dalexsoto

Minor stuff, but 👍 once fixed

dalexsoto · 2017-07-18T18:05:57Z

src/spritekit.cs

+	[BaseType (typeof(NSObject))]
+	[DisableDefaultCtor]
+	interface SKRenderer
+	{


Minor: Brace should be on the above line

dalexsoto · 2017-07-18T18:07:11Z

src/spritekit.cs

+	[TV (11,0), Watch (4,0), Mac (13,0), iOS (11,0)]
+	[BaseType (typeof(SKNode))]
+	interface SKTransformNode
+	{


Minor: Brace should be on the above line

monojenkins · 2017-07-18T20:59:01Z

Build failure

VincentDondain · 2017-07-18T21:59:41Z

Unrelated test failure: https://bugzilla.xamarin.com/show_bug.cgi?id=55719

[spritekit] Update for Xcode 9 beta 1, 2 & 3

7295a4a

- Added Quaternion support to XI runtime (simd_quatf).

dnfclas added the cla-already-signed label Jul 17, 2017

VincentDondain requested review from spouliot, rolfbjarne and dalexsoto July 17, 2017 19:08

dalexsoto requested changes Jul 17, 2017

View reviewed changes

spouliot suggested changes Jul 18, 2017

View reviewed changes

rolfbjarne requested changes Jul 18, 2017

View reviewed changes

VincentDondain added 2 commits July 18, 2017 11:59

[spritekit] Fixes based on comments

1af8b71

[spritekit] Fixes based on comments (2)

07df2b2

[spritekit] Fixes based on comments (3)

3e4679b

VincentDondain requested review from dalexsoto, rolfbjarne and spouliot July 18, 2017 18:00

[spritekit] Don't execute on watchOS

19ae06f

rolfbjarne approved these changes Jul 18, 2017

View reviewed changes

dalexsoto approved these changes Jul 18, 2017

View reviewed changes

[spritekit] Fix braces new line

e0085f2

chamons approved these changes Jul 19, 2017

View reviewed changes

spouliot approved these changes Jul 19, 2017

View reviewed changes

VincentDondain merged commit 49b020e into xamarin:xcode9 Jul 19, 2017

VincentDondain deleted the spritekit-b3 branch July 19, 2017 20:19

[spritekit] Update for Xcode 9 beta 1, 2 & 3 #2331

[spritekit] Update for Xcode 9 beta 1, 2 & 3 #2331

Conversation

VincentDondain commented Jul 17, 2017

monojenkins commented Jul 17, 2017

dalexsoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spouliot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jul 18, 2017

monojenkins commented Jul 18, 2017

monojenkins commented Jul 18, 2017

VincentDondain commented Jul 18, 2017

dalexsoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jul 18, 2017

VincentDondain commented Jul 18, 2017