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.

Sign up for GitHub

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

Jump to bottom

[C] Clear OpenTK namespace #13767

Merged
merged 28 commits into from
Feb 14, 2022
Merged

[C] Clear OpenTK namespace #13767

merged 28 commits into from
Feb 14, 2022

Conversation

StephaneDelcroix
Copy link
Member

@StephaneDelcroix StephaneDelcroix commented Jan 18, 2022 •
edited
Loading

fixes #13160

  • remove unused types
  • use System.Numerics when possible
  • move own created types from OpenTK namespace to CoreGraphics
  • create missing types in CoreGraphics namespace

@rolfbjarne rolfbjarne added the run-dotnet-tests Run all the .NET tests label Jan 18, 2022
rolfbjarne
rolfbjarne requested changes Jan 18, 2022
View reviewed changes
Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

First pass done! Looks good, although I'd like to know the reason for the removals related to Matrix3/Matrix4.

Also: it doesn't look like the opentk submodule has to be bumped (the diff between those two hashes show no difference).

runtime/bindings-generator.cs Outdated Show resolved Hide resolved
src/OpenGL/OpenTK/Math/Quaternion.cs Outdated Show resolved Hide resolved
src/OpenGL/OpenTK/Math/Vector2.cs Outdated Show resolved Hide resolved
src/OpenGL/OpenTK/Math/Vector3.cs Outdated Show resolved Hide resolved
src/OpenGL/OpenTK/Math/Vector4.cs Outdated Show resolved Hide resolved
src/SceneKit/SCNVector4.cs Outdated Show resolved Hide resolved
src/Simd/QuaternionDouble.cs Outdated Show resolved Hide resolved
src/Simd/QuaternionDouble.cs Outdated Show resolved Hide resolved
src/glkit.cs Outdated Show resolved Hide resolved
src/spritekit.cs Outdated Show resolved Hide resolved
@mandel-macaque
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mandel-macaque
Copy link
Member

Building issues are unrelated in the PR, the checkout of the project failed on devops

@mandel-macaque
Copy link
Member

the CI just corrected me and well as rolf, we do not need to bump the submodule, that results in the following error when building on the CI:

fatal: remote error: upload-pack: not our ref 6fa80b477c7e26d1c95e8c23657cd23b34d6751e
fatal: Fetched in submodule path 'external/opentk', but it did not contain 6fa80b477c7e26d1c95e8c23657cd23b34d6751e. Direct fetching of that commit failed.

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view
@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view
@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view
@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view
@rolfbjarne

This comment was marked as outdated.

Sign in to view
@mandel-macaque
Copy link
Member

Current failure is:

SceneKit/SCNMatrix4_dotnet.cs(832,6): error CS1061: 'Quaternion' does not contain a definition for 'ToAxisAngle' and no accessible extension method 'ToAxisAngle' accepting a first argument of type 'Quaternion' could be found (are you missing a using directive or an assembly reference?)

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view
@rolfbjarne

This comment was marked as outdated.

Sign in to view
@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view
@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view
@StephaneDelcroix StephaneDelcroix force-pushed the sde_opentk branch 3 times, most recently from 2ff20e5 to 714f92b Compare January 26, 2022 09:32
@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view
@mandel-macaque
Copy link
Member

@StephaneDelcroix the failures in the older mac OS can be ignored, yet they show that the branch is far behind main since those were fixed. Can you please rebase with main (or merge)?

@rolfbjarne
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ [PR Build] Tests passed on Build. ✅

Tests passed on Build.

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API Current PR diff

ℹ️ API Diff (from PR only) (please review changes)

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Generator diff

ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

🎉 All 146 tests passed 🎉

Pipeline on Agent XAMBOT-1042.BigSur'
Merge 062f2ae into adff9df

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ [PR Build] Tests passed on Build. ✅

Tests passed on Build.

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API Current PR diff

ℹ️ API Diff (from PR only) (please review changes)

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Generator diff

ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

🎉 All 146 tests passed 🎉

Pipeline on Agent XAMBOT-1102.BigSur'
Merge 9cf6822 into eb281ae

@rolfbjarne
Merge remote-tracking branch 'origin/main' into HEAD
a242d13
@rolfbjarne
[cecil-tests] Add test to make sure we don't have any OpenTK types in…
b964c19 
… our platform assemblies.
@rolfbjarne
[src] Remove the OpenTK source code from the .NET build.
4f8b59f
@rolfbjarne
Bring back GLKReflectionMapEffect.Matrix now that we have a Matrix3 m…
32db187 
…atrix.
@rolfbjarne
Fix mapping of OpenTK.Matrix3 in SCNQuaternion.
75a6391
@rolfbjarne
[SceneKit] Add back quaternion constructor for .NET.
76440ad
@rolfbjarne
Add README explaining the different numerics/math types we have in .NET
6961956 
And how they map to legacy Xamarin types (when they do).
@rolfbjarne
Improve code comments.
6ca06ff
@rolfbjarne
Fix spaces / tabs
5f798a3
@rolfbjarne
Enable nullability.
54a083a
@rolfbjarne
Add missing padding field to NVector3i
b1f691c
@rolfbjarne
Make quaternion and vector API more like the System.Numerics versions.
e51909c
dalexsoto
dalexsoto approved these changes Feb 11, 2022
View reviewed changes
Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

Love the docs!

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API Current PR diff

ℹ️ API Diff (from PR only) (please review changes)

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Generator diff

ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

5 tests failed, 141 tests passed.

Failed tests

  • trimmode link/Mac Catalyst [dotnet]/Release [dotnet]: TimedOut (Execution timed out after 1200 seconds.
    No test log file was produced)
  • link all/iOS Unified 64-bits - simulator/Debug: Failed
  • link all/iOS Unified 64-bits - simulator/Release: Failed
  • monotouch-test/tvOS - simulator/Debug (LinkSdk) [dotnet]: Failed
  • MSBuild tests/Integration: Failed (Execution failed with exit code 2)

Pipeline on Agent XAMBOT-1100.BigSur'
Merge e51909c into 25cfcaa

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API Current PR diff

ℹ️ API Diff (from PR only) (please review changes)

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Generator diff

ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

3 tests failed, 143 tests passed.

Failed tests

  • monotouch-test/watchOS 32-bits - simulator/Debug (LinkSdk): TimedOut
  • monotouch-test/watchOS 32-bits - simulator/Debug (static registrar): Crashed
  • monotouch-test/watchOS 32-bits - simulator/Release (all optimizations): Crashed

Pipeline on Agent XAMBOT-1096.BigSur'
Merge 80ec1c5 into d9a0206

@rolfbjarne
Copy link
Member

Test failures are unrelated (https://github.com/xamarin/maccore/issues/581).

@rolfbjarne rolfbjarne merged commit f36606e into main Feb 14, 2022
@rolfbjarne rolfbjarne deleted the sde_opentk branch February 14, 2022 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalexsoto dalexsoto dalexsoto approved these changes

@rolfbjarne rolfbjarne rolfbjarne approved these changes

@mandel-macaque mandel-macaque Awaiting requested review from mandel-macaque mandel-macaque is a code owner

Assignees
No one assigned
Labels
run-dotnet-tests Run all the .NET tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

.NET: Clear the OpenTK namespace
5 participants
@StephaneDelcroix @mandel-macaque @vs-mobiletools-engineering-service2 @rolfbjarne @dalexsoto