From ed494dbf590debea9e9198e237d9449616aaa609 Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Sat, 15 Aug 2020 15:11:56 +0900 Subject: [PATCH] restorable: Reland: Do not record pixels if restoring is not requried This change also remove the restrictions of operations on graphicscommand.Image. For example, now DrawTriangles and ReplacePixels can be mixed on the same image. Fixes #1022 --- internal/graphicscommand/image.go | 30 --------------- internal/graphicscommand/image_test.go | 7 +--- .../graphicsdriver/opengl/context_desktop.go | 8 ++++ internal/graphicsdriver/opengl/context_js.go | 11 ++++++ .../graphicsdriver/opengl/context_mobile.go | 10 +++++ internal/graphicsdriver/opengl/gl/package.go | 1 + internal/graphicsdriver/opengl/gles/const.go | 1 + internal/graphicsdriver/opengl/image.go | 6 +++ internal/restorable/image.go | 37 +++++++++++++------ 9 files changed, 64 insertions(+), 47 deletions(-) diff --git a/internal/graphicscommand/image.go b/internal/graphicscommand/image.go index 7ac3f12e5..85b252387 100644 --- a/internal/graphicscommand/image.go +++ b/internal/graphicscommand/image.go @@ -26,15 +26,6 @@ import ( "github.com/hajimehoshi/ebiten/v2/internal/png" ) -type lastCommand int - -const ( - lastCommandNone lastCommand = iota - lastCommandClear - lastCommandDrawTriangles - lastCommandReplacePixels -) - // Image represents an image that is implemented with OpenGL. type Image struct { image driver.Image @@ -51,8 +42,6 @@ type Image struct { id int bufferedRP []*driver.ReplacePixelsArgs - - lastCommand lastCommand } var nextID = 1 @@ -151,12 +140,6 @@ func (i *Image) InternalSize() (int, int) { // If the source image is not specified, i.e., src is nil and there is no image in the uniform variables, the // elements for the source image are not used. func (i *Image) DrawTriangles(srcs [graphics.ShaderImageNum]*Image, offsets [graphics.ShaderImageNum - 1][2]float32, vertices []float32, indices []uint16, clr *affine.ColorM, mode driver.CompositeMode, filter driver.Filter, address driver.Address, dstRegion, srcRegion driver.Region, shader *Shader, uniforms []interface{}) { - if i.lastCommand == lastCommandNone { - if !i.screen && mode != driver.CompositeModeClear { - panic("graphicscommand: the image must be cleared first") - } - } - if shader == nil { // Fast path for rendering without a shader (#1355). img := srcs[0] @@ -178,12 +161,6 @@ func (i *Image) DrawTriangles(srcs [graphics.ShaderImageNum]*Image, offsets [gra i.resolveBufferedReplacePixels() theCommandQueue.EnqueueDrawTrianglesCommand(i, srcs, offsets, vertices, indices, clr, mode, filter, address, dstRegion, srcRegion, shader, uniforms) - - if i.lastCommand == lastCommandNone && !i.screen { - i.lastCommand = lastCommandClear - } else { - i.lastCommand = lastCommandDrawTriangles - } } // Sync syncs the texture data in CPU and GPU so that Pixels can return the texture data immediately. @@ -218,12 +195,6 @@ func (i *Image) Pixels() ([]byte, error) { } func (i *Image) ReplacePixels(pixels []byte, x, y, width, height int) { - // ReplacePixels for a part might invalidate the current image that are drawn by DrawTriangles (#593, #738). - if i.lastCommand == lastCommandDrawTriangles { - if x != 0 || y != 0 || i.width != width || i.height != height { - panic("graphicscommand: ReplacePixels for a part after DrawTriangles is forbidden") - } - } i.bufferedRP = append(i.bufferedRP, &driver.ReplacePixelsArgs{ Pixels: pixels, X: x, @@ -231,7 +202,6 @@ func (i *Image) ReplacePixels(pixels []byte, x, y, width, height int) { Width: width, Height: height, }) - i.lastCommand = lastCommandReplacePixels } func (i *Image) IsInvalidated() bool { diff --git a/internal/graphicscommand/image_test.go b/internal/graphicscommand/image_test.go index 55d66d990..df408f064 100644 --- a/internal/graphicscommand/image_test.go +++ b/internal/graphicscommand/image_test.go @@ -69,11 +69,6 @@ func TestClear(t *testing.T) { } func TestReplacePixelsPartAfterDrawTriangles(t *testing.T) { - defer func() { - if r := recover(); r == nil { - t.Errorf("ReplacePixels must panic but not") - } - }() const w, h = 32, 32 clr := NewImage(w, h) src := NewImage(w/2, h/2) @@ -89,6 +84,8 @@ func TestReplacePixelsPartAfterDrawTriangles(t *testing.T) { dst.DrawTriangles([graphics.ShaderImageNum]*Image{clr}, [graphics.ShaderImageNum - 1][2]float32{}, vs, is, nil, driver.CompositeModeClear, driver.FilterNearest, driver.AddressUnsafe, dr, driver.Region{}, nil, nil) dst.DrawTriangles([graphics.ShaderImageNum]*Image{src}, [graphics.ShaderImageNum - 1][2]float32{}, vs, is, nil, driver.CompositeModeSourceOver, driver.FilterNearest, driver.AddressUnsafe, dr, driver.Region{}, nil, nil) dst.ReplacePixels(make([]byte, 4), 0, 0, 1, 1) + + // TODO: Check the result. } func TestShader(t *testing.T) { diff --git a/internal/graphicsdriver/opengl/context_desktop.go b/internal/graphicsdriver/opengl/context_desktop.go index bcbc84c61..e700fab1e 100644 --- a/internal/graphicsdriver/opengl/context_desktop.go +++ b/internal/graphicsdriver/opengl/context_desktop.go @@ -173,6 +173,14 @@ func (c *context) framebufferPixels(f *framebuffer, width, height int) []byte { return pixels } +func (c *context) framebufferPixelsToBuffer(f *framebuffer, buffer buffer, width, height int) { + gl.Flush() + c.bindFramebuffer(f.native) + gl.BindBuffer(gl.PIXEL_PACK_BUFFER, uint32(buffer)) + gl.ReadPixels(0, 0, int32(width), int32(height), gl.RGBA, gl.UNSIGNED_BYTE, nil) + gl.BindBuffer(gl.PIXEL_PACK_BUFFER, 0) +} + func (c *context) activeTexture(idx int) { gl.ActiveTexture(gl.TEXTURE0 + uint32(idx)) } diff --git a/internal/graphicsdriver/opengl/context_js.go b/internal/graphicsdriver/opengl/context_js.go index ff0b539a0..a7797bdb4 100644 --- a/internal/graphicsdriver/opengl/context_js.go +++ b/internal/graphicsdriver/opengl/context_js.go @@ -219,6 +219,17 @@ func (c *context) framebufferPixels(f *framebuffer, width, height int) []byte { return jsutil.Uint8ArrayToSlice(p) } +func (c *context) framebufferPixelsToBuffer(f *framebuffer, buffer buffer, width, height int) { + c.ensureGL() + gl := c.gl + + c.bindFramebuffer(f.native) + gl.Call("bindBuffer", gles.PIXEL_PACK_BUFFER, js.Value(buffer)) + // void gl.readPixels(x, y, width, height, format, type, GLintptr offset); + gl.Call("readPixels", 0, 0, width, height, gles.RGBA, gles.UNSIGNED_BYTE, 0) + gl.Call("bindBuffer", gles.PIXEL_PACK_BUFFER, nil) +} + func (c *context) activeTexture(idx int) { c.ensureGL() gl := c.gl diff --git a/internal/graphicsdriver/opengl/context_mobile.go b/internal/graphicsdriver/opengl/context_mobile.go index 8fcfb3432..d18910a32 100644 --- a/internal/graphicsdriver/opengl/context_mobile.go +++ b/internal/graphicsdriver/opengl/context_mobile.go @@ -160,6 +160,16 @@ func (c *context) framebufferPixels(f *framebuffer, width, height int) []byte { return pixels } +func (c *context) framebufferPixelsToBuffer(f *framebuffer, buffer buffer, width, height int) { + c.ctx.Flush() + + c.bindFramebuffer(f.native) + + c.ctx.BindBuffer(gles.PIXEL_PACK_BUFFER, uint32(buffer)) + c.ctx.ReadPixels(nil, 0, 0, int32(width), int32(height), gles.RGBA, gles.UNSIGNED_BYTE) + c.ctx.BindBuffer(gles.PIXEL_PACK_BUFFER, 0) +} + func (c *context) activeTexture(idx int) { c.ctx.ActiveTexture(uint32(gles.TEXTURE0 + idx)) } diff --git a/internal/graphicsdriver/opengl/gl/package.go b/internal/graphicsdriver/opengl/gl/package.go index 516e4f89f..5537dc15d 100644 --- a/internal/graphicsdriver/opengl/gl/package.go +++ b/internal/graphicsdriver/opengl/gl/package.go @@ -21,6 +21,7 @@ const ( ELEMENT_ARRAY_BUFFER = 0x8893 DYNAMIC_DRAW = 0x88E8 STREAM_DRAW = 0x88E0 + PIXEL_PACK_BUFFER = 0x88EB PIXEL_UNPACK_BUFFER = 0x88EC SHORT = 0x1402 FLOAT = 0x1406 diff --git a/internal/graphicsdriver/opengl/gles/const.go b/internal/graphicsdriver/opengl/gles/const.go index 6ab27462c..508402bd5 100644 --- a/internal/graphicsdriver/opengl/gles/const.go +++ b/internal/graphicsdriver/opengl/gles/const.go @@ -22,6 +22,7 @@ const ( ELEMENT_ARRAY_BUFFER = 0x8893 DYNAMIC_DRAW = 0x88E8 STREAM_DRAW = 0x88E0 + PIXEL_PACK_BUFFER = 0x88EB PIXEL_UNPACK_BUFFER = 0x88EC SHORT = 0x1402 FLOAT = 0x1406 diff --git a/internal/graphicsdriver/opengl/image.go b/internal/graphicsdriver/opengl/image.go index 53ec5d7d6..d3abb2594 100644 --- a/internal/graphicsdriver/opengl/image.go +++ b/internal/graphicsdriver/opengl/image.go @@ -135,6 +135,12 @@ func (i *Image) ReplacePixels(args []*driver.ReplacePixelsArgs) { if i.pbo.equal(*new(buffer)) { i.pbo = i.graphics.context.newPixelBufferObject(w, h) + if i.pbo.equal(*new(buffer)) { + panic("opengl: newPixelBufferObject failed") + } + if i.framebuffer != nil { + i.graphics.context.framebufferPixelsToBuffer(i.framebuffer, i.pbo, i.width, i.height) + } } if i.pbo.equal(*new(buffer)) { panic("opengl: newPixelBufferObject failed") diff --git a/internal/restorable/image.go b/internal/restorable/image.go index 150910243..068949617 100644 --- a/internal/restorable/image.go +++ b/internal/restorable/image.go @@ -170,19 +170,29 @@ func (i *Image) Extend(width, height int) *Image { panic(fmt.Sprintf("restorable: the original size (%d, %d) cannot be extended to (%d, %d)", i.width, i.height, width, height)) } - if i.stale { - panic("restorable: Extend at a stale image is forbidden") - } - - if len(i.drawTrianglesHistory) > 0 { - panic("restorable: Extend after DrawTriangles is forbidden") - } - newImg := NewImage(width, height) newImg.SetVolatile(i.volatile) - i.basePixels.Apply(newImg.image) + // Use DrawTriangles instead of ReplacePixels because the image i might be stale and not have its pixels + // information. + srcs := [graphics.ShaderImageNum]*Image{i} + var offsets [graphics.ShaderImageNum - 1][2]float32 + sw, sh := i.image.InternalSize() + vs := quadVertices(0, 0, float32(sw), float32(sh), 0, 0, float32(sw), float32(sh), 1, 1, 1, 1) + is := graphics.QuadIndices() + dr := driver.Region{ + X: 0, + Y: 0, + Width: float32(sw), + Height: float32(sh), + } + newImg.DrawTriangles(srcs, offsets, vs, is, nil, driver.CompositeModeCopy, driver.FilterNearest, driver.AddressUnsafe, dr, driver.Region{}, nil, nil) + + // Overwrite the history as if the image newImg is created only by ReplacePixels. Now drawTrianglesHistory + // and basePixels cannot be mixed. + newImg.drawTrianglesHistory = nil newImg.basePixels = i.basePixels + newImg.stale = i.stale i.Dispose() @@ -287,6 +297,11 @@ func (i *Image) ReplacePixels(pixels []byte, x, y, width, height int) { i.image.ReplacePixels(make([]byte, 4*width*height), x, y, width, height) } + if !needsRestoring() || i.screen || i.volatile { + i.makeStale() + return + } + if x == 0 && y == 0 && width == w && height == h { if pixels != nil { i.basePixels.AddOrReplace(pixels, 0, 0, w, h) @@ -298,9 +313,7 @@ func (i *Image) ReplacePixels(pixels []byte, x, y, width, height int) { return } - // It looked like ReplacePixels on a part of image deletes other region that are rendered by DrawTriangles - // (#593, #758). - + // drawTrianglesHistory and basePixels cannot be mixed. if len(i.drawTrianglesHistory) > 0 { panic("restorable: ReplacePixels for a part after DrawTriangles is forbidden") }