Skip to content

[scenekit] SCNPhysicsShape's Create should take a SCNMatrix4 array, not SCNVector3 - #4040

Merged
VincentDondain merged 1 commit into
dotnet:masterfrom
VincentDondain:issue-4030
May 7, 2018
Merged

[scenekit] SCNPhysicsShape's Create should take a SCNMatrix4 array, not SCNVector3#4040
VincentDondain merged 1 commit into
dotnet:masterfrom
VincentDondain:issue-4030

Conversation

@VincentDondain

Copy link
Copy Markdown
Contributor

…ot SCNVector3

- Fixes dotnet#4030: SCNPhysicsShape.Create transforms param is bound incorrectly
(dotnet#4030)
@VincentDondain
VincentDondain requested a review from dalexsoto as a code owner May 5, 2018 00:25
@monojenkins

Copy link
Copy Markdown
Contributor

Build failure
Build comment file:

Provisioning succeeded
Build succeeded
API Diff (from stable)
API Diff (from PR only)
Generator Diff
🔥 Test run failed 🔥

Test results

1 tests failed, 0 tests skipped, 79 tests passed.

Failed tests

  • monotouch-test/watchOS - simulator/Debug: Failed

@VincentDondain

Copy link
Copy Markdown
Contributor Author

Known issue: MessageHandlerTest.DnsFailure https://github.com/xamarin/maccore/issues/701

@VincentDondain
VincentDondain merged commit 1d94887 into dotnet:master May 7, 2018
@VincentDondain
VincentDondain deleted the issue-4030 branch May 7, 2018 20:12
@rolfbjarne

Copy link
Copy Markdown
Member

@VincentDondain this introduces a breaking change:

Type Changed: SceneKit.SCNPhysicsShape

Removed method:
-public static SCNPhysicsShape Create (SCNPhysicsShape[] shapes, SCNVector3[] transforms);

Added method:
+public static SCNPhysicsShape Create (SCNPhysicsShape[] shapes, SCNMatrix4[] transforms);

@VincentDondain

Copy link
Copy Markdown
Contributor Author

@rolfbjarne fixed in #4126, I missed the !

However we should fix the report for API diff #4040 (comment), it should have failed.

@rolfbjarne

Copy link
Copy Markdown
Member

However we should fix the report for API diff #4040 (comment), it should have failed.

That's not as easy to automate as it sounds 😒
We could show that there were breaking changes (but not automatically fail the build, because there might be changes that are acceptable, or the tool detects incorrectly).

@VincentDondain

Copy link
Copy Markdown
Contributor Author

@rolfbjarne shouldn't "API Diff (from PR only)" fail the build? If we want we can always justify and merge anyway. Subsequent PRs shouldn't fail because the breaking change would then end up in "API Diff (from stable)" and we wouldn't make this one fail the build.

In any case I think "API Diff (from PR only)" should at least show 🔥 if there breaking changes.

dalexsoto pushed a commit to dalexsoto/xamarin-macios that referenced this pull request Jul 11, 2018
…ot SCNVector3 (dotnet#4040)

- Fixes dotnet#4030: SCNPhysicsShape.Create transforms param is bound incorrectly
(dotnet#4030)
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.

4 participants