ebiten: internal/shader: possible breaking changes in `position` in Kage between v2.4 and v2.5

So, do you all agree that automatic convertingposition and texCoord to [0, 1] is not feasible? One of the reasons is that this forces to use uniform variables in a shader program and then affects batching. As I suggested in the document, utility functions to normalize values should be fine.

So, this is my first main point: position can be useful, and it should be. The current v2.5 underspecified behavior goes pretty heavily against this. And I would definitely not want position to disappear.

Definitely, position is highly useful in shaders.

I want to highlight that the fact that SolarLune used a separate unmanaged offscreen to get around the quirks affecting position should not be treated as a story of success (e.g. “ok something broke but it was quickly fixed”). The fix may not take a lot of code, but requires knowledge about Ebitengine internals and requires complicated explanations (which are basically: let’s create a situation where we have the same context as we used to in v2.4). I don’t think this is ok for the average user.

Agreed, yeah - creating an unmanaged image isn’t hard, of course, but it shouldn’t be “the fix” to make position work consistently or “knowably”. Ideally, the user shouldn’t have to think about it.

Then again, if making a texture unmanaged is the only way to have position work reliably, then that would be fine, I think, if it were properly documented (or, better yet, if it was done automatically).

I think a good next step is writing down all the tricky cases, because they are too many and they are too tricky.

This probably is a good way forward - defining what the issue is, clearly, should help make things simpler to approach. The alternative would be to start from the end and work backwards - what do we want position and texCoords to represent, and how do we get that?

And I think @hajimehoshi also agrees, but is replying in terms of implementation

Yes, you’re right - thanks for working with us on this, @hajimehoshi - I know it’s not easy, especially when we aren’t (or at least I am not) really familiar with the internals of Ebitengine’s workings that would make this or other issues difficult or even impossible to resolve.

imageSrcNAt should not do such magic.

That’s how texture sampling works already for GLSL (and I’d imagine other shader languages), though, if I’m correct…? For example, GLSL’s texture() function works with texels, as I recall.

@Zyko0 / @tinne26 - Pinging to get you guys’ suggestions.

I’m afraid I’m against it.

1. A little overhead is fine when we’re talking about custom shaders, I think - it’s not a feature everyone would use, and it already should be offloading work from the CPU to the GPU, or only doing what the GPU can do. If it was a bit slower to send data to the GPU because of conversions, I think it would be worth the tradeoff (though doing some testing and seeing how much of a tradeoff would be nice).
2. It should always match with the lower-right point. imageSrcNAt should work with a normalized texture coordinate (0 to 1) range.
3. The range being 0 to 1 is more intuitive than the current alternative, which is that the range is unknown, essentially, and that you have to use functions to convert it to a usable, normalized range.
4. It’s up to you. I would probably rather it worked like other shading languages, but if it’s easier to you to do, then that’s fine.

So you mean the upper-left point should always be (0, 0)?

Yes.

I don’t mind too much what you end up choosing for this, but I just want it to be simple, and for position and texCoord to, ideally, be usable directly without having to convert or change them (basically, I think most users end up using image(Src/Dst)TextureSize() / image(Src/Dst)RegionOnTexture() just to get position or texCoord set up to sample a texture; this should, ideally, no longer be necessary). I probably wouldn’t mind if you could do simple debugging, but given that you generally can’t for shader programs, it’s imperative that the building blocks (position, texCoord, etc) work extremely consistently, predictably, and simply.

Oh, and if resolving this also allows for using images of varying sizes in shaders, that would also be good.

We discussed this on the Discord, but yes, I believe it probably would be best to simply turn off texture atlasing when using shaders. That way position and texCoord do what they’re supposed to do.

The only real alternative would be to fix it so that position and texCoord always work properly; if that’s not realistic, then just turning off atlasing seems to be fine.

EDIT: To be clear, I mean that Ebitengine should automatically turn off atlasing for textures when using them as source or destination images with shaders.

I agree that reverting v2.5 to behave like v2.4 (no atlasing) is the most immediate patch for this issue.

This doesn’t change the big problem, which is that input parameters are too hard to use, basically in any mode, and any tutorial, even with the new proposed functions to make life easier, kinda requires an initial setup and explanation and a “basically, you need to use these functions anyway because otherwise this is too difficult to use unless you spend quite a while reading and understanding the doc that explains how all this works internally”. To me, this means we need to discuss more in depth where we want to go and figure out how to make it nicer for the average user, even if it requires a //v2, or a later breaking change or some other directive. But before adding any directive like that, I think we need to discuss all this in a separate issue to make the changes really future proof. I think your proposal with the pixel mode directive and the new functions “works”, but it’s not as easy to use or pleasant as it should be in the long run (imo). So I’m skeptical of starting to add compiler directives to “quick-fix” multiple of the currently open issues in a way that feels somewhat hacky.

I vote for reverting to v2.4 behavior and opening a new issue to discuss the model we want to go towards. Unless you really feel that pixel mode and the new proposed functions are actually satisfactory in the long run, of course.

I’m afraid I disagree with this. For example, such a breaking change would make it extremely difficult for users to experiment multiple versions of Ebitengine to find some issues. Users are sometimes forced to use old versions of Ebitengine when a new Ebitengine has some issues.

The fact that “Kage” is a separate language, and quite a distinct fonctionnality, that shader files are “assets” makes this a fair point to me. Otherwise, I would favor breaking change too.

Thinking about a “mode” and a “directive” I’d still prefer going with a “version” though 👀 (where default/legacy are v1 and v2 only supports pixels). Although it sounds like semantics, it allows to slowly deprecate the old version with “texels”. While if you have a “mode”, you’re officially supporting two Kage shader modes for the current set of features, and the potential new features to come.

Also, whether you go with a mode or a version, I think we still need a way to encourage users to write their new shaders in “pixel units”. Which again, I think is more encouraging by specifying that the shader is “v2” as opposed to “//kage-mode:pixels”. Users will probably be more willing to set the latest Kage version in their file, rather than pick a new “mode”.

If the fact that users might be confused about what Kage shader is in pixels or in texels (just from a “version”), I do not think that’s an issue, the fact that it breaks with the wrong directive does, and the reason being that units are in texels or pixels could be an internal detail.

So, do you all agree that automatic convertingposition and texCoord to [0, 1] is not feasible? One of the reasons is that this forces to use uniform variables in a shader program and then affects batching. As I suggested in the document, utility functions to normalize values should be fine.

If it’s not feasible or would hurt performance, then it isn’t the ideal solution. You know better than us if it’s possible, of course, but I’ll follow you lead on that one.

I think switching to pixels for position sounds like a reasonable compromise that wouldn’t be hard to deal with for the user, while producing reliable results and being feasible to implement internally (at least, from my point of view).

I do, though, feel like pixel mode should be the default without a directive; the texel mode can be the directive. If shaders break between the previous release of ebiten and the current one, and users have to update anyway, they might as well update in the most future-proof way possible, and not to have the default be broken.

@tinne26 I’m still writing the document, but I hope the current summery of Ebitengine’s implementation would help you to understand:

https://docs.google.com/document/d/1lJTKxMqyyO-LxNS4KmuNakQwlhdBgzkmP2LNwpJk5bg/edit#bookmark=id.eglkzn1frx1r

EDIT: I added my proposal. I appreciate you folks’ feedbacks.

I think a good next step is writing down all the tricky cases, because they are too many and they are too tricky. It’s not just that we don’t know how to make position simpler, it’s that no one has a clear view of what happens with the shader parameters on all the permutations of API calls and configurations. E.g.: subimages can affect shader inputs, source image atlases can affect shader inputs, target atlases can affect shader inputs, DrawRectShader includes GeoM which can affect shader inputs, antialias can affect shader inputs, etc. This also kinda applies to texCoord to some degree. I think having such a list of “special cases” with the behavior detailed is a requirement before we can continue discussing this. Then we can go point by point saying which special cases seem acceptable and which ones don’t.

I’ll try to write a Google doc to summrize the current situation and my opinion. Stay tuned.

I agree with SolarLune that position and texCoord should be as predictable and have as few special cases as possible. That being said, I also understand why this may be hard or even impossible.

For me, the ideal situation would be for position to keep working in pixels but Ebitengine handles atlas offsets internally, preventing that from affecting the user. I don’t know whether this is possible, but my main point here is that allowing position to work with pixels and predictably means that position is a very practical input parameter. In fact, the whole intro tutorial of kage-desk revolves around this (as this was apparently a reasonable assumption before v2.5 arrived). Working with pixels is very valuable even on hiDPI-aware programs, as many sprites and effects can still be done at a specific size and only upscaled later as required.

So, this is my first main point: position can be useful, and it should be. The current v2.5 underspecified behavior goes pretty heavily against this. And I would definitely not want position to disappear.

Of course, having to do shaders outside atlases may become a performance problem, but I want to highlight that the fact that SolarLune used a separate unmanaged offscreen to get around the quirks affecting position should not be treated as a story of success (e.g. “ok something broke but it was quickly fixed”). The fix may not take a lot of code, but requires knowledge about Ebitengine internals and requires complicated explanations (which are basically: let’s create a situation where we have the same context as we used to in v2.4). I don’t think this is ok for the average user.

I think a good next step is writing down all the tricky cases, because they are too many and they are too tricky. It’s not just that we don’t know how to make position simpler, it’s that no one has a clear view of what happens with the shader parameters on all the permutations of API calls and configurations. E.g.: subimages can affect shader inputs, source image atlases can affect shader inputs, target atlases can affect shader inputs, DrawRectShader includes GeoM which can affect shader inputs, antialias can affect shader inputs, etc. This also kinda applies to texCoord to some degree. I think having such a list of “special cases” with the behavior detailed is a requirement before we can continue discussing this. Then we can go point by point saying which special cases seem acceptable and which ones don’t.

For example, if I try to think about the case where we pass multiple images and they are all the same size but are on different atlases and are subimages with different origins, my brain simply short-circuits. Like, yeah, maybe nothing weird will happen, but in v2.5, I wouldn’t dare say what will happen without actually trying it. Like, I see what you guys are discussing and I’m like “ok, I can’t even say anything yet because I don’t even know where we are right now”.