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

border: Avoid generating empty segments. #3056

Closed
wants to merge 2 commits into from
Closed

border: Avoid generating empty segments. #3056

wants to merge 2 commits into from

Conversation

@emilio
Copy link
Member

emilio commented Sep 13, 2018

This is a bit less invasive and should do the job.

I've confirmed that all the negative sizes came from edges, when a huge radii
with radius > rect.size - cw_border.width - ccw_border.width caused avail_width to be negative.

Still we're creating a gazillion empty borders which are useless, so prevent
that too by checking in add_edge_segment and add_corner_segment.

I've confirmed that this passes the reftests that are failing in #3049, but waiting for try just to double-check.


This change is Reviewable

emilio added 2 commits Sep 13, 2018
Instead, just add functions to add the instances directly, avoiding the
intermediate allocation and deindenting a bunch of the code.

This should have no behavior change.
This is a bit less invasive and should do the job.

I've confirmed that all the negative sizes come from edges, when a huge radii
with radius > rect.size - cw_border.width - ccw_border.width.

Still we're creating a gazillion empty borders which are useless, so prevent
that too.
@emilio
Copy link
Member Author

emilio commented Sep 13, 2018

r? @kvark (only last commit of course)

@kvark
Copy link
Member

kvark commented Sep 13, 2018

@emilio ideally, the first commit should be left with me as an author. If you rebased on the PR, you'd have it right.

@emilio
Copy link
Member Author

emilio commented Sep 13, 2018

So I was seeing the same failures on our automation, and I debugged again and I understand the issue now. Will file an issue, but there's no point in keeping this open for now.

The TL;DR: is that nothing the output of BorderRenderTaskInfo::new can depend on anything that isn't in our BorderCacheKey, as of right now. Will mention why in a second.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.