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

Change Rect's expand/trunc to more useful methods. #107

Merged
merged 1 commit into from
May 7, 2020

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented May 5, 2020

I added the expand / trunc methods to various shapes back in #93. They've been quite useful, especially for Size in my personal usage. However the implementations for Rect were naive. Although their behavior was correctly documented and so not a bug per se, I think the behavior is questionable. That is to say, it's not the Rect itself that gets expanded or truncated, its the individual coordinates.

While working on DPI improvements in druid I found a need for methods that expand the Rect itself, not its coordinates. I also found an existing usage of Rect::expand in the GTK druid-shell code. That specific usage expects the Rect itself to be expanded and so is buggy right now.

I also found usage of Rect::expand in the blurred rects in piet, here and here. I haven't really analyzed the blurred rects code to know if it expects the coordinates to be expanded or the Rect itself. I would imagine the Rect itself, but perhaps @raphlinus can chime in here.

This PR changes the behavior of Rect::expand and Rect::trunc to what I think are less surprising and more useful methods. They perform their action on the Rect itself.

I made the following illustration to show the change in behavior:

kurbo-rect

These new functions also support a reversed orientation Rect, e.g. Rect::new(10, 10, 5, 5). That is supported via two if statements. As a pleasant surprise LLVM seems to optimize the branches away. I made three different implementations of the functions, two of them branchless. Benchmarks showed no improvement over the much simpler if version and after briefly inspecting the assembly it seemed like an optimization success story indeed. I kept the bench code in the PR, but I can delete it if we don't expect future optimization attempts and want to keep the repository clean.

@raphlinus
Copy link
Contributor

I didn't look at the code yet (I can if it would be helpful), but can confirm that I expected the behavior to be "rectangle that encloses the input with integer coordinates." It also happens to be the case that for blurred rectangles one pixel is unlikely to make a difference, but that's only an argument for why I didn't find this before. I also use it in piet-gpu for bounding boxes.

Technically this should be a breaking change, in case anybody actually was depending on the older behavior.

@xStrom
Copy link
Member Author

xStrom commented May 5, 2020

Yeah it's a breaking change in theory, but I have a hunch that in practice it's more of a fixing change. 😄

Although I'm not sure what it means if it's a breaking change, given that there's no changelog and the major version is zero. I do know that I'm in full support of doing this breaking/fixing.

@raphlinus
Copy link
Contributor

Although I'm not sure what it means if it's a breaking change, given that there's no changelog and the major version is zero. I do know that I'm in full support of doing this breaking/fixing.

Due to the way Rust semver works, going to 0.6.0 for the next release addresses my concern.

@cmyr
Copy link
Member

cmyr commented May 7, 2020

Is that a +1 from you @raphlinus?

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

Yup, looks good, thanks! I think I'm running into this now in piet-gpu, will snap to the newer version when merged :)

@cmyr cmyr merged commit 29eea23 into linebender:master May 7, 2020
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.

3 participants