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

Add AttachScreenshot option #284

Closed
bruno-garcia opened this issue Aug 20, 2021 · 32 comments · Fixed by #670
Closed

Add AttachScreenshot option #284

bruno-garcia opened this issue Aug 20, 2021 · 32 comments · Fixed by #670
Assignees
Labels
Feature New feature or request

Comments

@bruno-garcia
Copy link
Member

When the option is set to true, events will include a screenshot of the game.

This feature is being added to Xamarin:

https://github.com/getsentry/sentry-xamarin/search?q=screenshot

https://github.com/getsentry/sentry-xamarin/blob/411be60d616789ad81ec13e2e50c7c2a777b4c67/Src/Sentry.Xamarin/Internals/Device/Screenshot/ScreenshotEventProcessor.droid.ios.cs

@bruno-garcia bruno-garcia added the Feature New feature or request label Aug 20, 2021
@bitsandfoxes
Copy link
Contributor

Gotchas to keep in mind:

  • Record at end of frame i.e. a coroutine yield return new WaitForEndOfFrame() and not in the middle of rendering
  • ScreenCapture.CaptureScreenshotAsTexture() returns a Texture2D that can be encoded to .png, .jpg, .tga byte[]

@bruno-garcia
Copy link
Member Author

@oddgames-david
Copy link

Gotchas to keep in mind:

  • Record at end of frame i.e. a coroutine yield return new WaitForEndOfFrame() and not in the middle of rendering
  • ScreenCapture.CaptureScreenshotAsTexture() returns a Texture2D that can be encoded to .png, .jpg, .tga byte[]

Also:

  • Native screenshot would be better, ensures always receiving a screenshot when the exception is threaded or from a java/objective-c error (so long as it didn't crash)
  • Need to resize it before compressing to jpg, make the max height and compress amount a setting

This is my current implementation but most of the time I'd love to have screenshots for native exceptions, they are harder to diagnose.

    public class ScreenshotAttachment : IAttachmentContent
    {

        public Stream GetStream()
        {

            if (!UnityThread.IsRunningOnUnityThread)
                return new MemoryStream();

            float ratio = (float)Screen.width / (float)Screen.height;

            int targetHeight = Mathf.Min(Screen.height, 360);
            int targetWidth = Mathf.RoundToInt((float)targetHeight * ratio);

            RenderTexture screenshot = RenderTexture.GetTemporary(Screen.width, Screen.height);

            ScreenCapture.CaptureScreenshotIntoRenderTexture(screenshot);
            
            // Create a render texture to render into
            RenderTexture screenshotResize = RenderTexture.GetTemporary(targetWidth, targetHeight, 0);

            Graphics.Blit(screenshot, screenshotResize);

            RenderTexture.ReleaseTemporary(screenshot);

            RenderTexture previousActiveRT = RenderTexture.active;
            RenderTexture.active = screenshotResize;

            // Create a texture & read data from the active RenderTexture
            Texture2D result = new Texture2D(targetWidth, targetHeight) { name = "Screenshot" };
            result.ReadPixels(new Rect(0, 0, targetWidth, targetHeight), 0, 0);
            result.Apply();

            // Reset to initial state
            RenderTexture.active = previousActiveRT;

            RenderTexture.ReleaseTemporary(screenshotResize);

            var stream = new MemoryStream(result.EncodeToJPG(80));

            result.SafeDestroy();

            return stream;
        }

    }

@bitsandfoxes
Copy link
Contributor

The resizing of the screenshot is a good catch. Thanks for sharing!
The native screenshot is a good idea and something that's planned to be added.

@oddgames-david
Copy link

If you allowed attachment streams to be async we could implement screenshots fairly easily ourselves. Or even just give us a callback 1 frame before you actually call GetStream

@pistoleta
Copy link

Gotchas to keep in mind:

  • Record at end of frame i.e. a coroutine yield return new WaitForEndOfFrame() and not in the middle of rendering
  • ScreenCapture.CaptureScreenshotAsTexture() returns a Texture2D that can be encoded to .png, .jpg, .tga byte[]

Also:

  • Native screenshot would be better, ensures always receiving a screenshot when the exception is threaded or from a java/objective-c error (so long as it didn't crash)
  • Need to resize it before compressing to jpg, make the max height and compress amount a setting

This is my current implementation but most of the time I'd love to have screenshots for native exceptions, they are harder to diagnose.

    public class ScreenshotAttachment : IAttachmentContent
    {

        public Stream GetStream()
        {

            if (!UnityThread.IsRunningOnUnityThread)
                return new MemoryStream();

            float ratio = (float)Screen.width / (float)Screen.height;

            int targetHeight = Mathf.Min(Screen.height, 360);
            int targetWidth = Mathf.RoundToInt((float)targetHeight * ratio);

            RenderTexture screenshot = RenderTexture.GetTemporary(Screen.width, Screen.height);

            ScreenCapture.CaptureScreenshotIntoRenderTexture(screenshot);
            
            // Create a render texture to render into
            RenderTexture screenshotResize = RenderTexture.GetTemporary(targetWidth, targetHeight, 0);

            Graphics.Blit(screenshot, screenshotResize);

            RenderTexture.ReleaseTemporary(screenshot);

            RenderTexture previousActiveRT = RenderTexture.active;
            RenderTexture.active = screenshotResize;

            // Create a texture & read data from the active RenderTexture
            Texture2D result = new Texture2D(targetWidth, targetHeight) { name = "Screenshot" };
            result.ReadPixels(new Rect(0, 0, targetWidth, targetHeight), 0, 0);
            result.Apply();

            // Reset to initial state
            RenderTexture.active = previousActiveRT;

            RenderTexture.ReleaseTemporary(screenshotResize);

            var stream = new MemoryStream(result.EncodeToJPG(80));

            result.SafeDestroy();

            return stream;
        }

    }

How do you attach this to the event?

@pistoleta
Copy link

If you allowed attachment streams to be async we could implement screenshots fairly easily ourselves. Or even just give us a callback 1 frame before you actually call GetStream

If BeforeSend would be async we could also set attachments in the context without problems no?

@pistoleta
Copy link

To be honest I never expected Sentry would not have a feature like this... Unity Diagnostics doesn't have 1/10 part of the features you own but does have the screenshot... I dont mean to criticize is just a screenshot of the game just when the exception happened is extremely useful and might save you a lot of time finding where a problem is.

I hope you guys consider giving some priority to this or a temporary workaround.
Thanks a lot!

@bruno-garcia
Copy link
Member Author

This is a priority to us. We're adding support for screenshots on iOS and Android, and there's a thumbnail in Sentry's issue detail page.
The spec for SDK development is here: https://develop.sentry.dev/sdk/features/#screenshots and we plan on adding this to Unity too

@oddgames-david
Copy link

oddgames-david commented Mar 30, 2022

@pistoleta Here's an updated version that works some of the time. You just add it to your Configure(SentryUnityOptions options) method.

public class ScreenshotAttachment : IAttachmentContent, UnityEngine.ILogHandler
{
    private ILogHandler handler;
    private Texture2D screenshot;

    public ScreenshotAttachment()
    {
        handler = Debug.unityLogger.logHandler;
        Debug.unityLogger.logHandler = this;
    }

    private async Task Capture()
    {
        
        Debug.Log("Capturing screenshot");

        await new WaitForEndOfFrame();

        float ratio = (float)Screen.width / (float)Screen.height;

        int targetHeight = Mathf.Min(Screen.height, 360);
        int targetWidth = Mathf.RoundToInt((float)targetHeight * ratio);

        RenderTexture screenshotRt = RenderTexture.GetTemporary(Screen.width, Screen.height);

        ScreenCapture.CaptureScreenshotIntoRenderTexture(screenshotRt);

        // Create a render texture to render into
        RenderTexture screenshotResize = RenderTexture.GetTemporary(targetWidth, targetHeight, 0);

##if UNITY_IOS || UNITY_EDITOR
        Graphics.Blit(screenshotRt, screenshotResize, new Vector2(1, -1), new Vector2(0, 1));
##else
        Graphics.Blit(screenshotRt, screenshotResize);
##endif
        RenderTexture.ReleaseTemporary(screenshotRt);

        RenderTexture previousActiveRT = RenderTexture.active;
        RenderTexture.active = screenshotResize;

        // Create a texture & read data from the active RenderTexture
        screenshot = new Texture2D(targetWidth, targetHeight) { name = "Screenshot" };
        screenshot.ReadPixels(new Rect(0, 0, targetWidth, targetHeight), 0, 0);
        screenshot.Apply();
    }

    public Stream GetStream()
    {


        if (screenshot == null)
        {
            UnityEngine.Debug.Log("Returning empty stream due to missing screenshot texture2d");
            return new MemoryStream();
        }

        var stream = new MemoryStream(screenshot.EncodeToJPG(80));

        screenshot.SafeDestroy();
        UnityEngine.Debug.Log("captured screenshot attachment");
        return stream;
    }

    public void LogFormat(LogType logType, UnityEngine.Object context, string format, params object[] args)
    {
        handler.LogFormat(logType, context, format, args);
    }

    public async void LogException(Exception exception, UnityEngine.Object context)
    {
        await Capture();
        handler.LogException(exception,context);
    }
}

edit by @vaind: fix the codeblock

@vaind vaind self-assigned this Mar 30, 2022
@vaind
Copy link
Collaborator

vaind commented Mar 30, 2022

Hmm, seeing the screenshots don't have a cursor (when in UI), maybe we should also include cursor position in all events? https://docs.unity3d.com/ScriptReference/Input-mousePosition.html

It's not the same issue though, just asking here and if it makes sense we can have a follow-up issue for that.

@vaind vaind mentioned this issue Mar 30, 2022
3 tasks
@vaind
Copy link
Collaborator

vaind commented Mar 31, 2022

For backup: an async version would look somewhat like the following code. It wouldn't work with WebGL because of its single-threaded nature it requires a synchronous stream CopyTo() and we can't have that and wait for the end of frame. GetAwaiter().GetResult() wouldn't work either - just blocks the UI thread indefinitely.

        ScreenshotAttachmentContent.GetStream():
        {
            if (!_behaviour.MainThreadData.IsMainThread())
            {
                _options.DiagnosticLogger?.LogDebug("Won't capture screenshot because we're not on the main thread");
                return new MemoryStream();
            }
            else
            {
                return new ScreenshotCaptureStream(_options, _behaviour);
            }
        }

    // The async version is currently unused because we want the screenshot to be as early as possible after the error
    internal class ScreenshotCaptureStream : Stream
    {
        private readonly SentryOptions _options;
        private readonly TaskCompletionSource<byte[]> _tcs;

        public ScreenshotCaptureStream(SentryOptions options, SentryMonoBehaviour behaviour)
        {
            _options = options;
            _tcs = new();
            behaviour.StartCoroutine(CaptureScreenshot());
        }

        private IEnumerator CaptureScreenshot()
        {
            yield return new WaitForEndOfFrame();
            _tcs.TrySetResult(ScreenshotAttachmentContent.CaptureScreenshot(_options));
        }

        public override async Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken)
        {
            var bytes = await _tcs.Task.ConfigureAwait(false);
            _options.DiagnosticLogger?.LogDebug("Awaiting screenshot finished: {0} bytes", bytes.Length);
            var writer = new BinaryWriter(destination);
            writer.Write(bytes);
            _options.DiagnosticLogger?.LogDebug("Finished writing the screenshot");
        }

        public override bool CanRead => throw new NotImplementedException();
        public override bool CanSeek => throw new NotImplementedException();
        public override bool CanWrite => throw new NotImplementedException();
        public override long Length => throw new NotImplementedException();
        public override long Position { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
        public override void Flush() => throw new NotImplementedException();
        public override int Read(byte[] buffer, int offset, int count) => throw new NotImplementedException();
        public override long Seek(long offset, SeekOrigin origin) => throw new NotImplementedException();
        public override void SetLength(long value) => throw new NotImplementedException();
        public override void Write(byte[] buffer, int offset, int count) => throw new NotImplementedException();
    }

@pistoleta
Copy link

pistoleta commented Mar 31, 2022

I understand for this you had to implement a IDiagnosticLogger and assign it to optios.DiagnosticLogger inside the SentryOptionsConfiguration, correct?

Also, why all the hassle trying to make the screenCapture async? couldn't you simply use Unity's API with
ScreenCapture.CaptureScreenshot() ?

Is it because you don't want the game lag when creating the screenshot? or we must do it like this for some reason?

Thanks for your help.

@vaind
Copy link
Collaborator

vaind commented Mar 31, 2022

No that's not the case. You can see the actual implementation in #670 - ScreenShotAttachment.cs

@pistoleta
Copy link

pistoleta commented Apr 4, 2022

No that's not the case. You can see the actual implementation in #670 - ScreenShotAttachment.cs

I still fail to see how the example in GitHub you pointed can be added to the SentryOptionsConfiguration. I tested in editor and yes, it sends a screenshot when I push the button, but I thought we were talking about attaching a screenshot the moment an exception/error triggers... am I missing something?
I apologize if im being too dumb.
Maybe this is the previous step for what I am asking, im sorry im still not that familiar to all this GitHub thing.

@pistoleta
Copy link

pistoleta commented Apr 4, 2022

public class SentryOptionsConfiguration : ScriptableOptionsConfiguration
{
    public override void Configure(SentryUnityOptions options)
    {
        var buildCFG =  (BuildConfigSO) Resources.Load("Configuration/BuildConfig", typeof(BuildConfigSO));
        options.Environment = buildCFG.buildType.ToString();
        options.Release =Application.version;
        options.AutoSessionTracking = true; // default: false
        options.SampleRate = 1;
        options.TracesSampleRate = 1;
        options.BeforeSend= BeforeSend;
        options.MaxBreadcrumbs = 1000;
    }
    private SentryEvent? BeforeSend(SentryEvent arg)
    {
        return arg;
    }

This is my configure method, just in case I'm missing something.

@vaind
Copy link
Collaborator

vaind commented Apr 4, 2022

It's not released yet so it wouldn't work for you yet, unless you've made a custom build with that branch (which I strongly assume you haven't, based on your message).

I'd suggest waiting for the next package release (v0.14.0) which should include this feature, if everything goes according to the plan (i.e. we don't find any blocking issues)

@pistoleta
Copy link

It's not released yet so it wouldn't work for you yet, unless you've made a custom build with that branch (which I strongly assume you haven't, based on your message).

I'd suggest waiting for the next package release (v0.14.0) which should include this feature, if everything goes according to the plan (i.e. we don't find any blocking issues)

Ah ok, thanks for your kindness and understanding, GitHub it's a bit hard to follow but I'll get used to it.

@oddgames-david
Copy link

@vaind I'd watch out for black screenshots when not using

yield new WaitForEndOfFrame();

@pistoleta
Copy link

I downloaded last release and used the example for sending screenshot but the screenshot seems to be captured upwards.... any hint where the problem might be? I used the exact same code provided in the example.

@bitsandfoxes
Copy link
Contributor

I downloaded last release and used the example for sending screenshot but the screenshot seems to be captured upwards.... any hint where the problem might be? I used the exact same code provided in the example.

Which platform did you target?

@pistoleta
Copy link

iOS, btw i had to comment:
result.SafeDestroy();
since Texture2D doesn't have that method implemented. What's this line supposed to do, release it from memory?

@vaind
Copy link
Collaborator

vaind commented Apr 28, 2022

used the example for sending screenshot

Do you mean the example code, not the new functionality implemented by the latest version? That example is old and doesn't do proper mirroring (necessary for most platforms)

@bitsandfoxes
Copy link
Contributor

Sorry to see you run into problems.
Could you open up a new issue detailing what happened, where (platform, Unity-version), and what you had to do to get there/fix it? The Attach Screenshot feature is supposed to work out of the box and you definitely should not have to comment anything to make it work.

@bitsandfoxes
Copy link
Contributor

If you're updating the Sentry package you also have to update the samples coming with it.

@vaind
Copy link
Collaborator

vaind commented Apr 28, 2022

Also, there's no result.SafeDestroy(); anywhere in this repository as far as I can tell - what example are you actually referring to?

@pistoleta
Copy link

wrong script 🤦 , seems I already had a ScreenShotAttachement that tried to make work some time ago. Apologies

@bitsandfoxes
Copy link
Contributor

No worries! Happy to see it worked out for you regardless!

@pistoleta
Copy link

sorry but I dont see any way to attach screenshot in the 0.15.0 at least nothing new in the examples

@vaind
Copy link
Collaborator

vaind commented Apr 29, 2022

you can enable it in the settings in the Unity UI (Tools->Sentry)

@pistoleta
Copy link

awesome, sorry I didnt see it in the changelog

@pistoleta
Copy link

sorry but I dont see any image attachment, neither from iOS reports or if I send one from unity editor... I enabled from the settings In tools->sentry and also I the SentryOptionsCOnfiguration ScriptableObject.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants