From 8169253a574a026be324c7341c6ad03d45ce23b6 Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Tue, 9 Jan 2024 02:44:20 +0900 Subject: [PATCH] internal/restorable: remove unused functions and variables Updates #805 --- internal/restorable/doc.go | 54 +------ internal/restorable/export_test.go | 23 --- internal/restorable/image.go | 218 +--------------------------- internal/restorable/images.go | 34 +---- internal/restorable/pixelrecords.go | 159 -------------------- internal/restorable/rect_test.go | 165 --------------------- 6 files changed, 7 insertions(+), 646 deletions(-) delete mode 100644 internal/restorable/export_test.go delete mode 100644 internal/restorable/pixelrecords.go delete mode 100644 internal/restorable/rect_test.go diff --git a/internal/restorable/doc.go b/internal/restorable/doc.go index fb51cae66..adc4c448f 100644 --- a/internal/restorable/doc.go +++ b/internal/restorable/doc.go @@ -12,57 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package restorable offers an Image struct that stores image commands +// Package restorable used to offer an Image struct that stores image commands // and restores its pixel data from the commands when context lost happens. // -// When a function like DrawImage or Fill is called, an Image tries to record -// the information for restoring. +// However, now Ebitengine doesn't handle context losts, and this package is +// just a thin wrapper. // -// * Context lost -// -// Contest lost is a process that information on GPU memory are removed by OS -// to make more room on GPU memory. -// This can happen e.g. when GPU memory usage is high, or just switching applications -// might cause context lost on mobiles. -// As Ebitengine's image data is on GPU memory, the game can't continue when context lost happens -// without restoring image information. -// The package restorable is the package to record information for such restoring. -// -// * DrawImage -// -// DrawImage function tries to record an item of 'draw image history' in the target image. -// If a target image is stale or volatile, no item is created. -// If an item of the history is created, -// it can be said that the target image depends on the source image. -// In other words, If A.DrawImage(B, ...) is called, -// it can be said that the image A depends on the image B. -// -// * Fill, WritePixels and Dispose -// -// These functions are also drawing functions and the target image stores the pixel data -// instead of draw image history items. There is no dependency here. -// -// * Making images stale -// -// After any of the drawing functions is called, the target image can't be depended on by -// any other images. For example, if an image A depends on an image B, and B is changed -// by a Fill call after that, the image A can't depend on the image B anymore. -// In this case, as the image B can no longer be used to restore the image A, -// the image A becomes 'stale'. -// As all the stale images are resolved before context lost happens, -// draw image history items are kept as they are -// (even if an image C depends on the stale image A, it is still fine). -// -// * Stale image -// -// A stale image is an image that can't be restored from the recorded information. -// All stale images must be resolved by reading pixels from GPU before the frame ends. -// If a source image of DrawImage is a stale image, the target always becomes stale. -// -// * Volatile image -// -// A volatile image is a special image that is always cleared when a frame starts. -// For instance, the game screen passed via the update function is a volatile image. -// A volatile image doesn't have to record the drawing history. -// If a source image of DrawImage is a volatile image, the target always becomes stale. +// TODO: Integrate this package into internal/atlas and internal/graphicscommand (#805). package restorable diff --git a/internal/restorable/export_test.go b/internal/restorable/export_test.go deleted file mode 100644 index aad181758..000000000 --- a/internal/restorable/export_test.go +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright 2023 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. - -package restorable - -import ( - "image" -) - -func AppendRegionRemovingDuplicates(regions *[]image.Rectangle, region image.Rectangle) { - appendRegionRemovingDuplicates(regions, region) -} diff --git a/internal/restorable/image.go b/internal/restorable/image.go index ef193bcfa..99140d0bc 100644 --- a/internal/restorable/image.go +++ b/internal/restorable/image.go @@ -23,60 +23,6 @@ import ( "github.com/hajimehoshi/ebiten/v2/internal/graphicsdriver" ) -type Pixels struct { - pixelsRecords *pixelsRecords -} - -// Apply applies the Pixels state to the given image especially for restoring. -func (p *Pixels) Apply(img *graphicscommand.Image) { - // Pixels doesn't clear the image. This is a caller's responsibility. - - if p.pixelsRecords == nil { - return - } - p.pixelsRecords.apply(img) -} - -func (p *Pixels) AddOrReplace(pix *graphics.ManagedBytes, region image.Rectangle) { - if p.pixelsRecords == nil { - p.pixelsRecords = &pixelsRecords{} - } - p.pixelsRecords.addOrReplace(pix, region) -} - -func (p *Pixels) Clear(region image.Rectangle) { - // Note that we don't care whether the region is actually removed or not here. There is an actual case that - // the region is allocated but nothing is rendered. See TestDisposeImmediately at shareable package. - if p.pixelsRecords == nil { - return - } - p.pixelsRecords.clear(region) -} - -func (p *Pixels) ReadPixels(pixels []byte, region image.Rectangle, imageWidth, imageHeight int) { - if p.pixelsRecords == nil { - for i := range pixels { - pixels[i] = 0 - } - return - } - p.pixelsRecords.readPixels(pixels, region, imageWidth, imageHeight) -} - -func (p *Pixels) AppendRegion(regions []image.Rectangle) []image.Rectangle { - if p.pixelsRecords == nil { - return regions - } - return p.pixelsRecords.appendRegions(regions) -} - -func (p *Pixels) Dispose() { - if p.pixelsRecords == nil { - return - } - p.pixelsRecords.dispose() -} - type ImageType int const ( @@ -87,44 +33,16 @@ const ( ImageTypeScreen // ImageTypeVolatile indicates the image is cleared whenever a frame starts. - // - // Regular non-volatile images need to record drawing history or read its pixels from GPU if necessary so that all - // the images can be restored automatically from the context lost. However, such recording the drawing history or - // reading pixels from GPU are expensive operations. Volatile images can skip such operations, but the image content - // is cleared every frame instead. ImageTypeVolatile ) -// Image represents an image that can be restored when GL context is lost. +// Image represents an image. type Image struct { image *graphicscommand.Image width int height int - basePixels Pixels - - // stale indicates whether the image needs to be synced with GPU as soon as possible. - stale bool - - // staleRegions indicates the regions to restore. - // staleRegions is valid only when stale is true. - // staleRegions is not used when AlwaysReadPixelsFromGPU() returns true. - staleRegions []image.Rectangle - - // pixelsCache is cached byte slices for pixels. - // pixelsCache is just a cache to avoid allocations (#2375). - // - // A key is the region and a value is a byte slice for the region. - // - // It is fine to reuse the same byte slice for the same region for basePixels, - // as old pixels for the same region will be invalidated at basePixel.AddOrReplace. - pixelsCache map[image.Rectangle][]byte - - // regionsCache is cached regions. - // regionsCache is just a cache to avoid allocations (#2375). - regionsCache []image.Rectangle - imageType ImageType } @@ -192,20 +110,11 @@ func clearImage(i *graphicscommand.Image, region image.Rectangle) { i.DrawTriangles([graphics.ShaderImageCount]*graphicscommand.Image{}, vs, is, graphicsdriver.BlendClear, region, [graphics.ShaderImageCount]image.Rectangle{}, clearShader.shader, nil, graphicsdriver.FillAll) } -// makeStale makes the image stale. -func (i *Image) makeStale(rect image.Rectangle) { - i.stale = true -} - // ClearPixels clears the specified region by WritePixels. func (i *Image) ClearPixels(region image.Rectangle) { i.WritePixels(nil, region) } -func (i *Image) needsRestoring() bool { - return i.imageType == ImageTypeRegular -} - // WritePixels replaces the image pixels with the given pixels slice. // // The specified region must not be overlapped with other regions by WritePixels. @@ -218,18 +127,11 @@ func (i *Image) WritePixels(pixels *graphics.ManagedBytes, region image.Rectangl panic(fmt.Sprintf("restorable: out of range %v", region)) } - // TODO: Avoid making other images stale if possible. (#514) - // For this purpose, images should remember which part of that is used for DrawTriangles. - theImages.makeStaleIfDependingOn(i) - if pixels != nil { i.image.WritePixels(pixels, region) } else { clearImage(i.image, region) } - - // Even if the image is already stale, call makeStale to extend the stale region. - i.makeStale(region) } // DrawTriangles draws triangles with the given image. @@ -248,10 +150,6 @@ func (i *Image) DrawTriangles(srcs [graphics.ShaderImageCount]*Image, vertices [ if len(vertices) == 0 { return } - theImages.makeStaleIfDependingOn(i) - - // Even if the image is already stale, call makeStale to extend the stale region. - i.makeStale(dstRegion) var imgs [graphics.ShaderImageCount]*graphicscommand.Image for i, src := range srcs { @@ -275,88 +173,6 @@ func (i *Image) ReadPixels(graphicsDriver graphicsdriver.Graphics, pixels []byte return nil } -// makeStaleIfDependingOn makes the image stale if the image depends on target. -func (i *Image) makeStaleIfDependingOn(target *Image) { - if i.stale { - return - } - if i.dependsOn(target) { - // There is no new region to make stale. - i.makeStale(image.Rectangle{}) - } -} - -// makeStaleIfDependingOnShader makes the image stale if the image depends on shader. -func (i *Image) makeStaleIfDependingOnShader(shader *Shader) { - if i.stale { - return - } - if i.dependsOnShader(shader) { - // There is no new region to make stale. - i.makeStale(image.Rectangle{}) - } -} - -// dependsOn reports whether the image depends on target. -func (i *Image) dependsOn(target *Image) bool { - return false -} - -// dependsOnShader reports whether the image depends on shader. -func (i *Image) dependsOnShader(shader *Shader) bool { - return false -} - -// dependingImages returns all images that is depended on the image. -func (i *Image) dependingImages() map[*Image]struct{} { - r := map[*Image]struct{}{} - return r -} - -// hasDependency returns a boolean value indicating whether the image depends on another image. -func (i *Image) hasDependency() bool { - return false -} - -// Restore restores *graphicscommand.Image from the pixels using its state. -func (i *Image) restore(graphicsDriver graphicsdriver.Graphics) error { - w, h := i.width, i.height - // Do not dispose the image here. The image should be already disposed. - - switch i.imageType { - case ImageTypeScreen: - // The screen image should also be recreated because framebuffer might - // be changed. - i.image = graphicscommand.NewImage(w, h, true) - i.basePixels.Dispose() - i.basePixels = Pixels{} - i.stale = false - i.staleRegions = i.staleRegions[:0] - return nil - case ImageTypeVolatile: - i.image = graphicscommand.NewImage(w, h, false) - iw, ih := i.image.InternalSize() - clearImage(i.image, image.Rect(0, 0, iw, ih)) - return nil - } - - if i.stale { - panic("restorable: pixels must not be stale when restoring") - } - - gimg := graphicscommand.NewImage(w, h, false) - // Clear the image explicitly. - iw, ih := gimg.InternalSize() - clearImage(gimg, image.Rect(0, 0, iw, ih)) - - i.basePixels.Apply(gimg) - - i.image = gimg - i.stale = false - i.staleRegions = i.staleRegions[:0] - return nil -} - // Dispose disposes the image. // // After disposing, calling the function of the image causes unexpected results. @@ -364,11 +180,6 @@ func (i *Image) Dispose() { theImages.remove(i) i.image.Dispose() i.image = nil - i.basePixels.Dispose() - i.basePixels = Pixels{} - i.pixelsCache = nil - i.stale = false - i.staleRegions = i.staleRegions[:0] } func (i *Image) Dump(graphicsDriver graphicsdriver.Graphics, path string, blackbg bool, rect image.Rectangle) (string, error) { @@ -378,30 +189,3 @@ func (i *Image) Dump(graphicsDriver graphicsdriver.Graphics, path string, blackb func (i *Image) InternalSize() (int, int) { return i.image.InternalSize() } - -// appendRegionRemovingDuplicates adds a region to a given list of regions, -// but removes any duplicate between the newly added region and any existing regions. -// -// In case the newly added region is fully contained in any pre-existing region, this function does nothing. -// Otherwise, any pre-existing regions that are fully contained in the newly added region are removed. -// -// This is done to avoid unnecessary reading pixels from GPU. -func appendRegionRemovingDuplicates(regions *[]image.Rectangle, region image.Rectangle) { - for _, r := range *regions { - if region.In(r) { - // The newly added rectangle is fully contained in one of the input regions. - // Nothing to add. - return - } - } - // Separate loop, as regions must not get mutated before above return. - n := 0 - for _, r := range *regions { - if r.In(region) { - continue - } - (*regions)[n] = r - n++ - } - *regions = append((*regions)[:n], region) -} diff --git a/internal/restorable/images.go b/internal/restorable/images.go index c8d608f6c..adcc9469b 100644 --- a/internal/restorable/images.go +++ b/internal/restorable/images.go @@ -22,9 +22,8 @@ import ( // images is a set of Image objects. type images struct { - images map[*Image]struct{} - shaders map[*Shader]struct{} - lastTarget *Image + images map[*Image]struct{} + shaders map[*Shader]struct{} } // theImages represents the images for the current process. @@ -71,42 +70,13 @@ func (i *images) addShader(shader *Shader) { // remove removes img from the images. func (i *images) remove(img *Image) { - i.makeStaleIfDependingOn(img) delete(i.images, img) } func (i *images) removeShader(shader *Shader) { - i.makeStaleIfDependingOnShader(shader) delete(i.shaders, shader) } -// makeStaleIfDependingOn makes all the images stale that depend on target. -// -// When target is modified, all images depending on target can't be restored with target. -// makeStaleIfDependingOn is called in such situation. -func (i *images) makeStaleIfDependingOn(target *Image) { - if target == nil { - panic("restorable: target must not be nil at makeStaleIfDependingOn") - } - if i.lastTarget == target { - return - } - i.lastTarget = target - for img := range i.images { - img.makeStaleIfDependingOn(target) - } -} - -// makeStaleIfDependingOn makes all the images stale that depend on shader. -func (i *images) makeStaleIfDependingOnShader(shader *Shader) { - if shader == nil { - panic("restorable: shader must not be nil at makeStaleIfDependingOnShader") - } - for img := range i.images { - img.makeStaleIfDependingOnShader(shader) - } -} - var graphicsDriverInitialized bool // InitializeGraphicsDriverState initializes the graphics driver state. diff --git a/internal/restorable/pixelrecords.go b/internal/restorable/pixelrecords.go deleted file mode 100644 index b97446d4f..000000000 --- a/internal/restorable/pixelrecords.go +++ /dev/null @@ -1,159 +0,0 @@ -// Copyright 2019 The Ebiten 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. - -package restorable - -import ( - "fmt" - "image" - - "github.com/hajimehoshi/ebiten/v2/internal/graphics" - "github.com/hajimehoshi/ebiten/v2/internal/graphicscommand" -) - -type pixelsRecord struct { - rect image.Rectangle - pix *graphics.ManagedBytes -} - -func (p *pixelsRecord) readPixels(pixels []byte, region image.Rectangle, imageWidth, imageHeight int) { - r := p.rect.Intersect(region.Intersect(image.Rect(0, 0, imageWidth, imageHeight))) - if r.Empty() { - return - } - - dstBaseX := r.Min.X - region.Min.X - dstBaseY := r.Min.Y - region.Min.Y - lineWidth := 4 * r.Dx() - if p.pix != nil { - srcBaseX := r.Min.X - p.rect.Min.X - srcBaseY := r.Min.Y - p.rect.Min.Y - for j := 0; j < r.Dy(); j++ { - dstX := 4 * ((dstBaseY+j)*region.Dx() + dstBaseX) - srcX := 4 * ((srcBaseY+j)*p.rect.Dx() + srcBaseX) - p.pix.Read(pixels[dstX:dstX+lineWidth], srcX, srcX+lineWidth) - } - } else { - for j := 0; j < r.Dy(); j++ { - dstX := 4 * ((dstBaseY+j)*region.Dx() + dstBaseX) - for i := 0; i < lineWidth; i++ { - pixels[i+dstX] = 0 - } - } - } -} - -type pixelsRecords struct { - records []*pixelsRecord -} - -func (pr *pixelsRecords) addOrReplace(pixels *graphics.ManagedBytes, region image.Rectangle) { - if pixels.Len() != 4*region.Dx()*region.Dy() { - msg := fmt.Sprintf("restorable: len(pixels) must be 4*%d*%d = %d but %d", region.Dx(), region.Dy(), 4*region.Dx()*region.Dy(), pixels.Len()) - if pixels == nil { - msg += " (nil)" - } - panic(msg) - } - - // Remove or update the duplicated records first. - var n int - for _, r := range pr.records { - if r.rect.In(region) { - continue - } - pr.records[n] = r - n++ - } - for i := n; i < len(pr.records); i++ { - pr.records[i] = nil - } - pr.records = pr.records[:n] - - // Add the new record. - pr.records = append(pr.records, &pixelsRecord{ - rect: region, - pix: pixels, - }) -} - -func (pr *pixelsRecords) clear(region image.Rectangle) { - if region.Empty() { - return - } - - var n int - var needsClear bool - for _, r := range pr.records { - if r.rect.In(region) { - continue - } - if r.rect.Overlaps(region) { - needsClear = true - } - pr.records[n] = r - n++ - } - for i := n; i < len(pr.records); i++ { - pr.records[i] = nil - } - pr.records = pr.records[:n] - if needsClear { - pr.records = append(pr.records, &pixelsRecord{ - rect: region, - }) - } -} - -func (pr *pixelsRecords) readPixels(pixels []byte, region image.Rectangle, imageWidth, imageHeight int) { - for i := range pixels { - pixels[i] = 0 - } - for _, r := range pr.records { - r.readPixels(pixels, region, imageWidth, imageHeight) - } -} - -func (pr *pixelsRecords) apply(img *graphicscommand.Image) { - // TODO: Isn't this too heavy? Can we merge the operations? - for _, r := range pr.records { - if r.pix != nil { - // Clone a ManagedBytes as the package graphicscommand has a different lifetime management. - img.WritePixels(r.pix.Clone(), r.rect) - } else { - clearImage(img, r.rect) - } - } -} - -func (pr *pixelsRecords) appendRegions(regions []image.Rectangle) []image.Rectangle { - for _, r := range pr.records { - if r.rect.Empty() { - continue - } - regions = append(regions, r.rect) - } - return regions -} - -func (pr *pixelsRecords) dispose() { - for _, r := range pr.records { - if r.pix == nil { - continue - } - // As the package graphicscommands already has cloned ManagedBytes objects, it is OK to release it. - _, f := r.pix.GetAndRelease() - f() - } -} diff --git a/internal/restorable/rect_test.go b/internal/restorable/rect_test.go deleted file mode 100644 index 27f094270..000000000 --- a/internal/restorable/rect_test.go +++ /dev/null @@ -1,165 +0,0 @@ -// Copyright 2023 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. - -package restorable_test - -import ( - "image" - "testing" - - "github.com/hajimehoshi/ebiten/v2/internal/restorable" -) - -func areEqualRectangles(a, b []image.Rectangle) bool { - if len(a) != len(b) { - return false - } - - for i := range a { - if a[i] != b[i] { - return false - } - } - - return true -} - -func TestRemoveDuplicatedRegions(t *testing.T) { - cases := []struct { - Regions []image.Rectangle - NewRegions []image.Rectangle - Expected []image.Rectangle - }{ - { - Regions: nil, - NewRegions: nil, - Expected: nil, - }, - { - NewRegions: []image.Rectangle{ - image.Rect(0, 0, 2, 2), - }, - Expected: []image.Rectangle{ - image.Rect(0, 0, 2, 2), - }, - }, - { - NewRegions: []image.Rectangle{ - image.Rect(0, 0, 2, 2), - image.Rect(0, 0, 1, 1), - }, - Expected: []image.Rectangle{ - image.Rect(0, 0, 2, 2), - }, - }, - { - NewRegions: []image.Rectangle{ - image.Rect(0, 0, 1, 1), - image.Rect(0, 0, 2, 2), - }, - Expected: []image.Rectangle{ - image.Rect(0, 0, 2, 2), - }, - }, - { - NewRegions: []image.Rectangle{ - image.Rect(0, 0, 1, 3), - image.Rect(0, 0, 2, 2), - image.Rect(0, 0, 3, 1), - }, - Expected: []image.Rectangle{ - image.Rect(0, 0, 1, 3), - image.Rect(0, 0, 2, 2), - image.Rect(0, 0, 3, 1), - }, - }, - { - NewRegions: []image.Rectangle{ - image.Rect(0, 0, 1, 3), - image.Rect(0, 0, 2, 2), - image.Rect(0, 0, 3, 1), - image.Rect(0, 0, 4, 4), - }, - Expected: []image.Rectangle{ - image.Rect(0, 0, 4, 4), - }, - }, - { - NewRegions: []image.Rectangle{ - image.Rect(0, 0, 1, 3), - image.Rect(0, 0, 2, 2), - image.Rect(0, 0, 3, 1), - image.Rect(0, 0, 4, 4), - image.Rect(1, 1, 2, 2), - }, - Expected: []image.Rectangle{ - image.Rect(0, 0, 4, 4), - }, - }, - { - NewRegions: []image.Rectangle{ - image.Rect(0, 0, 1, 3), - image.Rect(0, 0, 2, 2), - image.Rect(0, 0, 3, 1), - image.Rect(0, 0, 4, 4), - image.Rect(0, 0, 5, 5), - }, - Expected: []image.Rectangle{ - image.Rect(0, 0, 5, 5), - }, - }, - { - Regions: []image.Rectangle{ - image.Rect(0, 0, 1, 3), - image.Rect(0, 0, 2, 2), - image.Rect(0, 0, 3, 1), - image.Rect(0, 0, 4, 4), - }, - NewRegions: []image.Rectangle{ - image.Rect(0, 0, 5, 5), - }, - Expected: []image.Rectangle{ - image.Rect(0, 0, 5, 5), - }, - }, - { - Regions: []image.Rectangle{ - image.Rect(0, 0, 2, 2), - image.Rect(0, 0, 3, 1), - image.Rect(0, 0, 4, 4), - image.Rect(0, 0, 5, 5), - }, - NewRegions: []image.Rectangle{ - image.Rect(0, 0, 1, 3), - }, - Expected: []image.Rectangle{ - image.Rect(0, 0, 2, 2), - image.Rect(0, 0, 3, 1), - image.Rect(0, 0, 4, 4), - image.Rect(0, 0, 5, 5), - }, - }, - } - - for _, c := range cases { - got := c.Regions - for _, r := range c.NewRegions { - restorable.AppendRegionRemovingDuplicates(&got, r) - } - want := c.Expected - if !areEqualRectangles(got, want) { - t.Errorf("restorable.RemoveDuplicatedRegions(%#v): got: %#v, want: %#v", c.NewRegions, got, want) - } - } -}