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

feat: per-side border widths #836

Merged
merged 28 commits into from
Sep 28, 2024

Conversation

Tropix126
Copy link
Contributor

@Tropix126 Tropix126 commented Aug 31, 2024

Adds support for individual border-widths per side. Made based on observation of figma's stroke rendering system.

Basic Rundown

Rather than using Skia's stroking API, we instead draw the border as a second path (the "border path") with either a larger or smaller corner radius (the offset for roundness is calculated by RectElement::border_corner_radius_offset). We then mask out a smaller rect (the "border clip path") from the inside of the border rect.

What this gives us is the ability to offset the border rect allowing for different widths on each side.

Current Limitations

  • The parser requires the user to input exactly four width values and no less currently. This means all examples and devtools are currently broken. I need to find a way to parse the widths in a clean way.
  • Devtools are broken as mentioned before.
  • Zero-width borders bleed through from under the rect if they happen to render on a subpixel. This is a skia bug and will be fixed avoided once pixel snapping is implemented. (feat: Physical pixel snapping #837)

Some cursed showcases

Superellipse roundness with variable widths on each side. (this rendering is actually more correct than figma's, which doesn't do squircle calculations on borders :D)
image

Variable widths with variable corner radius.
image

@marc2332 marc2332 added the enhancement 🔥 New feature or request label Sep 1, 2024
@Tropix126 Tropix126 marked this pull request as ready for review September 13, 2024 16:35
@Tropix126
Copy link
Contributor Author

Linux build is failing for reasons I can't determine. The build also took 49 minutes :p

@marc2332
Copy link
Owner

Linux build is failing for reasons I can't determine. The build also took 49 minutes :p

Check the skia bindings

Copy link

codecov bot commented Sep 14, 2024

Codecov Report

Attention: Patch coverage is 83.12655% with 68 lines in your changes missing coverage. Please review.

Project coverage is 76.01%. Comparing base (edc1af2) to head (42a6593).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/core/src/elements/rect.rs 71.42% 56 Missing ⚠️
crates/state/src/values/border.rs 89.91% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #836      +/-   ##
==========================================
+ Coverage   75.76%   76.01%   +0.25%     
==========================================
  Files         207      207              
  Lines       23514    23831     +317     
==========================================
+ Hits        17816    18116     +300     
- Misses       5698     5715      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tropix126 Tropix126 mentioned this pull request Sep 15, 2024
Copy link
Owner

@marc2332 marc2332 left a comment

Choose a reason for hiding this comment

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

Would be nice to have the border.rs example update showing this new thing right?

Copy link
Owner

@marc2332 marc2332 left a comment

Choose a reason for hiding this comment

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

this branch:
image

main:
image

🤔

@Tropix126
Copy link
Contributor Author

Rendering quality has been fixed. Example has been updated as part of #889 (which is dependent on this PR).

@marc2332 marc2332 self-requested a review September 24, 2024 09:03
@marc2332 marc2332 added this to the 0.3.0 milestone Sep 27, 2024
crates/state/src/values/border.rs Outdated Show resolved Hide resolved
@marc2332 marc2332 self-requested a review September 28, 2024 15:35
examples/border.rs Show resolved Hide resolved
@marc2332 marc2332 merged commit b671d55 into marc2332:main Sep 28, 2024
4 checks passed
marc2332 added a commit that referenced this pull request Sep 28, 2024
* feat: per-side border widths

* refactor: make `BorderWidth::is_all_zero` more sane

* fix: wrong operator in `is_all_zero`

* implement parsing for per-side widths

* feat: impl `Display` for `Border`

* fix lints

* fix another lint

* chore: fmt

* fix tests

* fix: add `RRect::rect` to mocked engine

* chore: improve border test coverage

* fix: border test assertion

* fix: radius offset calculations for different border alignments

* clean up border drawing code

* fmt

* revert needless `get_rounded_rect` name change

* format

* add `PathFillType` to mocked engine

* remove `border_align` and place it in main syntax

* add support for multiple borders per element

* mock `Path::set_fill_type`

* update docs

* update `border` attribute documentation

* fix some poor wording in the border docs

* fmt

* fix jagged antialiasing on borders

* lint

* fmt

* mock `Canvas::draw_drrect` and `RRect::bounds`

* update border example

* fix borders not rendering with no background

* feat: per-side border widths (#836)

* feat: per-side border widths

* refactor: make `BorderWidth::is_all_zero` more sane

* fix: wrong operator in `is_all_zero`

* implement parsing for per-side widths

* feat: impl `Display` for `Border`

* fix lints

* fix another lint

* chore: fmt

* fix tests

* fix: add `RRect::rect` to mocked engine

* chore: improve border test coverage

* fix: border test assertion

* fix: radius offset calculations for different border alignments

* clean up border drawing code

* fmt

* revert needless `get_rounded_rect` name change

* format

* add `PathFillType` to mocked engine

* mock `Path::set_fill_type`

* update docs

* fix jagged antialiasing on borders

* lint

* mock `Canvas::draw_drrect` and `RRect::bounds`

* simplify border visibility check

Co-authored-by: Marc Espin <[email protected]>

---------

Co-authored-by: Marc Espin <[email protected]>

* fix: checkbox component

* feat: per-side border widths

* refactor: make `BorderWidth::is_all_zero` more sane

* fix: wrong operator in `is_all_zero`

* implement parsing for per-side widths

* feat: impl `Display` for `Border`

* fix lints

* fix another lint

* chore: fmt

* chore: improve border test coverage

* fix: border test assertion

* fix: radius offset calculations for different border alignments

* clean up border drawing code

* fmt

* revert needless `get_rounded_rect` name change

* format

* remove `border_align` and place it in main syntax

* add support for multiple borders per element

* update docs

* update `border` attribute documentation

* fix some poor wording in the border docs

* fmt

* fix jagged antialiasing on borders

* lint

* fmt

* update border example

* fix borders not rendering with no background

* fix: stray `border_align`

---------

Co-authored-by: Marc Espin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🔥 New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants