ebiten icon indicating copy to clipboard operation
ebiten copied to clipboard

ebiten: read-only `Image`

Open hajimehoshi opened this issue 2 years ago • 5 comments

Images from AppendGlyphs should not be modified since they are from a cache and might be reused.

To avoid problems, should we have a read-only property for an Ebiten image?

hajimehoshi avatar Apr 02 '22 09:04 hajimehoshi

As you yourself pointed out, many other variables in Golang are mutable, like color.White or unicode range tables. While many of us would like to be able to annotate some variables as read-only (myself first), the reality is that Golang's type system does not provide such guarantees for public types. I think trying to work around this isn't the way to go. ~~No pun intended.~~

Perhaps more importantly, we have to consider the following practical concerns:

  • Even if we had such a field, what would we do if the read-only property was violated? Neither panicking nor failing silently sound like we are making the situation any better. It's a different failure, but it's still bad.
  • If you allow read-only images, then you need to allow users to mark Ebiten images as read-only too. For example: they implement their own text renderer and have their own caches (...guilty). I don't think the scope of the current problem justifies the extra complexity or the addition of such a new method.

That being said, besides the recently fixed spec, what we could actually do is consider changing the name of the Image field in the Glyph struct to CachedMask, ReadOnlyMask, ReadOnlyImage or something like that that makes it more obvious that the image should not be modified. This would also make the code more self-documenting and would greatly reduce the chance of anything going wrong even if the spec is missed or forgotten.

tinne26 avatar Apr 02 '22 22:04 tinne26

I don't think the scope of the current problem justifies the extra complexity or the addition of such a new method.

I agree.

changing the name of the Image field in the Glyph struct to CachedMask, ReadOnlyMask, ReadOnlyImage or something like that

Hm, I'm not sure it is a good idea that a field that is named with ReadOnly but is actually writable... 🤔

hajimehoshi avatar Apr 03 '22 20:04 hajimehoshi

While this is a terrible idea right now, just for the sake of completeness I'll say that it would be possible to use generics to separate images into SourceImage and DestinationImage, and have functions like ReplacePixels and DrawImage exist only on DestinationImage, but allow SourceImage to also be used as a parameter to DrawImage. Then, the AppendGlyphs callback function would receive a SourceImage that naturally can't be modified because it doesn't have the methods for it.

Of course, this is a mess from a complexity perspective, mostly because for users it's hard to understand and pollutes the documentation (and it would massively break the API), but technically it's easy to implement and would solve the problem "cleanly". In case it inspires other solutions or v3 changes.

tinne26 avatar Jul 15 '22 20:07 tinne26

but allow SourceImage to also be used as a parameter to DrawImage

How would we pass a DestinationImage to DrawImage?

hajimehoshi avatar Jul 16 '22 05:07 hajimehoshi

That's the generics part, where you would have to define a type Image interface { DestinationImage | SourceImage } type or something like that. It's unclear that generics are mature enough in Golang, and this has many many problems as already stated.

tinne26 avatar Jul 16 '22 07:07 tinne26