Skip to content

WIP: Draw backgrounds separately from foregrounds#6224

Closed
DHowett wants to merge 3 commits intomasterfrom
dev/duhowett/background-atlas
Closed

WIP: Draw backgrounds separately from foregrounds#6224
DHowett wants to merge 3 commits intomasterfrom
dev/duhowett/background-atlas

Conversation

@DHowett
Copy link
Copy Markdown
Member

@DHowett DHowett commented May 27, 2020 •
edited
Loading

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

DHowett
DHowett commented May 27, 2020
View reviewed changes
src/renderer/base/renderer.cpp
(void)target;
(void)it;
(void)pEngine;
#if 0
Copy link
Copy Markdown
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.

these, and their guarded regions, wil be deleted

DHowett
DHowett commented May 27, 2020
View reviewed changes
src/buffer/out/TextAttribute.hpp
(!checkForeground && _background == other._background));
}

// This returns whether this attribute, if printed directly next to another attribute, for the space
Copy link
Copy Markdown
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.

wrong. need to resurrect this tho

@DHowett
Copy link
Copy Markdown
Member Author

DHowett commented May 27, 2020

This will be cut deep by the 2661 fix.,

DHowett
DHowett commented May 27, 2020
View reviewed changes
src/renderer/base/renderer.cpp
}

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

@DHowett DHowett May 27, 2020 •
edited
Loading

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 DHowett mentioned this pull request May 27, 2020
Merged
11 tasks
@DHowett
Copy link
Copy Markdown
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).

@zadjii-msft zadjii-msft mentioned this pull request Jun 1, 2020
Closed
@DHowett DHowett closed this Jun 15, 2020
@miniksa miniksa reopened this Jun 15, 2020
@miniksa
Copy link
Copy Markdown
Member

miniksa commented Jun 15, 2020

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

@miniksa miniksa self-assigned this Jun 15, 2020
@miniksa miniksa mentioned this pull request Jun 15, 2020
Merged
@DHowett
Copy link
Copy Markdown
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

Reviewers

No reviews

Assignees

@miniksa miniksa

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@DHowett @miniksa