ebiten icon indicating copy to clipboard operation
ebiten copied to clipboard

Allow different size image sources for Kage

Open hajimehoshi opened this issue 3 years ago • 3 comments

I'm not sure we could do it.

hajimehoshi avatar Nov 10 '21 03:11 hajimehoshi

CC @SolarLune

hajimehoshi avatar Nov 10 '21 03:11 hajimehoshi

Hello!

I asked this in connection with my 3D software renderer, so it's a bit of a niche use case. Instead, if we were to look at this request from a general perspective, I could see people needing to, say, composite two images of different sizes in a shader (like, perhaps, an image of a sprite sheet and some kind of detail or color map for switching palettes). If it's not possible currently, that's OK - there are ways to work around it, so it's not critical, but I think I could see the utility if it wouldn't drastically hurt performance or API readability.

SolarLune avatar Nov 10 '21 21:11 SolarLune

Sure, let me de-prioritize this. Thanks,

hajimehoshi avatar Nov 11 '21 02:11 hajimehoshi

Now we added the pixel mode (#1431), it should be easy to treat different sizes of source images.

hajimehoshi avatar Apr 24 '23 14:04 hajimehoshi

This might conflict with the idea of utility functions of normalization (#2644)

hajimehoshi avatar Apr 24 '23 14:04 hajimehoshi

Hmm, the biggest problem is that the current design of imageSrcNAt does some tricks and treats all the source images as if they are on the same region. This hack doesn't match with the change for source image sizes.

Probably we have to deprecate imageSrcNAt and introduce a new API to expose different regions of source images, but this would degrade the usability. With the pixel mode, the usability would not so change, but I am not sure.

hajimehoshi avatar Apr 25 '23 13:04 hajimehoshi

Just mentioning that, as I exposed on #2646 and other places, I consider separate imageNSize() functions and less magic from imageSrcNAt() a positive change, even if we end up having "more functions". I don't think the usability is impacted in any significant way (the necessary API deprecations and changes are the most painful part, but that's life). Maybe others think differently, but personally I'm in favor of the less magical, more explicit model where each image/texture has its own functions/properties.

tinne26 avatar Apr 25 '23 14:04 tinne26

So

imageSrc1At(texCoord)

will be

// Texels
newImageSrc1At(((texCoord - src0Origin) * src0Size) / src1Size + src1Origin)

// Pixels
newImageSrc1At(texCoord - src0Origin + src1Origin)

I'm not sure this is acceptable. If we didn't have the pixel mode, I would deny this idea.

hajimehoshi avatar Apr 25 '23 14:04 hajimehoshi

I consider separate imageNSize() functions and less magic from imageSrcNAt() a positive change, even if we end up having "more functions".

I generally agree; the simpler and less "magic" is done, the better. However, ideally, that's not because the function that did the magic is broken up into smaller functions, but rather that the magic is no longer necessary.

So

imageSrc1At(texCoord)

will be

// Texels
newImageSrc1At(((texCoord - src0Origin) * src0Size) / src1Size + src1Origin)

// Pixels
newImageSrc1At(texCoord - src0Origin + src1Origin)

I'm not sure this is acceptable. If we didn't have the pixel mode, I would deny this idea.

I thought that it was decided to turn off atlasing when shaders were used; if so, then you wouldn't need any origin manipulation, right?


As for the concept of arbitrary texture sizes in Kage, allow me to make a case for it.

Support for arbitrary input texture sizes in a shader system is an inherent, relatively basic feature that is used heavily in shaders. There's many use cases where it's useful; for example, if you render a sprite (one ~~uniform~~ source image) and read from a screen buffer (another ~~uniform~~ source image). This might be if you were rendering a sprite on top of walls or other objects to maintain visibility, like this:

renderOnTop

Another use case would be if you render a sprite (one ~~uniform~~ source image) and read from a noise texture (another ~~uniform~~ source image) - this might be to dissolve a sprite over time, for example, like this:

dissolveEffects

(The above example probably uses mathematical functions rather than a noise texture to get the effect, but you could do the same thing with a texture.)

I believe this is a fairly important issue now because the alternative basically means wasting CPU / GPU time and memory. You have to maintain additional buffer images and/or render textures to large buffers to ensure buffer sizes are the same. This effectively hamstrings the shader system dramatically for any non-trivial and real-world usages.

SolarLune avatar Apr 25 '23 16:04 SolarLune

Quick reply:

I thought that it was decided to turn off atlasing when shaders were used; if so, then you wouldn't need any origin manipulation, right?

No we have not decided yet. And, we were talking about atlasing for a destination image, not a source image. Unatlasing a destination image would not solve the issue.

hajimehoshi avatar Apr 25 '23 16:04 hajimehoshi

I think before anything else is done, we need to come to a conclusion on texCoord usage, and I think I'd just turn off atlasing for all source and destination textures. That's a discussion for another issue, though.

SolarLune avatar Apr 25 '23 16:04 SolarLune

I think we have never discussed about atlasing source images. This sounds a new topic. If you have a suggestion about this, please file a new issue or a new discussion. Thanks,

hajimehoshi avatar Apr 25 '23 16:04 hajimehoshi

My current idea is to allow different source sizes only for the pixel mode.

  1. Use imageSrcNAt. This still does tricks for N >= 1 images, but the trick is just adsjuting the offsets. Actually this is already done.
  2. Introduce a new low-level API like imageSrcNRawAt (?) which doesn't do tricks for N >= 1. This requires an explicit trick for the user side like imageSrc1RawAt(texCoord - src0Origin + src1Origin)

We can discuss 2. later. There is no action item for 1. as this is already done. So, simply allowing different sized source images for the pixel mode is what we have to do.

My concern is that the notion of 'normalization' of pixels to the range [0, 1] will be more complicated, if we work on #2644 , but I don't have a clear idea about this.

hajimehoshi avatar Apr 26 '23 02:04 hajimehoshi

Use imageSrcNAt. This still does tricks for N >= 1 images, but the trick is just adsjuting the offsets. Actually this is already done.

We can discuss 2. later. There is no action item for 1. as this is already done. So, simply allowing different sized source images for the pixel mode is what we have to do.

This sounds perfectly understandable; thank you for working with me on this!

EDIT: Also, thanks for the clarification on atlasing, I must have been mistaken.

SolarLune avatar Apr 26 '23 18:04 SolarLune

@SolarLune Do you think it is ok to allow various source sizes only for DrawTrianglesShader? DrawRectShader is a little special and would break some assumptions if it accepted various sized sources (https://github.com/hajimehoshi/ebiten/issues/2166#issuecomment-1658765390)

hajimehoshi avatar Jul 31 '23 16:07 hajimehoshi

I think that would be fine, yeah. Of course, the documentation should mention the difference, but if it's only feasible that way, then that would be the way to go.

SolarLune avatar Aug 01 '23 16:08 SolarLune

Note to myself:

  • The first action item is to change var __textureSourceRegionSize vec2 to var __textureSourceRegionSizes [4]vec2 .
  • How can I fix imageSrcRegionOnTexture()?
    • Now, imageSrcNAt automatically adjusts the given offset when N >= 1, as if all the source images are on the same region. However, if we allow different sizes, this assumption is no longer valid.
    • With the pixel mode, imageSrcNRegionOnTexture() might be better?
  • The easiest (but dumbest?) solution is to let users use imageSrcNUnsafeAt for different size images. imageSrcRegionOnTexture keeps to return the 0th image's info.

hajimehoshi avatar Aug 22 '23 15:08 hajimehoshi

Assuming different size source images are available only with the pixel mode,

  • imageSrcNAt returns a texture color for the given position. If N >= 1, the position is automatically adjusted, as if all the image's left-upper position is the same as 0th image's. Nth source image's bondary is checked.
  • imageSrcNUnsafeAt returns a texture color for the given position. If N >= 1, the position is automatically adjusted. This works without a boundary check.
  • imageSrcRegionOnTexture returns the 0th image's region on its texture for compatibility.
  • A new function imageSrcNOrigin returns an offset of nth image on a texture in pixels or texels (this depends on modes).
    • I'm worried that this function might be confusing. When a user wants to use imageSrcNAt, a user might think imageSrcNOrigin is necessary. Actually this is not necessary as imageSrcNAt automatically adjusts the given position. I might not provide this function.
  • A new function imageSrcNSize returns a size of nth image on a texture in pixels or texels (this depends on modes).
  • New functions imageDstOrigin and imageDstSize might be provided for consistency.

Any thoughts?

hajimehoshi avatar Aug 22 '23 17:08 hajimehoshi

I think it's reasonable within the current API. I also think it shows how the API for working with images in shaders needs an eventual revamp:

  • If we eventually get normalization functions, the origin functions are not necessary for the average user, in a way (or is there any other use besides normalizing positions to [0, 1] range?).
  • For advanced users we probably still want to offer those origin functions, but then I think the reasonable direction is more similar to what I proposed on #2646, to have a low-level set of functions and maybe name them imageSrcNAtlasOrigin or imageSrcNRawOrigin or whatever. But I think this is only a consideration for an eventual revamp of the whole set of functions. At the moment, I think doing this only for these functions would be counterproductive.

So, I think the friction and weirdness and potential confusion is unavoidable due to previous design decisions, which didn't have the benefit of the insight we have at the current point in time.

The only change I'd consider here is using imageSrcNTextureSize() instead of imageSrcNSize(), for consistency and to avoid adding imageDstSize() (which I'm assumming is the same thing as imageDstTextureSize() but with the new naming convention).

tinne26 avatar Aug 22 '23 17:08 tinne26

Thank you for feedbacks!

  • I'll keep imageSrcNAt's automatic offset conversion for compatibility.
  • I might add imageSrcNRawOrigin or something, but this would be a separate issue.

The only change I'd consider here is using imageSrcNTextureSize() instead of imageSrcNSize(), for consistency and to avoid adding imageDstSize()

My intention is that imageSrcNSize returns the region's size, not the texture size. So imageSrcNTextureSize doesn't make sense...

hajimehoshi avatar Aug 23 '23 13:08 hajimehoshi

Existing functions:

  • imageSrcNAt returns Nth source image's color. If N >= 1, the given offset is automatically adjusted.
  • imageDstAt returns a destionation image's color.
  • imageSrcRegionOnTexture returns 0th source image's region on its texture.
  • imageDstRegionOnTexture returns a destination image's region on its texture.

New functions:

  • imageSrcNOrigin returns Nth source image's region origin on its texture in pixels or texels. (This replaces imageSrcRegionOnTexture)
  • imageSrcNSize returns Nth source image's region size on its texture in pixels or texels. (This replaces imageSrcRegionOnTexture)
  • imageDstOrigin returns the destination image's region origin on its texture in pixels or texels. (This replaces imageDstRegionOnTexture)
  • imageDstSize returns the destination image's region size on its texture in pixels or texels. (This replaces imageDstRegionOnTexture)

hajimehoshi avatar Aug 23 '23 13:08 hajimehoshi