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

Make shaders not follow padding; consider moving padding into the renderer #10422

Open
Acurisu opened this issue Jun 14, 2021 · 5 comments
Open
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Milestone

Comments

@Acurisu
Copy link

Acurisu commented Jun 14, 2021

Description of the new feature/enhancement

I'm not sure whether this is by design or not but making the shader only render inside the padded area makes it, in my opinion, look less good. I like my text to be padded without having my shaders cut off where the text area ends.

For smaller paddings one could currently just move the texture a little by sampling at an offset but this will break highlighting at a certain point.

@Acurisu Acurisu added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 14, 2021
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jun 14, 2021
@DHowett
Copy link
Member

DHowett commented Jun 17, 2021

You're right to call this out. We're not in a good place to handle this right now.

Back when we implemented padding, we had the debate about handling it while rendering or outside the renderer.

The downsides of doing it inside the renderer include that we need all hit testing (click position mapping from pixel to glyph cell) to involve the renderer, which is not a particularly lovable architecture.

The upside, of course, is that it would have made this a non-issue. 😄

I do think we'll need to do something here.

@DHowett DHowett added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 17, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 17, 2021
@DHowett DHowett added this to the Terminal Backlog milestone Jun 17, 2021
@DHowett DHowett changed the title Make shaders not follow padding Make shaders not follow padding; consider moving padding into the renderer Jun 17, 2021
@zadjii-msft zadjii-msft modified the milestones: Terminal Backlog, Backlog Jan 4, 2022
@DHowett
Copy link
Member

DHowett commented Sep 12, 2023

Alright, I've been working on this for a while! It's promising.

I wanted to tap in @mrange to ask a question.

I want to finish up the v1 interface for the constant buffer we pass to the pixel shader.

Right now, it contains the following...

float time; // fractional seconds since the shader last "started"
float scale; // display scale as a fraction; 1.0 is 100%
float2 resolution; // **size of the addressable character viewport in pixels**
float4 background; // color of the background fill, if there is one

I've "learned," or at least observed, that passing something to the pixel shader that is in viewport pixel coordinates makes for overcomplicated shader code.

@lhecker suggested that once we factor in padding, we might want to revisit how we pass that stuff to the shader.

  • Is the resolution in pixels useful?
    • Should it be the resolution of the entire rendered surface, rather than just the viewport? This would include gutters and padding
  • Is it valuable to have the viewport?
    • The viewport is smaller than the entire rendered area, due to padding and gutters.
    • Should it be in UV coordinates?
    • From the viewport, you can derive the space occupied by the padding.
    • However, you may not be able to derive the gutters.
    • Is it meaningful to differentiate the padding from the gutters?
  • What about the cursor position and size? And, type? It takes up a full cell, but it has different appearances...
    • UV? Pixel?
  • What else should we include? There's a request in Expose time, date, mouse, ideally CPU / GPU / memory utilization to custom HLSL shaders #9468 for system statistics, but I'm wary of introducing them at the renderer layer. The mouse position could be useful.

@lhecker
Copy link
Member

lhecker commented Sep 12, 2023

Is it meaningful to differentiate the padding from the gutters?

@DHowett I'm not sure the distinction is obvious, since OP refers to what you call "gutters" as the "padding" already. 😅 Personally speaking, it's my first time realizing that you consider them distinct. I considered the extra padding to belong to the gutters so far (= gutters aren't always uniformly sized).

The "gutters" are the 8px margin/padding around all 4 sides of the viewport. (And unless I misunderstood) Dustin is referring to the extra "padding" on the right and bottom sides when the terminal window is maximized. Here's the difference (left is windowed, right is maximized):

image

Edit: Or... Wait. Did I mix gutters and padding up? Ugh I always mix them up. I call them both just the "margin", because that's how CSS would call it.


Couple more ideas: We could "standardize" the names of the parameters. That way we could dynamically build the constant buffer with whatever information the shader wants and it would still work if we add more fields in the future.

It'd be great if we could allow for compute shaders too, since those require an entirely different setup, but are way more flexible. I suspect that true shader freedom requires a more complex setup with separate metadata (but I'm also not an expert with D3DReflect so maybe it is possible to cram all metadata into the shader files).

@mrange
Copy link
Contributor

mrange commented Sep 13, 2023 via email

@Welding-Torch
Copy link

Hi there, would love to see an update on this as the gutters/padding/margins are still breaking the immersion of using some shaders 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

6 participants