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

WIP: Draw backgrounds separately from foregrounds #6224

Closed
wants to merge 3 commits into from

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented May 27, 2020

This implements #6193. It's a WIP.

Summary of the Pull Request


Alright, I've got this down to a single function that takes each dirty region and produces...

  • A list of damaged backgrounds to repaint
  • A list of colored cluster lists to repaint

I don't like the number of vectors here, but... we're saving money by invalidating narrowly whenever possible.

Dirty Region [1..n]
- Origin (SCREEN CHAR COORDS)
- At line wrap point
- Region intersected right half of DBCS char (left trim)
- Background Run [1..n]
  - Attribute
  - Rect Covered (SCREEN CHAR COORDS)
- Colored Foreground Run [1..n]
  - Attribute
  - Columns
  - Cluster [1..n]

Right now, because of some minor issues (#2661), backgrounds are attributes and actual final colors. It's not pleasant!

To fix:

  • reverse is occasionally (!) broken
  • render engines need flags to say "plz no background splitting" (vt engine needs to update drawing brushes per text run, foreground and background)
  • need to re-enable space optimization (if the cell is non-printing (and has no underline, etc.), no point in forwarding it to the cluster renderer!)

References

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

(void)target;
(void)it;
(void)pEngine;
#if 0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these, and their guarded regions, wil be deleted

(!checkForeground && _background == other._background));
}

// This returns whether this attribute, if printed directly next to another attribute, for the space
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong. need to resurrect this tho

@DHowett
Copy link
Member Author

DHowett commented May 27, 2020

This will be cut deep by the 2661 fix.,

@@ -104,6 +104,53 @@ Renderer::~Renderer()
return S_OK;
}

template<>
struct fmt::formatter<TextColor>
Copy link
Member Author

@DHowett DHowett May 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need these to be in a this place, but we could move them into their appropriate headers later

i'd probably just delete em

@DHowett
Copy link
Member Author

DHowett commented May 27, 2020

Merge in work from the #803 bugfix -- also, maybe we should generalize BackgroundRun to RenderableAttributeRun! This would let us katamari together runs line drawing characters and break them only when they change.

We want to consider how this interacts with underline (part of foreground).

@miniksa
Copy link
Member

miniksa commented Jun 15, 2020

reopening and assigning to myself, I might need this for perf.

@DHowett
Copy link
Member Author

DHowett commented Jul 1, 2020

Don't need to keep open, will post branch name in issue

@DHowett DHowett closed this Jul 1, 2020
@DHowett DHowett deleted the dev/duhowett/background-atlas branch October 26, 2021 16:59
@DHowett DHowett restored the dev/duhowett/background-atlas branch October 26, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants