From 7d725f3e58dab8e28a12f8a22f4ec96cc50b27bd Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Mon, 4 Jul 2022 11:50:06 +0900 Subject: [PATCH] Revert "internal/graphicscommand: bug fix: using an image just after ReplacePixels might fail on Metal" This reverts commit c31cc4ecffca2657444e50716d638f3ebbf84ad3. Reason: This didn't fix the issue. Updates #2154 --- internal/graphicscommand/image.go | 42 +----- .../graphicsdriver/metal/graphics_darwin.go | 1 + internal/processtest/testdata/issue2154.go | 121 ------------------ 3 files changed, 6 insertions(+), 158 deletions(-) delete mode 100644 internal/processtest/testdata/issue2154.go diff --git a/internal/graphicscommand/image.go b/internal/graphicscommand/image.go index d2ee4914f..67fdb780a 100644 --- a/internal/graphicscommand/image.go +++ b/internal/graphicscommand/image.go @@ -18,7 +18,6 @@ import ( "fmt" "image" "os" - "runtime" "sort" "strings" @@ -55,11 +54,6 @@ func genNextID() int { return id } -var ( - dummyDst *Image - dummySrc *Image -) - // NewImage returns a new image. // // Note that the image is not initialized yet. @@ -80,9 +74,9 @@ func NewImage(width, height int, screenFramebuffer bool) *Image { return i } -func (i *Image) resolveBufferedReplacePixels() bool { +func (i *Image) resolveBufferedReplacePixels() { if len(i.bufferedRP) == 0 { - return false + return } c := &replacePixelsCommand{ dst: i, @@ -90,8 +84,6 @@ func (i *Image) resolveBufferedReplacePixels() bool { } theCommandQueue.Enqueue(c) i.bufferedRP = nil - - return true } func (i *Image) Dispose() { @@ -136,16 +128,13 @@ 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 graphicsdriver.CompositeMode, filter graphicsdriver.Filter, address graphicsdriver.Address, dstRegion, srcRegion graphicsdriver.Region, shader *Shader, uniforms [][]float32, evenOdd bool) { - var needDummyRendering bool if shader == nil { // Fast path for rendering without a shader (#1355). img := srcs[0] if img.screen { panic("graphicscommand: the screen image cannot be the rendering source") } - if img.resolveBufferedReplacePixels() { - needDummyRendering = true - } + img.resolveBufferedReplacePixels() } else { for _, src := range srcs { if src == nil { @@ -154,31 +143,10 @@ func (i *Image) DrawTriangles(srcs [graphics.ShaderImageNum]*Image, offsets [gra if src.screen { panic("graphicscommand: the screen image cannot be the rendering source") } - if src.resolveBufferedReplacePixels() { - needDummyRendering = true - } + src.resolveBufferedReplacePixels() } } - if i.resolveBufferedReplacePixels() { - needDummyRendering = true - } - - // On Metal, using an image just after ReplacePixel might fail (#2154). - // This is a very dirty hack but there seemed no other way. - // In order to reproduce the issue, run: - // - // go test ./internal/processtest/ -run=/issue2154.go -count=100 - // - // TODO: Detect the graphics library and apply this logic only when the graphics library is Metal. - if runtime.GOOS == "darwin" && needDummyRendering { - if dummyDst == nil { - dummyDst = NewImage(1, 1, false) - } - if dummySrc == nil { - dummySrc = NewImage(1, 1, false) - } - dummyDst.DrawTriangles([graphics.ShaderImageNum]*Image{dummySrc}, [graphics.ShaderImageNum - 1][2]float32{}, make([]float32, 8*3), []uint16{0, 1, 2}, affine.ColorMIdentity{}, graphicsdriver.CompositeModeSourceOver, graphicsdriver.FilterNearest, graphicsdriver.AddressUnsafe, graphicsdriver.Region{}, graphicsdriver.Region{}, nil, nil, false) - } + i.resolveBufferedReplacePixels() theCommandQueue.EnqueueDrawTrianglesCommand(i, srcs, offsets, vertices, indices, clr, mode, filter, address, dstRegion, srcRegion, shader, uniforms, evenOdd) } diff --git a/internal/graphicsdriver/metal/graphics_darwin.go b/internal/graphicsdriver/metal/graphics_darwin.go index e6443f028..f5624edeb 100644 --- a/internal/graphicsdriver/metal/graphics_darwin.go +++ b/internal/graphicsdriver/metal/graphics_darwin.go @@ -1185,6 +1185,7 @@ func (i *Image) syncTexture() { bce.EndEncoding() cb.Commit() + // TODO: Are fences available here? cb.WaitUntilCompleted() } diff --git a/internal/processtest/testdata/issue2154.go b/internal/processtest/testdata/issue2154.go deleted file mode 100644 index cee3c6168..000000000 --- a/internal/processtest/testdata/issue2154.go +++ /dev/null @@ -1,121 +0,0 @@ -// Copyright 2022 The Ebitengine Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//go:build ignore -// +build ignore - -package main - -import ( - "errors" - "fmt" - "image/color" - - "github.com/hajimehoshi/ebiten/v2" -) - -var regularTermination = errors.New("regular termination") - -var srcInit *ebiten.Image - -func init() { - const ( - w = 2 - h = 2 - ) - - //src2 := ebiten.NewImage(1, 1) - - src0 := ebiten.NewImage(w, h) - src0.Fill(color.RGBA{0xff, 0xff, 0xff, 0xff}) - src0.Set(0, 0, color.RGBA{0, 0, 0, 0xff}) - src0.Set(0, 1, color.RGBA{0, 0, 0, 0xff}) - src0.Set(1, 0, color.RGBA{0, 0, 0, 0xff}) - - src1 := ebiten.NewImage(w, h) - // Using the image as a source just after Set caused troubles on Metal. - // For example, inserting src1.Fill(color.RGBA{0, 0xff, 0, 0xff}) here hid the error. - src1.DrawImage(src0, nil) - - srcInit = src1 -} - -type Game struct { - count int - dst *ebiten.Image -} - -func (g *Game) Update() error { - g.count++ - if g.count == 16 { - return regularTermination - } - return nil -} - -func (g *Game) Draw(screen *ebiten.Image) { - screen.Fill(color.RGBA{0xff, 0xff, 0xff, 0xff}) - screen.DrawImage(srcInit, nil) - - if g.dst == nil { - g.dst = ebiten.NewImage(screen.Size()) - return - } - - g.dst.DrawImage(screen, nil) - if got, want := g.dst.At(0, 0), (color.RGBA{0, 0, 0, 0xff}); got != want { - panic(fmt.Sprintf("count: %d, got: %v, want: %v", g.count, got, want)) - } - if got, want := g.dst.At(1, 1), (color.RGBA{0xff, 0xff, 0xff, 0xff}); got != want { - panic(fmt.Sprintf("count: %d, got: %v, want: %v", g.count, got, want)) - } - g.dst.Clear() - - const ( - w = 2 - h = 2 - ) - - src0 := ebiten.NewImage(w, h) - defer src0.Dispose() - src0.Fill(color.RGBA{0xff, 0xff, 0xff, 0xff}) - src0.Set(0, 0, color.RGBA{0, 0, 0, 0xff}) - src0.Set(0, 1, color.RGBA{0, 0, 0, 0xff}) - src0.Set(1, 0, color.RGBA{0, 0, 0, 0xff}) - - src1 := ebiten.NewImage(w, h) - defer src1.Dispose() - src1.DrawImage(src0, nil) - - screen.Fill(color.RGBA{0xff, 0xff, 0xff, 0xff}) - screen.DrawImage(src1, nil) - - g.dst.DrawImage(screen, nil) - if got, want := g.dst.At(0, 0), (color.RGBA{0, 0, 0, 0xff}); got != want { - panic(fmt.Sprintf("count: %d, got: %v, want: %v", g.count, got, want)) - } - if got, want := g.dst.At(1, 1), (color.RGBA{0xff, 0xff, 0xff, 0xff}); got != want { - panic(fmt.Sprintf("count: %d, got: %v, want: %v", g.count, got, want)) - } -} - -func (g *Game) Layout(width, height int) (int, int) { - return width, height -} - -func main() { - if err := ebiten.RunGame(&Game{}); err != nil && err != regularTermination { - panic(err) - } -}