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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

SKRegion: Add constructors and methods for easier interaction with SKRectI and SKPath #788

Merged
merged 1 commit into from Mar 13, 2019

Conversation

@vexx32
Copy link
Contributor

commented Feb 10, 2019

  1. Cleaned up SKRegion file in terms of style. Style was inconsistent at best in this file.
    • To assist with this in future, I have added omnisharp.json to the project root, which OmniSharp can utilize to format the C# source files automatically. Nobody should need to deal with repo style on a manual basis where possible.
    • If this file doesn't quite meet every standard set in the style guide, please feel free to alter the rules laid out therein. I just want to make it as easy as possible for potential contributors to get it right the first time. 馃槉
  2. Added some constructors to SKRegion:
    • new SKRegion(SKRect rect) -- creates a new region and calls SetRect(rect)
    • new SKRegion(SKPath path) -- creates a new region and calls SetPath(path)
  3. Altered SetPath(SKPath path) method to utilize SKRectI.Ceiling() method rather than use a custom implementation. It seemed inconsistent and tricky to verify that the existing method would work as intended, and moving to an existing, standardized, and (hopefully) tested method seems like a better move. Let me know if there's a specific reason it was done the way it was, happy to learn here.
  4. Added Intersects(SKPath path) method to complement the existing SKRectI and SKRegion Intersects overloads.
  5. Added Op(SKPath path, SKRegionOperation op) method to complement the existing similar methods for SKRectI and SKRegion.

Resolves #778 (for the most part... I'd need someone to talk me through the native methods and if/why they demand integer values for SKRegion bounds in order to look at potentially allowing direct use of SKRect with SKRegion without rounding values all over the place.)

@mikekinsman

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

OPS Build status updates of commit f90f2f5:

馃暀 Preparing: average preparing time is 1 min(s) 59 sec(s)

@mikekinsman

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

OPS Build status updates of commit f90f2f5:

馃暀 Incremental building: average incremental building time is 19 sec(s)

@mikekinsman

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

OPS Build status updates of commit f90f2f5:

Validation status: passed

File Status Preview URL Details
binding/Binding/SKRegion.cs Succeeded
omnisharp.json Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

@charlesroddie

This comment has been minimized.

Copy link

commented Feb 21, 2019

@vexx32 Since this PR is two distinct things (1. constructors and methods and 2. formatting), wouldn't it make sense to split it in two?

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Yeah, that's a pretty good point. I'll split the style stuff out tonight or tomorrow and put that in a separate PR :)

鈾伙笍 Update SKRegion constructors and methods
 Add new methods and ctors for path shenanigans
鈾伙笍 Make SetPath(SKPath p) more reliable, hopefully

@vexx32 vexx32 force-pushed the vexx32:RegionTools branch from f90f2f5 to bf4c54a Feb 22, 2019

@mikekinsman

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

OPS Build status updates of commit bf4c54a:

馃暀 Preparing: average preparing time is 1 min(s) 58 sec(s)

@mikekinsman

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

OPS Build status updates of commit bf4c54a:

馃暀 Incremental building: average incremental building time is 19 sec(s)

@mikekinsman

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

OPS Build status updates of commit bf4c54a:

Validation status: passed

File Status Preview URL Details
binding/Binding/SKRegion.cs Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@charlesroddie removed the style-related commits from this PR. Style-related commits have been moved to #795 馃槃

@mattleibow mattleibow merged commit bf4c54a into mono:master Mar 13, 2019

14 of 19 checks passed

SkiaSharp #20190222.1 failed
Details
SkiaSharp-ci Build #20190221.2 has failed
Details
SkiaSharp-ci (Build Managed (Linux)) Build Managed (Linux) failed
Details
SkiaSharp-ci (Build Managed (Windows)) Build Managed (Windows) failed
Details
SkiaSharp-ci (Build Managed (macOS)) Build Managed (macOS) failed
Details
OpenPublishing.Build Validation status: passed
Details
SkiaSharp-ci (Build Native Android (Windows)) Build Native Android (Windows) succeeded
Details
SkiaSharp-ci (Build Native Android (macOS)) Build Native Android (macOS) succeeded
Details
SkiaSharp-ci (Build Native Linux (Linux)) Build Native Linux (Linux) succeeded
Details
SkiaSharp-ci (Build Native Tizen (Linux)) Build Native Tizen (Linux) succeeded
Details
SkiaSharp-ci (Build Native Tizen (Windows)) Build Native Tizen (Windows) succeeded
Details
SkiaSharp-ci (Build Native Tizen (macOS)) Build Native Tizen (macOS) succeeded
Details
SkiaSharp-ci (Build Native UWP (Windows)) Build Native UWP (Windows) succeeded
Details
SkiaSharp-ci (Build Native Win32 (Windows)) Build Native Win32 (Windows) succeeded
Details
SkiaSharp-ci (Build Native iOS (macOS)) Build Native iOS (macOS) succeeded
Details
SkiaSharp-ci (Build Native macOS (macOS)) Build Native macOS (macOS) succeeded
Details
SkiaSharp-ci (Build Native tvOS (macOS)) Build Native tvOS (macOS) succeeded
Details
SkiaSharp-ci (Build Native watchOS (macOS)) Build Native watchOS (macOS) succeeded
Details
license/cla All CLA requirements met.
Details

@vexx32 vexx32 deleted the vexx32:RegionTools branch Mar 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.