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

Rework for current FinalScreenDrawer design, allow the user to control the images monitored by the engine. #3166

Closed
4 of 11 tasks
sedyh opened this issue Dec 2, 2024 · 10 comments

Comments

@sedyh
Copy link
Contributor

sedyh commented Dec 2, 2024

Operating System

  • Windows
  • macOS
  • Linux
  • FreeBSD
  • OpenBSD
  • Android
  • iOS
  • Nintendo Switch
  • PlayStation 5
  • Xbox
  • Web Browsers

What feature would you like to be added?

The current solution to the problem of drawing shaders to the screen has some design flaws, in particular the FinalScreen interface, which lacks some Image capabilities due to implementation quirks, and also cannot be used by most image-receiving functions.

So, right now we have an offscreen for drawing FinalScreen:

type gameForUI struct {
    offscreen  *Image
}

Then, we have a loop that will constantly monitor screen size and recreating that offscreen:

func (c *context) layoutGame(outsideWidth, outsideHeight float64, deviceScaleFactor float64) (int, int) {
    if c.offscreen != nil && (c.offscreen.width != ow || c.offscreen.height != oh) {
    	c.offscreen.Deallocate()
    	c.offscreen = nil
    }
    if c.offscreen == nil {
    	c.offscreen = c.newOffscreenImage(ow, oh)
    }
}

Then, we have that draw call for it with some complicated logic for scaling probably because its different from a normal unmanaged image.

func (g *gameForUI) DrawFinalScreen(scale, offsetX, offsetY float64) {
  if d, ok := g.game.(FinalScreenDrawer); ok {
    d.DrawFinalScreen(g.screen, g.offscreen, geoM)
    return
  }
}
func (g *gameForUI) DrawFinalScreen(scale, offsetX, offsetY float64) {
  switch {
  case !screenFilterEnabled.Load(), math.Floor(scale) == scale:
  	op := &DrawImageOptions{}
  	op.GeoM = geoM
  	g.screen.DrawImage(g.offscreen, op)
  case scale < 1:
  	op := &DrawImageOptions{}
  	op.GeoM = geoM
  	op.Filter = FilterLinear
  	g.screen.DrawImage(g.offscreen, op)
  default:
  	op := &DrawRectShaderOptions{}
  	op.Images[0] = g.offscreen
  	op.GeoM = geoM
  	w, h := g.offscreen.Bounds().Dx(), g.offscreen.Bounds().Dy()
  	g.screen.DrawRectShader(w, h, g.screenShader, op)
  }
}

I suggest to deprecate all of that and make a function for creating unmanaged images that will be resized by ebitengine inside the lib, because its knows the best, when the resizing begin and end (could use slice with indicies instead of map).

type gameForUI struct {
    offscreens  map[int]*Image
}
func (g *gameForUI) NewOffscreen(id int) {
    imageType := atlas.ImageTypeUnmanaged
    if ui.Get().IsScreenClearedEveryFrame() {
	imageType = atlas.ImageTypeVolatile
    }
    g.offscreen = newImage(image.Rect(0, 0, width, height), imageType)
    if _, ok := g.offscreens[id]; !ok {
        g.offscreens[id] = g.offscreen.image
    }
}
func (g *gameForUI) DeallocateOffscreen(id int) {
    if v, ok := g.offscreens[id]; ok {
        v.Deallocate()
    }
}
func (g *gameForUI) Offscreen(id int) *ebiten.Image {
    return g.offscreens[id]
}

Which will be monitored just as previous image:

func (c *context) layoutGame(outsideWidth, outsideHeight float64, deviceScaleFactor float64) (int, int) {
    for _, offscreen := range c.offscreens {
      if offscreen != nil && (offscreen.width != ow || offscreen.height != oh) {
    	  offscreen.Deallocate()
    	  offscreen = nil
      }
      if offscreen == nil {
    	  offscreen = c.newOffscreenImage(ow, oh)
      }
    }
}

Not sure about how to pass offscreens from gameForUI to context, probably some getter will do the job.
I'd also like to suggest to prepend and preshrink the image while zooming begin in order to minimize the number of allocations.

Why is this needed?

The new solution will work with every ebitengine util functions and other third-party libraries functions.
It will allow to get rid of that closed interface that will take some effort to maintan in the future.

@hajimehoshi
Copy link
Owner

hajimehoshi commented Dec 2, 2024

I don't quite understand the benefits. Cloud you elaborate what kind of problem are you addressing on?

@sedyh
Copy link
Contributor Author

sedyh commented Dec 2, 2024

I don't quite understand the benefits. Cloud you elaborate what kind of problem are you addressing on?

Sure, It will allow to pass screen-sized ebiten.Image to any ebitengine function and third-party library.
You can't do that with the current FinalScreen image.

@hajimehoshi
Copy link
Owner

You can return such size at Layout, right?

@sedyh
Copy link
Contributor Author

sedyh commented Dec 2, 2024

You can return such size at Layout, right?

I mean, yes, it can be a separate library. But a separate library will not have access to the internal ui api which can be used to listen resize events. I believe that the engine knows better, when to resize it (even if its just cheking it with == right now).

There is also a point to get rid of that very strange implementation-closed interface which (in my opinion) can cause some problems with maintaining it in the future when new apis will be added to Image (which also is not compatible with most engine functions which is strange in design terms).

@hajimehoshi
Copy link
Owner

Please explain what the problem is, not how to solve it.

There is also a point to get rid of that very strange implementation-closed interface which (in my opinion) can cause some problems with maintaining it in the future when new apis will be added to Image (which also is not compatible with most engine functions which is strange in design terms).

The interface can add methods without breaking compatibility as the interface includes private methods so that no one other than ebiten package can implement the interface

@sedyh
Copy link
Contributor Author

sedyh commented Dec 2, 2024

Please explain what the problem is, not how to solve it.

I can't use FinalScreen with text/v2, vector, ebitenutil and all other engine functions that accepts Image, even if I convert it manually.

@hajimehoshi
Copy link
Owner

Ok I understood, but FinalDraw is not for such complicated rendering in the first place.

@hajimehoshi
Copy link
Owner

I think such libraries could accept an interface interested of Ebitengine but I'll consider it for v3 if I would do.

@sedyh
Copy link
Contributor Author

sedyh commented Dec 2, 2024

I think such libraries could accept an interface interested

Thats the problem with its design, that I mentioned. It cannot be implemented so we need to make full copy all the functions that accept Image. Thats why I'm suggesting another way to do it.

@hajimehoshi
Copy link
Owner

FinalDraw is a special function and my intention is not to provide a screen as a regular image to render texts or something. You should use regular Draw for that. So let me close this. If there is another problem, feel free to leave comments.

@hajimehoshi hajimehoshi closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants