kovidgoyal / kitty

Cross-platform, fast, feature-rich, GPU based terminal
https://sw.kovidgoyal.net/kitty/
GNU General Public License v3.0
24.15k stars 972 forks source link

Cell background color not drawn on top of images, regardless of Z index #2264

Closed ctrlcctrlv closed 4 years ago

ctrlcctrlv commented 4 years ago

The Kitty graphics protocol states:

Negative z-index values mean that the images will be drawn under the text.

I interpreted this to mean that text with background colors would have the background color drawn over the text, if not at z = -1, then certainly at z = -2.

This is, however, not true.

Observe: (command requires #2263)

kitty/launcher/kitty +icat --place 50x50@0x0 --z-index -10 /tmp/IM%02-1.jpg

2020-01-08-201447_1637x1760_scrot

I see this as a bug, it certainly complicates #163. Thoughts, @kovidgoyal?

kovidgoyal commented 4 years ago

No I intended negative z-index to mean images are drawn between the background color and the foreground color, i.e. under the text but not under the cells. You can see that in the example screenshot here https://sw.kovidgoyal.net/kitty/graphics-protocol.html

And I dont think this should change. The reason being that it would make it impossible to render text without background on top of images. While if you really want to render the text on a solid color on top of an image, you can do so by using a rectangle image with the solid color.

That is indeed another reason to special case handling of background images, that I forgot about.

ctrlcctrlv commented 4 years ago

Sorry, really don't agree. I think -1 should be the z-index of the cell backgrounds, so images drawn on -2 should be below it. It's true you can draw rectangles but that's needlessly complicated when we can just define the cell background's z-index.

kovidgoyal commented 4 years ago

It certainly shouldn't be -1 if you do want to define it as something other than -Infinity it should be MIN_ZINDEX / 2

ctrlcctrlv commented 4 years ago

Are you OK with me defining it as MIN_ZINDEX / 2?

kovidgoyal commented 4 years ago

On Wed, Jan 08, 2020 at 05:06:36AM -0800, Fredrick Brennan wrote:

Are you OK with me defining it as MIN_ZINDEX / 2?

Yes, remember to update the protocol documentation accordingly as well. It's in graphics-protocol.rst

ctrlcctrlv commented 4 years ago

Thank you Kovid. Really appreciate your quick replies. It's 9PM now so I'm going to bed, but I'll have more for you soon.

After this, I've got to get back to my FontForge work, but I'll probably end up back in Kitty land in future some day, since I use it as my terminal :P

kovidgoyal commented 4 years ago

On Wed, Jan 08, 2020 at 05:41:59AM -0800, Fredrick Brennan wrote:

Thank you Kovid. Really appreciate your quick replies. It's 9PM now so I'm going to bed, but I'll have more for you soon.

Cool :)

After this, I've got to get back to my FontForge work, but I'll probably end up back in Kitty land in future some day, since I use it as my terminal :P

It will be around.

ctrlcctrlv commented 4 years ago

If you have time, would you mind giving me some pointers as to how to implement this? Simply reordering draw_cells_interleaved does not work:

diff --git a/kitty/shaders.c b/kitty/shaders.c
index a8c8e7b7..dc6b74bd 100644
--- a/kitty/shaders.c
+++ b/kitty/shaders.c
@@ -390,13 +390,14 @@ draw_cells_simple(ssize_t vao_idx, ssize_t gvao_idx, Screen *screen) {

 static void
 draw_cells_interleaved(ssize_t vao_idx, ssize_t gvao_idx, Screen *screen) {
-    bind_program(CELL_BG_PROGRAM);
-    glDrawArraysInstanced(GL_TRIANGLE_FAN, 0, 4, screen->lines * screen->columns);
     glEnable(GL_BLEND);
     BLEND_ONTO_OPAQUE;
-
+    
     if (screen->grman->num_of_negative_refs) draw_graphics(GRAPHICS_PROGRAM, vao_idx, gvao_idx, screen->grman->render_data, 0, screen->grman->num_of_negative_refs);

+    bind_program(CELL_BG_PROGRAM);
+    glDrawArraysInstanced(GL_TRIANGLE_FAN, 0, 4, screen->lines * screen->columns);
+
     bind_program(CELL_SPECIAL_PROGRAM);
     glDrawArraysInstanced(GL_TRIANGLE_FAN, 0, 4, screen->lines * screen->columns);

I've tried a bunch of other stuff as well, including setting glBlendFunc(GL_ONE_MINUS_DST_ALPHA, GL_DST_ALPHA);, which seemed promising because that works here, but am not really an expert in OpenGL.

It seems like CELL_BG_PROGRAM is drawing black background for cells, even when it shouldn't? Or am I doing something wrong? How do I see these "programs" anyway? gl.c contains a Program programs[64] but that doesn't really help me.

ctrlcctrlv commented 4 years ago

I should mention that reordering does work if opacity = 0.0:

2020-01-09-122031_2912x1790_scrot

In this case it's simple reordering of draw_cells_interleaved_premult.

Of course I won't submit to you a PR w/just a simple reordering, but you know how it goes. I have to get it working once on my end before worrying about making the API nicer :)

kovidgoyal commented 4 years ago

See the big comment in cell_fragment.glsl for an explanation of rendering. The programs are all cell_vertex.glsl + cell_fragment.glsl with different defines. The defines are set and the programs are compiled in window.py

You need to think about what rendering behind background actually means. All cells have a background. That is by default black. So you cant actually render under the background. The way it work with a transparent window is the shader sets the background opacity to zero for cells with the default background. So essentially no background is rendered for cells that have the default background. You will need to do the same, when rendering images under bg.

ctrlcctrlv commented 4 years ago

Thank you Kovid for pointing me to the right files, will do