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

Add gfx::RectT slicing methods #102

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

martincapello
Copy link
Member

@martincapello martincapello commented Jul 31, 2024

I need these methods to implement aseprite/aseprite#4533

I have also added tests for them.

I'm not sure if you @dacap want this to be added here in laf. So let me know if you think I should move them to some aseprite utilities methods.

Maybe we can keep the sliceV and sliceH methods and move the nineSlice method. Idk whatever you say is okay.

@aseprite-bot
Copy link
Collaborator

clang-tidy review says "All clean, LGTM! 👍"

@dacap
Copy link
Member

dacap commented Jul 31, 2024

@martincapello in which way these methods are used? probably a gfx::Region can be used in this case/to calculate the rectangles (because we can subtract the center from the gfx::Region and get each band.

@martincapello
Copy link
Member Author

I'm using the nineSlice method to split Aseprite's 9-slices to get each rect that I have to use to split the image under the each part of the 9-slice slice. Also, I need it to split the 9-slice bounds while the user is modifying its center/size, so I can use each rect to scale each part of the original image according to the new size of each slice's part.

@martincapello
Copy link
Member Author

I think that maybe I could try to use a gfx::Region...I'm not sure how convenient it would be, but I can try if you want.

@dacap
Copy link
Member

dacap commented Jul 31, 2024

As I don't have the code at hand I cannot decide what is best for your case.

In case Region is not useful for your case, and you need these functions, I'd change the API to make all these functions const and return *this, the result could go to the parameters (as an array as in nineSlice() or as a couple of references, you could specify the same instance in one of the outputs too, but it's the API client decision).

nineSlice() can return *this too (remember to pass const RectT<T>& center).

@martincapello
Copy link
Member Author

martincapello commented Jul 31, 2024

As I don't have the code at hand I cannot decide what is best for your case.

I will push it soon.

In case Region is not useful for your case, and you need these functions, I'd change the API to make all these functions const and return *this, the result could go to the parameters (as an array as in nineSlice() or as a couple of references, you could specify the same instance in one of the outputs too, but it's the API client decision).

I have been playing around with https://fiddle.skia.org/ and trying the difference operator to subtract the center rect from a region...and it seems that it would not fit my case very well (it would make the code more complex actually). Alright, I will update the API. Also we can just move this to Aseprite if you are not sure that they belong to Laf. Mostly because nineSlice seems too specific to my use case...the sliceV and sliceH feels more generic or primitive...

nineSlice() can return *this too (remember to pass const RectT<T>& center).

Oh yeah, missed that one, thanks.

@aseprite-bot
Copy link
Collaborator

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@aseprite-bot
Copy link
Collaborator

clang-tidy review says "All clean, LGTM! 👍"

Now all the slice methods are const and return *this. Also
sliceV and sliceH now use two references to return the results
@aseprite-bot
Copy link
Collaborator

clang-tidy review says "All clean, LGTM! 👍"

@dacap dacap merged commit 90d292e into aseprite:beta Aug 9, 2024
11 checks passed
dacap added a commit that referenced this pull request Aug 9, 2024
@dacap
Copy link
Member

dacap commented Aug 9, 2024

I'll revert this commit force pushing laf beta, this commit should have gone to the main branch.

dacap pushed a commit to dacap/laf that referenced this pull request Aug 9, 2024
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