Add XBOX support. #183

KrzaQ · 2023-04-05T06:33:54Z

Please note: the proprietary XBOX dll is not included in this commit and is only available to verified customers.

konraddysput · 2023-04-05T09:36:55Z

Editor/BacktraceConfigurationEditor.cs

                    if (showNativeCrashesSettings)
                    {
-#if UNITY_STANDALONE_WIN
+#if UNITY_STANDALONE_WIN || UNITY_GAMECORE_XBOXSERIES


do you use it anywhere?

konraddysput · 2023-04-05T09:49:43Z

Runtime/Model/Database/BacktraceDatabaseAttachmentManager.cs

 #elif UNITY_STANDALONE_OSX
                    string.Format("~/Library/Logs/{0}/{1}/Player.log", Application.companyName, Application.productName);
-#elif UNITY_STANDALONE_WIN
+#elif UNITY_STANDALONE_WIN || UNITY_GAMECORE_XBOXSERIES


Are you sure Unity logs are available in this path on Xbox?

konraddysput · 2023-04-05T09:51:08Z

Runtime/Native/XBOX/NativeClient.cs

+
+    internal class NativeClient : NativeClientBase, INativeClient
+    {
+        [DllImport("breakpad_xbox_simplified_abi_mt.dll")]


Are you sure breakpad_xbox_simplified name is a good idea for a library that our users will add to the code? Wouldn't it be better if we name it backtrace-xbox or backtrace-unity-xbox?

I am not. It's "simplified ABI" since the dll only exposes a few basic functions, C-style (undecorated ABI), which is different from the full Breakpad library.

I am open to changing the name.

we can hide what is this because I'm not sure if this is important to share that this is c-style library. I think it's not from the end user perspective. How about: backtrace-unity-xbox ?

This library could be used from other game engines as well. I would rather not package the same library several times just by changing the name.

konraddysput · 2023-04-05T09:51:48Z

Runtime/Native/XBOX/NativeClient.cs

+    internal class NativeClient : NativeClientBase, INativeClient
+    {
+        [DllImport("breakpad_xbox_simplified_abi_mt.dll")]
+        private static extern bool BacktraceCrash();


Does it make sense to start every method in "Backtrace" library with prefix "Backtrace"? I think it is obvious enough if we call it "Crash"

The DLL exposes undecorated symbols, C-style. It's idiomatic to namespace them

yeah but the dll is alerady backtrace dll

That's not the issue. C does not have namespaces. If we have a method Initialize() in our DLL, and another DLL exposes the same method, they will clash.

but you call the dll directly based on the dll name

If that's a hard requirement for you, I can do it. But IMO it's better to offer one generic library that we could reuse in other projects.

it's still can be generic, right? We can share the library with a note it's an abi_mt library. We don't need to show in our code we use breakpad or this is the "simplify" the library. If we switch to other crash reporter for any reason someone will have more manual work to do. Can we just rename it to backtrace-xbox?

I'm of two minds here. We actually offer multiple versions of Breakpad because some customers use mt/md libraries.

konraddysput · 2023-04-05T09:52:08Z

Runtime/Native/XBOX/NativeClient.cs

+        );
+
+        [DllImport("breakpad_xbox_simplified_abi_mt.dll")]
+        private static extern bool BacktraceBreakpadInitialized();


do you use it anywhere?

Runtime/Native/XBOX/NativeClient.cs

konraddysput · 2023-04-05T09:59:51Z

Runtime/Native/XBOX/NativeClient.cs

+
+            if (!CaptureNativeCrashes)
+            {
+                Debug.LogWarning("Backtrace native integration status: Cannot initialize Crashpad client");


Crashpad client is not a good name - can we rename it to xbox native integration or something?

Copy-paste error, absolutely

konraddysput · 2023-04-05T10:00:05Z

Runtime/Native/XBOX/NativeClient.cs

+
+        public void HandleAnr()
+        {
+            throw new NotImplementedException();


is this expected?

We don't support ANR for now.

if we don't - why do we respect ANR behavior in the constructor by filtering error types? We need to be sure the use cannot setup it.

konraddysput · 2023-04-05T10:00:10Z

Runtime/Native/XBOX/NativeClient.cs

+
+        public void GetAttributes(IDictionary<string, string> attributes)
+        {
+            throw new NotImplementedException();


Is this expected?

I don't see the value in implementing this method, but can do so. But it'll require additional implementation on the C-side and the hassle of marshalling the data through C-strings, for very dubious (IMO) gain.

yeah but leaving not implemented exception is not something that our users want to see

konraddysput · 2023-04-05T10:00:17Z

Runtime/Native/XBOX/NativeClient.cs

+
+        public bool OnOOM()
+        {
+            throw new NotImplementedException();


Is this expected?

We don't support OOM for now.

Yeah but not implemented exception is not something that our users want to see - they expect to have the same behavior like in any other our integration

konraddysput · 2023-04-13T15:31:27Z

Editor/BacktraceConfigurationEditor.cs

                    }

-#if UNITY_STANDALONE_LINUX || UNITY_STANDALONE_OSX || UNITY_STANDALONE_WIN
+#if UNITY_STANDALONE_LINUX || UNITY_STANDALONE_OSX || UNITY_STANDALONE_WIN || UNITY_GAMECORE_XBOXSERIES


can we remove it?

konraddysput · 2023-04-13T15:32:19Z

Editor/BacktraceDatabaseConfigurationEditor.cs

            }

-#if UNITY_STANDALONE_WIN
+#if UNITY_STANDALONE_WIN || UNITY_GAMECORE_XBOXSERIES


can we remove it?

konraddysput · 2023-04-13T15:32:51Z

Runtime/Model/Database/BacktraceDatabaseSettings.cs

            get
            {
-#if UNITY_STANDALONE_LINUX || UNITY_STANDALONE_OSX || UNITY_STANDALONE_WIN
+#if UNITY_STANDALONE_LINUX || UNITY_STANDALONE_OSX || UNITY_STANDALONE_WIN || UNITY_GAMECORE_XBOXSERIES


can we remove it?

konraddysput · 2023-04-13T15:32:58Z

Runtime/Model/Database/BacktraceDatabaseSettings.cs

            {

-#if UNITY_STANDALONE_WIN
+#if UNITY_STANDALONE_WIN || UNITY_GAMECORE_XBOXSERIES


can we remove it?

Add XBOX support.

7545dff

KrzaQ requested a review from konraddysput April 5, 2023 06:33

KrzaQ self-assigned this Apr 5, 2023

konraddysput requested changes Apr 5, 2023

View reviewed changes

Paweł Zakrzewski added 2 commits April 11, 2023 23:32

Respond to comments from Konrad

cf0e544

Respond to comments, should be all ok by now.

875b789

konraddysput approved these changes Apr 14, 2023

View reviewed changes

konraddysput and others added 3 commits April 18, 2023 16:44

Catch possible exceptions throwed by the native client

ca6d004

Update the changelog.

fe89983

Version 3.8.0: Version update

95ff539

konraddysput merged commit e3ec70d into master Apr 18, 2023

konraddysput deleted the kq-add-xbox branch September 21, 2023 10:16

Add XBOX support. #183

Add XBOX support. #183

Uh oh!

Conversation

KrzaQ commented Apr 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konraddysput Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

konraddysput Apr 6, 2023 •

edited

Loading