JetBrains / skija

Java bindings for Skia
Apache License 2.0
2.63k stars 127 forks source link

Resizing with snap does not update drawable area #126

Open Martmists-GH opened 3 years ago

Martmists-GH commented 3 years ago

I made a sample app using lwjgl as backend that just draws some text in the corner: Screenshot_20210807_103641 This works fine when manually resizing to any size. However, once I start making it snap to a side, it does not update the surface correctly*: Screenshot_20210807_103748 It does tell me it resized to fullscreen in the handler callback though.

The code handling resizing is as follows:

// init
glfwSetWindowSizeCallback(window) { w, width, height -> onResize(width, height) }

// resize handler
fun onResize(width: Int, height: Int) {
    // create new backend
    val newBackend = BackendRenderTarget.makeGL(
        width,
        height,
        0,
        8,
        fbId,
        FramebufferFormat.GR_GL_RGBA8
    )
    val newSurface = Surface.makeFromBackendRenderTarget(
        context,
        renderTarget,
        SurfaceOrigin.BOTTOM_LEFT,
        SurfaceColorFormat.RGBA_8888,
        ColorSpace.getSRGB()
    )

    // keep old values
    val oldSurface = surface
    val oldBackend = renderTarget

    // set new ones
    // these are fields, used in the render loop to allow swapping the surfaces
    renderTarget = newBackend
    surface = newSurface

    // clear old values 
    // done this way to prevent closing while processing, 
    //  though this may still happen and I don't know of a good way to handle it
    oldSurface.close()
    oldBackend.close()

    println("onResize $width $height")
    ResizeEvent.EVENT.invoker().invoke(width, height)
}

*it seems it requires an additional resize to actually trigger the surface reload.

tonsky commented 3 years ago

Seems to work for me on macOS. Maybe you need to trigger redraw after resize?

Martmists-GH commented 3 years ago

How would I do so? I'd assumed the loop calling all the drawing methods would do so already

tonsky commented 3 years ago

Can you share the source?

Martmists-GH commented 3 years ago
class Window {
    // Skija values
    private lateinit var context: DirectContext
    private lateinit var renderTarget: BackendRenderTarget
    private lateinit var surface: Surface

    // Contains utilities and the canvas
    private lateinit var ctx: RenderContext

    private val window: Long

    // initial values
    private val width = 640
    private val height = 480

    // Seems to be needed?
    private var fbId = 0

    // Low numbers are rendered first
    private val layers = PriorityQueue<Pair<Int, RenderLayer>>(compareBy { it.first })

    init {
        glfwInit()
        glfwWindowHint(GLFW_VISIBLE, GLFW_FALSE)
        glfwWindowHint(GLFW_RESIZABLE, GLFW_TRUE)

        window = glfwCreateWindow(width, height, "Hello Window", NULL, NULL)
        glfwMakeContextCurrent(window)
        glfwSwapInterval(1)
        glfwShowWindow(window)

        // set up callbacks
        glfwSetWindowSizeCallback(window) { w, width, height -> onResize(width, height) }
        glfwSetKeyCallback(window) { w, key, scancode, action, mods -> onKey(key, scancode, action, mods) }
        glfwSetMouseButtonCallback(window) { w, button, action, mods -> onClick(button, action, mods) }
    }

    fun start() {
        GL.createCapabilities()
        context = DirectContext.makeGL()
        fbId = glGetInteger(0x8CA6) // GL_FRAMEBUFFER_BINDING

        renderTarget = BackendRenderTarget.makeGL(
            width,
            height,
            0,
            8,
            fbId,
            FramebufferFormat.GR_GL_RGBA8
        )

        surface = Surface.makeFromBackendRenderTarget(
            context,
            renderTarget,
            SurfaceOrigin.BOTTOM_LEFT,
            SurfaceColorFormat.RGBA_8888,
            ColorSpace.getSRGB()
        )

        ctx = RenderContext(surface.canvas)

        // Render loop
        while (!glfwWindowShouldClose(window)) {
            surface.canvas.clear(0xFF000000.toInt() + ctx.theme.BACKGROUND)

            layers.forEach {
                it.second.render(ctx)
            }

            context.flush()
            glfwSwapBuffers(window)
            glfwPollEvents()
        }

        surface.close()
        renderTarget.close()
        context.close()

        glfwFreeCallbacks(window);
        glfwDestroyWindow(window);

        glfwTerminate();
        glfwSetErrorCallback(null)?.free();
    }

    fun addLayer(priority: Int, layer: RenderLayer) {
        layers.add(Pair(priority, layer))
    }

    private fun onResize(width: Int, height: Int) {
        // Create new backend
        val newBackend = BackendRenderTarget.makeGL(
            width,
            height,
            0,
            8,
            fbId,
            FramebufferFormat.GR_GL_RGBA8
        )
        val newSurface = Surface.makeFromBackendRenderTarget(
            context,
            renderTarget,
            SurfaceOrigin.BOTTOM_LEFT,
            SurfaceColorFormat.RGBA_8888,
            ColorSpace.getSRGB()
        )

        // Swap (should reduce issues from using closed backend, assuming this runs on a separate thread)
        val oldSurface = surface
        val oldBackend = renderTarget
        val oldCtx = ctx
        renderTarget = newBackend
        surface = newSurface
        ctx = RenderContext(surface.canvas)

        // Close old backend
        oldSurface.close()
        oldBackend.close()
        oldCtx.textRenderer.close()

        println("onResize $width $height")
        ResizeEvent.EVENT.invoker().invoke(width, height)
    }

    private fun onKey(key: Int, scancode: Int, action: Int, mods: Int) {
        println("onKey $key $scancode $action $mods")
        KeyEvent.EVENT.invoker().invoke(key, scancode, action, mods)
    }

    private fun onClick(button: Int, action: Int, mods: Int) {
        println("onClick $button $action $mods")
        // TODO: Event
    }
}

it clearly logs onResize 1920 1080 when I drag it to the top of my screen to make it full-screen, but the text remains in the middle.

Martmists-GH commented 3 years ago

I've been able to confirm this affects multiple devices and multiple OSes. Based on where the contents appear when resizing, I'm guessing the internal buffer does not have the correct size and/or downscales the output to match the previous known backend size, though I have no real proof of this.

tonsky commented 3 years ago

Have you tried adding draw to the end of resize handler? That’s what I am doing in LWJGL example, and I don’t see any artefacts (apart from resize being late for ~1 frame, but that’s expected from GLFW): https://github.com/JetBrains/skija/blob/5fb057b0d17ab6ec2e5c284e56707f0527daa54a/examples/lwjgl/src/Main.java#L154-L156

What OSes have you tested this on?

Also, I don’t think I understad your issue completely. What exactly are you doing (step by step), and what are you seeing? What do you mean by “snap to a side”? When you say “it requires an additional resize to actually trigger the surface reload” do you mean it renders at the wrong size permanently or just for 1 frame?

Martmists-GH commented 3 years ago

Have you tried adding draw to the end of resize handler? That’s what I am doing in LWJGL example, and I don’t see any artefacts (apart from resize being late for ~1 frame, but that’s expected from GLFW):

Not yet, I'll give that a try.

To be precise, I've tested this on Arch Linux in KDE and on Windows 10. With snapping I mean when you drag a window to a side or corner, and the WM resizes it to full/half/quarter your screen size. The additional resize means resizing the window through normal means afterwards, though this seems to always have the previous window size as rendered size.


Adding a call to draw() at the end of my onResize function (different project, but exact same setup for rendering) does not seem to change anything. One thing I could think of is the value of dpi in that example, though I have no clue what this value does or means, especially since I try to resize to the new window size either way. Screenshot_20210815_210010

Martmists-GH commented 3 years ago

I recorded my screen to show how I can reproduce the issue

https://user-images.githubusercontent.com/16361449/129489637-75bf8e47-3074-4584-baa8-8d99e60db361.mp4

Martmists-GH commented 3 years ago

Taking screenshots from the above video and lining them up, you can see what I meant with "match the previous known backend size"; the size of the new buffer (left side to where it cuts off) is exactly equal to the previous size of the window

lined_up

tonsky commented 3 years ago

I checked with our LWJGL example (which is based on GLFW) and Ubuntu 20.04, seems to work fine. Not sure what’s going on in your case.

Are you sure you get correct width/height in onResize callback? Have you tried moving oldSurface.close()/oldBackend.close() before creating new ones?

tonsky commented 3 years ago

Can you check too, btw? Clone this and run ./examples/lwjgl/script/run.py --skija-version 0.93.1. Does it exibit the same behavior?

Martmists-GH commented 3 years ago
Compiling 55 java files to target/classes
error: release version 16 not supported
Usage: javac <options> <source files>
use --help for a list of possible options
Traceback (most recent call last):
  File "/home/mart/temp/skija/./examples/lwjgl/script/run.py", line 57, in <module>
    sys.exit(main())
  File "/home/mart/temp/skija/./examples/lwjgl/script/run.py", line 40, in main
    common.javac(sources, 'target/classes', classpath = classpath, release = '16')
  File "/home/mart/temp/skija/script/common.py", line 61, in javac
    check_call([
  File "/home/mart/temp/skija/script/common.py", line 23, in check_call
    res = subprocess.check_call(args, **kwargs)
  File "/usr/lib/python3.9/subprocess.py", line 373, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['javac', '-encoding', 'UTF8', '--release', '16', '--class-path', '/home/mart/.m2/repository/org/projectlombok/lombok/1.18.20/lombok-1.18.20.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl/3.2.3/lwjgl-3.2.3.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl-glfw/3.2.3/lwjgl-glfw-3.2.3.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl-opengl/3.2.3/lwjgl-opengl-3.2.3.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl/3.2.3/lwjgl-3.2.3-natives-linux.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl-glfw/3.2.3/lwjgl-glfw-3.2.3-natives-linux.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl-opengl/3.2.3/lwjgl-opengl-3.2.3-natives-linux.jar:/home/mart/.m2/repository/org/jetbrains/skija/skija-shared/0.93.1/skija-shared-0.93.1.jar:/home/mart/.m2/repository/org/jetbrains/skija/skija-linux/0.93.1/skija-linux-0.93.1.jar:target/classes', '-d', 'target/classes', 'src/Main.java', '../scenes/src/BitmapScene.java', '../scenes/src/RunIteratorScene.java', '../scenes/src/ShadowsScene.java', '../scenes/src/DebugTextRun.java', '../scenes/src/ImageCodecsScene.java', '../scenes/src/EmptyScene.java', '../scenes/src/WatchesScene.java', '../scenes/src/ShapersScene.java', '../scenes/src/ImageBenchScene.java', '../scenes/src/FontScene.java', '../scenes/src/Scenes.java', '../scenes/src/SquaresScene.java', '../scenes/src/Pair.java', '../scenes/src/ParagraphScene.java', '../scenes/src/SVGScalingScene.java', '../scenes/src/ColorFiltersScene.java', '../scenes/src/RuntimeEffectScene.java', '../scenes/src/HUD.java', '../scenes/src/DrawableScene.java', '../scenes/src/TextBlobScene.java', '../scenes/src/PathEffectsScene.java', '../scenes/src/ShadersScene.java', '../scenes/src/BlendsScene.java', '../scenes/src/SwingScene.java', '../scenes/src/WallOfTextScene.java', '../scenes/src/SkottieScene.java', '../scenes/src/SVGScene.java', '../scenes/src/PythagorasScene.java', '../scenes/src/TextLineDecorationsScene.java', '../scenes/src/TextLineScene.java', '../scenes/src/PixelGridScene.java', '../scenes/src/RunHandlerScene.java', '../scenes/src/GeometryScene.java', '../scenes/src/ImagesScene.java', '../scenes/src/ParagraphStyleScene.java', '../scenes/src/PathsScene.java', '../scenes/src/BitmapImageScene.java', '../scenes/src/CodecScene.java', '../scenes/src/DecorationsBenchScene.java', '../scenes/src/ShadowUtilsScene.java', '../scenes/src/FontRenderingScene.java', '../scenes/src/MaskFiltersScene.java', '../scenes/src/ImageFiltersScene.java', '../scenes/src/FontSizeScene.java', '../scenes/src/Scene.java', '../scenes/src/TextShapeBenchScene.java', '../scenes/src/FigmaScene.java', '../scenes/src/BreakIteratorScene.java', '../scenes/src/MatrixScene.java', '../scenes/src/TextStyleScene.java', '../scenes/src/DebugTextBlobHandler.java', '../scenes/src/ParagraphMetricsScene.java', '../scenes/src/PictureRecorderScene.java', '../scenes/src/FontVariationsScene.java']' returned non-zero exit status 2.

looks like it fails to compile when running that command with java 11 set, and with java 16:

Compiling 55 java files to target/classes
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Exception in thread "main" java.lang.AssertionError: Horizontal dpi=1.0, vertical dpi=1.0026315
    at org.jetbrains.skija.examples.lwjgl.Window.updateDimensions(Main.java:77)
    at org.jetbrains.skija.examples.lwjgl.Window.createWindow(Main.java:100)
    at org.jetbrains.skija.examples.lwjgl.Window.run(Main.java:60)
    at org.jetbrains.skija.examples.lwjgl.Main.main(Main.java:32)
Traceback (most recent call last):
  File "/home/mart/temp/skija/./examples/lwjgl/script/run.py", line 57, in <module>
    sys.exit(main())
  File "/home/mart/temp/skija/./examples/lwjgl/script/run.py", line 43, in main
    common.check_call([
  File "/home/mart/temp/skija/script/common.py", line 23, in check_call
    res = subprocess.check_call(args, **kwargs)
  File "/usr/lib/python3.9/subprocess.py", line 373, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['java', '--class-path', 'target/classes:/home/mart/.m2/repository/org/projectlombok/lombok/1.18.20/lombok-1.18.20.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl/3.2.3/lwjgl-3.2.3.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl-glfw/3.2.3/lwjgl-glfw-3.2.3.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl-opengl/3.2.3/lwjgl-opengl-3.2.3.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl/3.2.3/lwjgl-3.2.3-natives-linux.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl-glfw/3.2.3/lwjgl-glfw-3.2.3-natives-linux.jar:/home/mart/.m2/repository/org/lwjgl/lwjgl-opengl/3.2.3/lwjgl-opengl-3.2.3-natives-linux.jar:/home/mart/.m2/repository/org/jetbrains/skija/skija-shared/0.93.1/skija-shared-0.93.1.jar:/home/mart/.m2/repository/org/jetbrains/skija/skija-linux/0.93.1/skija-linux-0.93.1.jar', '-Djava.awt.headless=true', '-enableassertions', '-enablesystemassertions', '-Xcheck:jni', '-Dskija.logLevel=DEBUG', 'org.jetbrains.skija.examples.lwjgl.Main']' returned non-zero exit status 1.

Note that in both cases nothing visually appeared.

Martmists-GH commented 3 years ago

Are you sure you get correct width/height in onResize callback?

Yes

Have you tried moving oldSurface.close()/oldBackend.close() before creating new ones?

Yes, that still has the same issue.

tonsky commented 3 years ago

Remove assert from updateDimensions?