-
Notifications
You must be signed in to change notification settings - Fork 91
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
common: stroke dash offset support added #1607
Conversation
sample:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. a nitpick. please check a comment.
This reverts commit 6cbc1de. Setting def value for 'a' makes it impossible to overload the 'stroke' api with 3 values (needed for introducing dash offset).
86e8b13
to
4a031a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is beneficial to set a default value at Shape::fill() . I believe that reverting it is not a good idea.
src/bindings/capi/thorvg_capi.h
Outdated
@@ -1339,6 +1339,24 @@ TVG_API Tvg_Result tvg_shape_get_stroke_gradient(const Tvg_Paint* paint, Tvg_Gra | |||
TVG_API Tvg_Result tvg_shape_set_stroke_dash(Tvg_Paint* paint, const float* dashPattern, uint32_t cnt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please kill the old version of the api. I think we don't need to maintain both apis.
The feature is supported also in the svg loader. @issue: thorvg#1591
tvg_shape_set_stroke_dash() and tvg_shape_get_stroke_dash() require an extra argument: offset
4a031a6
to
046cedc
Compare
|
||
|
||
/*! | ||
* \brief Gets the dash pattern of the stroke. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hermet I added BETA_API - not sure whether it's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it's ready to official release then you don't need to add BETA_API tag. Otherwise you should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now this breaks the api compatibility.
* | ||
* @return Result::Success when succeed, Result::FailedAllocation otherwise. | ||
*/ | ||
Result stroke(uint8_t r, uint8_t g, uint8_t b, uint8_t a = 255) noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hermet our policy is that we can remove the def value from api just like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have two options:
- bring a new api name about stroke dash
- change the existing conflict api.
You know, you already suggested this change. I have no any objection of this.
Abou policy? Well... you know most apis have no default values but if it's useful for users then why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job!
|
||
|
||
/*! | ||
* \brief Gets the dash pattern of the stroke. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now this breaks the api compatibility.
The feature is supported also in the svg loader.
@issue: #1591