ebiten: NewImageFromImage should respect the original image bounds

[tinne26]

NewImageFromImage will reset the bounds to start at (0, 0) instead of keeping the bounds of the given image, and no safe alternative mechanism exists to modify them afterwards.

I came across this while drawing font glyphs: in glyph masks, bounds can be used to indicate which parts of the glyph go above and below the baseline. More generally, we could say that bounds do not only delimit the area of the image being used, but also its origin (x, y) coordinates.

Ebiten bounds vs image.Image bounds

While exploring the issue more in depth I realized that Ebiten semantics for bounds are actually quite different from image.Image specs (from now on go/image). Reviewing the go/image definition:

type Image interface {
    // [...]
    // Bounds returns the domain for which At can return non-zero color.
    // The bounds do not necessarily contain the point (0, 0).
    Bounds() Rectangle
    // At returns the color of the pixel at (x, y).
    // At(Bounds().Min.X, Bounds().Min.Y) returns the upper-left pixel of the grid.
    // At(Bounds().Max.X-1, Bounds().Max.Y-1) returns the lower-right one.
    At(x, y int) color.Color
}

I want to highlight the following:

• Ebiten in practice only uses bounds as a mechanism to locate subimages within their original image, and trying to interpret them in any different way, as it currently stands, will simply not work.
• Despite the previous, Ebiten still complies with the spec reviewed above, but only because:
  1. All Ebiten bounds are aligned within the underlying image.
  2. All ebiten bounds are non-negative.
• If Ebiten was to allow arbitrary bounds, the contradictions and incompatibilities with go/image would become more obvious, including the fact that At() could panic despite the passed coordinates being within bounds. In particular, negative coordinates would always panic.
• If we were to assume that drawing an image with shifted bounds must change the drawing position, Ebiten would break in many ways (e.g: current behavior of subimages would change drastically). If we were to assume that drawing an image with shifted bounds must not change the drawing position, Ebiten would still break anyway (!!). Analyzing the behavior of subimages in DrawImage would illustrate these points, but it's obvious if we refer back to the first item in this list.

Conclusion

Despite what it may look like, the point of the previous section wasn't to bash Ebiten for doing wrong something it hasn't tried to do yet, but rather to highlight the severe discordances with go/image and conclude:

• Trying to get NewImageFromImage to respect original bounds would be a non-breaking change only if bounds didn't do anything when drawing... but as previously mentioned, this would still break Ebiten internally and require changes in the implementation of subimages. Comparing the cost (implementation work + at least one image.Point more in *ebiten.Image to control subimage offset from original) and the benefits (users can access the bounds as they originally were) doesn't look too fun.
• Ebiten should determine what's its relationship with bounds (and update documentation accordingly to avoid more confusion):
  • Compliant, but firmly staying within the current restrictions (and acknowledging the limitations this might entail).
  • Compliant and removing the current limits. This involves rethinking subimages and maybe considering whether bounds can change the drawing position (and how and when are those translations applied!).
  • Rejecting go/image compliance and focusing on actual usage of *ebiten.Image instead. Given that specific performance considerations need to be taken into account anyway (e.g: At()), maybe some of the methods like ColorModel() and Bounds() simply are not justified in Ebiten. What are the advantages of complying with go/image?

[hajimehoshi]

Quick reply:

If Ebiten was to allow arbitrary bounds, the contradictions and incompatibilities with go/image would become more obvious, including the fact that At() could panic despite the passed coordinates being within bounds. In particular, negative coordinates would always panic.

In general, At should return 0 color instead of panicking when outside is accessed.

[hajimehoshi]

While exploring the issue more in depth I realized that Ebiten semantics for bounds are actually quite different from image.Image specs (from now on go/image). Reviewing the go/image definition:

I'm not sure what the differences between Ebiten's image and go/image are. go/image doesn't promise so much things and Ebiten's image should satisfy all the conditions, right?

For Draw*'s behavior, I understand there are some inconsistencies, but this is outside of what go/image defines.

[tinne26]

Yes, as I wrote, "Ebiten still complies with the spec reviewed above". But then I highlighted that it only complies due to a very specific set of restrictions in the ways Ebiten operates. I make the point that trying to fix NewImageFromImage by keeping the original image bounds, for example, would break these restrictions and make it obvious that Ebiten can't comply, under the current implementation, with a "more liberal" use of bounds. At() panicking was just an example of breakage under this hypothetical scenario (very simple to fix, actually, unlike other parts). I simply went on to explain how these discordances are not minor, subtle, edge-case bugs in a function or two, but rather structural: Ebiten is only using bounds as a tool for subimages... and when images are aligned to (0, 0) this happens to comply with go/image, but trying to go one step in any other direction (still within go/image spec) will showcase that many things won't work.

At first glance it may seem vain to focus on these "hypotheticals", but I argue that we can't even classify NewImageFromImage as a bug until we clarify what's Ebiten's position with regards to bounds... and that we can only decide Ebiten's position with regards to bounds once we are aware of all these discordances with go/image (not what is broken right now, but what would break if).

[hajimehoshi]

So, I cannot see the reason why we should keep the original bounds yet (I think you just suggest the possibility). I think I can change NewImageFromImage and Image implementation without breaking at Draw* behaviors, as Draw* doesn't respect the left-upper position anyway. (Even though the left-upper position is non-zero, Draw* treats the image as if the image's left-upper position is (0, 0)).

[hajimehoshi]

My understanding is that you want to use Ebiten's images for other Go utilities, right? Then negative left-upper positions might be meaningful in this case.

[hajimehoshi]

I realized the case when this breaks the compatibility: when an image created by NewImageFromImage is used as a rendering target, the bounds matter.

So, unfortunately I hesitate to introduce this change. I'd like to revisit when the version becomes v3.0, or if this is urgent, I'll consider to add a new function besides NewImageFromImage.

[tinne26]

It's ok, we can leave it for v3, I'll use a wrapper in the meantime.

[hajimehoshi]

Should NewImage take an image.Rectangle like what the standard image package does? i.e. in v3, should NewImageFromRect replaces NewImage?

[hajimehoshi]

See also #2017 and #2124. We are adding NewImageWithOptions and NewImageFromImageWithOptions.