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

ebiten: Create DrawImageWithShader function #1168

Closed
hajimehoshi opened this issue May 23, 2020 · 44 comments
Closed

ebiten: Create DrawImageWithShader function #1168

hajimehoshi opened this issue May 23, 2020 · 44 comments
Labels
Milestone

Comments

@hajimehoshi
Copy link
Owner

No description provided.

@hajimehoshi hajimehoshi added this to the v1.12.0 milestone May 23, 2020
hajimehoshi added a commit that referenced this issue May 27, 2020
Shader users should not have to care about the existence of uniform
variables.

Updates #1168
hajimehoshi added a commit that referenced this issue Jun 3, 2020
@hajimehoshi
Copy link
Owner Author

The current situation: https://gyazo.com/ac9b83238b1133310d5830d905b2bab4

package main

var Time float

// viewportSize is a predefined function.

func Vertex(position vec2, texCoord vec2, color vec4) vec4 {
	return mat4(
		2.0/viewportSize().x, 0, 0, 0,
		0, 2.0/viewportSize().y, 0, 0,
		0, 0, 1, 0,
		-1, -1, 0, 1,
	) * vec4(position, 0, 1)
}

func Fragment(position vec4) vec4 {
	pos := position.xy / viewportSize()
	color := 0.0
	color += sin(pos.x * cos(Time / 15.0) * 80.0) + cos(pos.y * cos(Time / 15.0) * 10.0)
	color += sin(pos.y * sin(Time / 10.0) * 40.0) + cos(pos.x * sin(Time / 25.0) * 40.0)
	color += sin(pos.x * sin(Time / 5.0) * 10.0) + sin(pos.y * sin(Time / 35.0) * 80.0)
	color *= sin(Time / 10.0) * 0.5
	return vec4(color, color * 0.5, sin(color + Time / 3.0 ) * 0.75, 1.0)
}

@gabstv
Copy link
Contributor

gabstv commented Jun 10, 2020

Nice! Is the Metal implementation still planned for v1.12? 🤞🏻

@Noofbiz
Copy link

Noofbiz commented Jun 10, 2020

Are you planning on making this shader language available outside of ebiten? I'm planning on trying to support Metal/Vulkan/DirectX as well, and having a go-syntax-style shader language that could work everywhere would be amazing!

@hajimehoshi
Copy link
Owner Author

Nice! Is the Metal implementation still planned for v1.12? 🤞🏻

Yes!

Are you planning on making this shader language available outside of ebiten?

Good question. No so far, because Ebiten requires a special hack to retrieve texels from textures (e.g., checking the region). In the future, I might fix this to be generalize for other game libraries, but I cannot guarantee.

@hajimehoshi hajimehoshi changed the title ebiten: Create a public API for shaders ebiten: Create DrawImageWithShader function Jun 11, 2020
@Noofbiz
Copy link

Noofbiz commented Jun 11, 2020

No worries, it’s just pretty cool stuff. I understand not wanting to maintain even more public facing APIs lol.

@ghost
Copy link

ghost commented Jul 19, 2020

In my game I found it tedious to use DrawTrianglesWithShader, so I made a simple wrapper function that:

  • Draws the shader to a given image by calling DrawTrianglesWithShader with the correct vertices
  • Draws the image to the screen

Example usage:

screen.DrawImageWithShader(img, shader, op)

// Would be equivelant to:
img.DrawTrianglesWithShader(vs, is, shader, op)
screen.DrawImage(img, op)

Is this how the DrawImageWithShader function would work? Or do you have something else in mind?

@hajimehoshi
Copy link
Owner Author

Yes, basically so.

What I've not determined is whether to handle multiple images. One is passed as the first argument and DrawImageWithShaderOptions might be able to have extra images.

@ghost
Copy link

ghost commented Jul 20, 2020

DrawImageWithShaderOptions might be able to have extra images.

Yes, that is what I implemented in my code:

type DrawImageWithShaderOptions struct {
	GeoM          GeoM
	ColorM        ColorM
	Uniforms      []interface{}
	Images        [4]*Image
	CompositeMode CompositeMode
	Filter        Filter
}

Images is then passed to DrawTrianglesWithShader.

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Jul 20, 2020

ColorM and Filter are not used in the custom shaders.

Images is then passed to DrawTrianglesWithShader.

I believe one image is used in most cases. Then one image is at an argument and three images are in the option, maybe?

@ghost
Copy link

ghost commented Jul 20, 2020

Am I understanding correctly that DrawImageWithShader draws the shader to the image (the argument), or does it draw the shader with the image as a uniform?

@ghost
Copy link

ghost commented Jul 20, 2020

Here is my code for example:

// DrawImageWithShader draws the shader to the given image, then draws the image.
func (i *Image) DrawImageWithShader(img *Image, shader *Shader, options *DrawImageWithShaderOptions) {
	// Calculate the vertices
	w, h := img.Size()
	vs := []Vertex{
		{
			DstX: 0,
			DstY: 0,
			SrcX: 0,
			SrcY: 0,
		},
		{
			DstX: float32(w),
			DstY: 0,
			SrcX: float32(w),
			SrcY: 0,
		},
		{
			DstX: 0,
			DstY: float32(h),
			SrcX: 0,
			SrcY: float32(h),
		},
		{
			DstX: float32(w),
			DstY: float32(h),
			SrcX: float32(w),
			SrcY: float32(h),
		},
	}
	is := []uint16{0, 1, 2, 1, 2, 3}

	// Draw the shader to the image
	img.DrawTrianglesWithShader(vs, is, shader, &DrawTrianglesWithShaderOptions{
		Uniforms: options.Uniforms,
		Images:   options.Images,
	})

	// Draw the image
	i.DrawImage(img, &DrawImageOptions{
		GeoM:          options.GeoM,
		ColorM:        options.ColorM,
		CompositeMode: options.CompositeMode,
		Filter:        options.Filter,
	})
}

@ghost
Copy link

ghost commented Jul 20, 2020

If the shader is not drawn to the image, what about calling the function DrawShader, and keeping all four images in DrawShaderOptions.

Here's an example:

op := &ebiten.DrawShaderOptions{}
op.GeoM.Translate(x, y)
op.Images[0] = img
screen.DrawShader(shader, op)

I think that would be more clear and simple.

@hajimehoshi
Copy link
Owner Author

Am I understanding correctly that DrawImageWithShader draws the shader to the image (the argument), or does it draw the shader with the image as a uniform?

My understanding is latter. This is a counterpart for DrawImage.

If the shader is not drawn to the image, what about calling the function DrawShader, and keeping all four images in DrawShaderOptions.

DrawShader makes much more sense! My concern is that the vertices are not determined without an image size. So probably

screen.DrawShader(shader, width, height, op)

? This seems a little ugly compared to specifying an image unfortunately :-/

@ghost
Copy link

ghost commented Jul 20, 2020

Hmm, I see the problem there.
Perhaps we could require at least one image being passed in Images, and use that to calculate the width and height.

@ghost
Copy link

ghost commented Jul 20, 2020

This seems a little ugly compared to specifying an image unfortunately :-/

I actually prefer this method, because it would allow you to set the size of the shader regardless of the size of the source image. Perhaps the width and height can be specified in DrawShaderOptions?

@hajimehoshi
Copy link
Owner Author

I actually prefer this method, because it would allow you to set the size of the shader regardless of the size of the source image. Perhaps the width and height can be specified in DrawShaderOptions?

I'd want to make the API work without specifying any options. Specifying width/height is mandatory there.

I understand rendering a shader without any images is a common use case. Probably passing the width/height explicitly seems the way to go.

Note that all the images must be the same size in DrawTrianglesWithShader. DrawShader will have the same restriction.

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Jul 20, 2020

Yet another idea:

DrawRectWithShader(shader *Shader, rect image.Rectangle, op *DrawRectWithShaderOptions)

type DrawRectWithShaderOptions struct {
    GeoM          GeoM
    CompositeMode CompositeMode
    Images        [4]*Image
    Uniforms      []interface{}
}

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Jul 20, 2020

If we separate the usage with a GeoM and without:

DrawShader(shader *Shader, op *DrawShaderOptions)

type DrawShaderOptions struct {
    CompositeMode CompositeMode
    Images        [4]*Image
    Uniforms      []interface{}
}

DrawShader renders a shader on the whole area of the target image.

DrawImageWithShader(image *Image, shader *Shader, op *DrawImageWithShaderOptions)

type DrawRectWithShaderOptions struct {
    GeoM          GeoM
    CompositeMode CompositeMode
    Images        [3]*Image
    Uniforms      []interface{}
}

@ghost
Copy link

ghost commented Jul 20, 2020

What about specifying the width and height in the *ebiten.Shader itself?

ebiten.NewShader([]byte(...), width, height)

And you can easily change them with:

shader.Width = width
shader.Height = height

@hajimehoshi
Copy link
Owner Author

Thank you for the suggestion! This seems a little awkward that a shader has its size, but I'm not sure.

@hajimehoshi
Copy link
Owner Author

I understand rendering a shader without any images is a common use case.

I said that, but I started to think this is relatively rare in actual game development compared to using an image. If so, I'd like to go with DrawImageWithShader taking one image as an argument, and let users use DrawTrianglesWithShader when they don't need images. @nanoslayer What do you think?

@ghost
Copy link

ghost commented Jul 20, 2020

I think the problem is that the function gets confusing when you have more than one image.
It would also mean that the size of the shader is dependent on the size of the image.
Is it technically possible for the shader to be drawn to an area larger than the source image?

@hajimehoshi
Copy link
Owner Author

I think the problem is that the function gets confusing when you have more than one image.

As there is a restriction that all the image sizes are same, I don't think this causes confusion.

It would also mean that the size of the shader is dependent on the size of the image.

Yes, but you can adjust this by GeoM.

Is it technically possible for the shader to be drawn to an area larger than the source image?

Then the area is just enlarged like what DrawImage does.

@ghost
Copy link

ghost commented Jul 20, 2020

I guess my only concern is that it is not very elegant to pass one image as an argument and the rest in the options.

Perhaps we could have both DrawShaderWithImage(shader, img, op) and DrawShader(shader, width, height, op), the former being more appropriate when you don't really care about the width and height, and the latter being more explicit and easier to use for multiple images. Maybe DrawShaderWithImage can only accept one image, whereas DrawShader could accept multiple?

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Jul 20, 2020

I guess my only concern is that it is not very elegant to pass one image as an argument and the rest in the options.

Yeah, I understand you point.

As the general solution is already provided by DrawTrianglesWithShader, I'd like to provide a shorthand way for common use cases. My assumption is that the most common use-case is using one image. What about providing DrawImageWithShader(img, shader, op) without Images option? (You seem to prefer DrawShaderWithImage, but I'd want to create DrawImageWithShader as the counterpart for DrawImage)

Perhaps we could have both DrawShaderWithImage(shader, img, op) and DrawShader(shader, width, height, op)

This depends on how common rendering a shader with 0 images or >2 images compared to 1 image. My assumption is that 1 is the most common, and 0 and >2 are not so common. My assumption might be wrong, so I appreciate your opinion :-)

@ghost
Copy link

ghost commented Jul 20, 2020

What about providing DrawImageWithShader(img, shader, op) without Images option?

Yes. I think DrawImageWithShader can be like a shorthand for DrawShader. If you need only one image, you can use DrawImageWithShader. If you want to have more than one image, you'll have to use DrawShader.

I also think that specifying the width and height as an argument in DrawShader might make it more clear that all the images should have that same width and height. Also, in the case of using only one image with DrawImageWithShader, the width and height does not matter as we can just use the width and height of the one image.

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Jul 20, 2020

Yes. I think DrawImageWithShader can be like a shorthand for DrawShader

I think we have reached agreement about DrawImageWithShader, but I'm not convinced about DrawShader.

My question is how often we need to use a shader with 0 or 2> images. If this is not so often, the existing and general DrawTrianglesWithShader should be enough. I'd like to add short-hand APIs only when they are actually common use cases.

@ghost
Copy link

ghost commented Jul 20, 2020

In my game I used a shader to implement lighten blend mode by passing two images, and taking the maximum of the colors. So while passing more than one image to a shader might not be that common, it is still a valid and possible use case.

I think that DrawTrianglesWithShader is kind of tedious to use because you cannot take advantage of ebiten.GeoM with it. Unless you add a function which converts ebiten.GeoM to vertices :P

If you only want to have one function, and want to optimize for the use case of having only one image, then I think having Images in the options is better than having to manually specify vertices and indices just because you need an extra image.

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Jul 20, 2020

Thanks! Actual use cases are great help to understand the situation better! Assuming rendering a rectangle area with a shader is common, what about:

  • DrawImageWithShader(img *Image, shader *Shader, option *...Option) (newly introduced, specifying only one image by the argument)
  • DrawRectangleWithShader(width, height int, shader *Shader, option *...Option) (newly introduced, specifying 0 to 4 images by the option)
  • DrawTrianglesWithShader(vs []Vertex, is []uint16, shader *Shader, option *...Option) (already defined)

?

I think DrawRectangleWithShader is more consistent with the existing functions than DrawShader, which sounds too general.

@ghost
Copy link

ghost commented Jul 20, 2020

Sure, that looks great!

hajimehoshi added a commit that referenced this issue Jul 20, 2020
hajimehoshi added a commit that referenced this issue Jul 20, 2020
@ghost
Copy link

ghost commented Jul 20, 2020

Is it possible for DrawRectangleWithShaderOptions to have a Filter field?

@hajimehoshi
Copy link
Owner Author

No it isn't. You'd need to create your own shader to emulate the filter like this:

#if defined(FILTER_LINEAR)
vec4 color;
highp vec2 texel_size = 1.0 / source_size;
// Shift 1/512 [texel] to avoid the tie-breaking issue.
// As all the vertex positions are aligned to 1/16 [pixel], this shiting should work in most cases.
highp vec2 p0 = pos - (texel_size) / 2.0 + (texel_size / 512.0);
highp vec2 p1 = pos + (texel_size) / 2.0 + (texel_size / 512.0);
# if !defined(ADDRESS_UNSAFE)
p0 = adjustTexelByAddress(p0, source_region);
p1 = adjustTexelByAddress(p1, source_region);
# endif // defined(ADDRESS_UNSAFE)
vec4 c0 = texture2D(T0, p0);
vec4 c1 = texture2D(T0, vec2(p1.x, p0.y));
vec4 c2 = texture2D(T0, vec2(p0.x, p1.y));
vec4 c3 = texture2D(T0, p1);
# if !defined(ADDRESS_UNSAFE)
if (p0.x < source_region[0]) {
c0 = vec4(0, 0, 0, 0);
c2 = vec4(0, 0, 0, 0);
}
if (p0.y < source_region[1]) {
c0 = vec4(0, 0, 0, 0);
c1 = vec4(0, 0, 0, 0);
}
if (source_region[2] <= p1.x) {
c1 = vec4(0, 0, 0, 0);
c3 = vec4(0, 0, 0, 0);
}
if (source_region[3] <= p1.y) {
c2 = vec4(0, 0, 0, 0);
c3 = vec4(0, 0, 0, 0);
}
# endif // defined(ADDRESS_UNSAFE)
vec2 rate = fract(p0 * source_size);
color = mix(mix(c0, c1, rate.x), mix(c2, c3, rate.x), rate.y);
#endif // defined(FILTER_LINEAR)

This requires functions to get texture sizes (#1239).

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Jul 20, 2020

image

I might remove DrawImageWithShader later before releasing 1.12 because the current API set seems verbose and DrawRectangleWithShader is easy enough to use probably. Removing an API after releasing is much much harder than adding one later.

Reopened this until I decided whether to remove DrawImageWithShader.

@hajimehoshi hajimehoshi reopened this Jul 20, 2020
@ghost
Copy link

ghost commented Jul 20, 2020

If you decide to get rid of DrawImageWithShader, should DrawRectangleWithShader be renamed to DrawShader? As DrawRectangleWithShader is really long and verbose.

We could implement the old idea:

screen.DrawShader(shader, width, height, op)

@hajimehoshi
Copy link
Owner Author

If you decide to get rid of DrawImageWithShader, should DrawRectangleWithShader be renamed to DrawShader? As DrawRectangleWithShader is really long and verbose.

I'm not sure, but this is definitely worth considering. The concern is DrawShader might sound too general as I said before, but this would work as a counterpart for DrawImage, maybe.

@hajimehoshi
Copy link
Owner Author

I came up with a good idea:

  • Merge DrawImage and DrawImageWithShader into DrawImage. DrawImageOptions will have Shader and Uniforms. If Shader is not nil, ColorM and Filter are just ignored.
  • Merge DrawTriangles and DrawTrianglesWithShader into DrawTriangles. DrawTrianglesOptions will have Shader and Uniforms. If Shader is not nil, ColorM, Filter and Address are just ignored.
  • Rename DrawRectangleWithShader to DrawRectangle or DrawImages?

@ghost
Copy link

ghost commented Jul 22, 2020

Sounds good! I'm not sure about renaming DrawRectangleWithShader though. The other names are not really clear.

@gabstv
Copy link
Contributor

gabstv commented Jul 22, 2020

maybe DrawRectangle could be in ebitenutil? Like ebitenutil.DrawShaderRect(dst *ebiten.Image, width, height ...)
Since it's a shorthand of DrawTriangles with a single quad?
There is already a ebitenutil.DrawRect that means a different thing than drawing a full sized quad into an image, so I'm not sure about the name.

@ghost
Copy link

ghost commented Jul 22, 2020

I guess renaming DrawRectangleWithShader to DrawRectangle is not that bad. You can just make clear in the documentation that its function is to draw a shader. Or you could rename it to DrawShader, but that sounds too general like you said.

@ghost
Copy link

ghost commented Jul 22, 2020

What about the name DrawShaderRect? @gabstv Oh I didn't notice that you had proposed the same name :D

hajimehoshi added a commit that referenced this issue Jul 23, 2020
@hajimehoshi
Copy link
Owner Author

Merge DrawTriangles and DrawTrianglesWithShader into DrawTriangles.

Oops DrawTriangles takes one image as an argument and this conflicts with multiple images in the option. Let me rethink.

@hajimehoshi
Copy link
Owner Author

  • I gave up merging DrawTriangles and DrawTrianglesWithShader as the former already takes one image as an argument.
  • DrawRectangleWithShader (and DrawTrianglesWithShader) seems unpopular. They were analogy of WithContext. DrawRectShader and DrawTrianglesShader seem slightly better?

hajimehoshi added a commit that referenced this issue Jul 25, 2020
@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Jul 25, 2020

image

In the future, Ebiten exposes the 'default' shaders and might be able to generalize the functions. Until then, the current idea is a good compromise.

@ghost
Copy link

ghost commented Jul 27, 2020

I like the names DrawShaderRect and DrawShaderTriangles better, but otherwise that looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants