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

Allow different size image sources for Kage #1870

Closed
hajimehoshi opened this issue Nov 10, 2021 · 21 comments
Closed

Allow different size image sources for Kage #1870

hajimehoshi opened this issue Nov 10, 2021 · 21 comments

Comments

@hajimehoshi
Copy link
Owner

I'm not sure we could do it.

@hajimehoshi hajimehoshi added this to the v2.3.0 milestone Nov 10, 2021
@hajimehoshi
Copy link
Owner Author

CC @SolarLune

@SolarLune
Copy link

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.

@hajimehoshi hajimehoshi modified the milestones: v2.3.0, v2.4.0 Nov 11, 2021
@hajimehoshi
Copy link
Owner Author

Sure, let me de-prioritize this. Thanks,

@hajimehoshi hajimehoshi modified the milestones: v2.4.0, v2.5.0 Aug 7, 2022
@hajimehoshi hajimehoshi modified the milestones: v2.5.0, v2.6.0 Nov 4, 2022
@hajimehoshi
Copy link
Owner Author

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

@hajimehoshi
Copy link
Owner Author

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

@hajimehoshi
Copy link
Owner Author

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.

@tinne26
Copy link

tinne26 commented Apr 25, 2023

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.

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Apr 25, 2023

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.

@SolarLune
Copy link

SolarLune commented Apr 25, 2023

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.

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Apr 25, 2023

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.

@SolarLune
Copy link

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.

@hajimehoshi
Copy link
Owner Author

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
Copy link
Owner Author

hajimehoshi commented Apr 26, 2023

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.

@SolarLune
Copy link

SolarLune commented Apr 26, 2023

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.

@hajimehoshi
Copy link
Owner Author

@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 (#2166 (comment))

@SolarLune
Copy link

SolarLune commented Aug 1, 2023

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.

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Aug 22, 2023

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
Copy link
Owner Author

hajimehoshi commented Aug 22, 2023

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?

@tinne26
Copy link

tinne26 commented Aug 22, 2023

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 Thoughts on the Kage API #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).

@hajimehoshi
Copy link
Owner Author

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
Copy link
Owner Author

hajimehoshi commented Aug 23, 2023

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 added a commit that referenced this issue Aug 27, 2023
This is a preparation to specify different sizes of source images.

Updates #1870
hajimehoshi added a commit that referenced this issue Aug 27, 2023
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

3 participants